[webkit-reviews] review denied: [Bug 36721] Return correct load result to NPAPI plugins calling getUrlNotify on target frames : [Attachment 97697] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Robert Hogan <robert at webkit.org>'s
request for review:
Bug 36721: Return correct load result to NPAPI plugins calling getUrlNotify on
target frames
https://bugs.webkit.org/show_bug.cgi?id=36721

Attachment 97697: Patch
https://bugs.webkit.org/attachment.cgi?id=97697&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97697&action=review

This patch needs a lot of work.  The use of RefCounted types is largely
incorrect and this patch appears to dump junk into FrameLoader.  We're trying
to improve FrameLoader by removing unrelated concerns from the class.

> Source/WebCore/loader/DocumentLoader.cpp:919
> +PassRefPtr<PluginNotification> DocumentLoader::pluginNotification()
> +{
> +    return m_pluginNotification;

If you're going transfer ownership here, you should call
m_pluginNotification.release(), which will zero out m_pluginNotification and
return that reference so that it can be transferred to another RefPtr without
churning the reference count.  As written, this code neither zeros out
m_pluginNotification nor avoid churning the reference count and is therefore
incorrect.

> Source/WebCore/loader/FrameLoader.cpp:816
> +    childFrame->loader()->setPluginNotification(m_pluginNotification);
> +    m_pluginNotification.clear();

This pattern should be replaced with:

childFrame->loader()->setPluginNotification(m_pluginNotification.release());

which accomplishes the same thing by avoids needlessly increasing and
decreasing the reference count of m_pluginNotification.

> Source/WebCore/loader/FrameLoader.cpp:1194
> +	   targetFrame->loader()->setPluginNotification(m_pluginNotification);
> +	   m_pluginNotification.clear();

ditto

> Source/WebCore/loader/FrameLoader.cpp:1244
> +void FrameLoader::load(const ResourceRequest& request, bool lockHistory,
PassRefPtr<PluginNotification> notification)
> +{
> +    m_pluginNotification = notification;
> +    load(request, lockHistory);
> +}

Please do not add another FrameLoader::load method.  We have too many of these
already and they are too confusing.  Please find another way to accomplish this
task.

> Source/WebCore/loader/FrameLoader.cpp:1261
> +    loader->setPluginNotification(m_pluginNotification);
> +    m_pluginNotification.clear();

ditto

> Source/WebCore/loader/FrameLoader.cpp:1272
> +void FrameLoader::load(const ResourceRequest& request, const String&
frameName, bool lockHistory, PassRefPtr<PluginNotification> notification)
> +{
> +    m_pluginNotification = notification;
> +    load(request, frameName, lockHistory);
> +}

Please don't add another FrameLoader::load method.

> Source/WebCore/loader/FrameLoader.cpp:1284
> -	   frame->loader()->load(request, lockHistory);
> +	   frame->loader()->load(request, lockHistory, m_pluginNotification);
> +	   m_pluginNotification.clear();

Another example of a missing call to release().

> Source/WebCore/loader/FrameLoader.cpp:1296
> +    loader->setPluginNotification(m_pluginNotification);
> +    m_pluginNotification.clear();

ditto

> Source/WebCore/loader/FrameLoader.cpp:2331
> +void FrameLoader::notifyPlugin(PassRefPtr<PluginNotification> notification,
const ResourceError& error)
> +{
> +    RefPtr<PluginNotification> notify = notification;
> +    if (!notify || notify->cancelled())
> +	   return;
> +
> +    // Mac and Chromium look after url notifications to plugins themselves
> +#if !PLATFORM(MAC) && !PLATFORM(CHROMIUM)
> +    notify->view()->urlNotify(notify, error);
> +#else
> +    UNUSED_PARAM(notification);
> +    UNUSED_PARAM(error);
> +#endif
> +}

This code seems unrelated to FrameLoader.  Please don't just dump random junk
into FrameLoader.

> Source/WebCore/loader/FrameLoader.cpp:2643
> +	      
targetFrame->loader()->setPluginNotification(m_pluginNotification);
> +	       m_pluginNotification.clear();

ditto

> Source/WebCore/loader/FrameLoader.cpp:2933
> +    mainFrame->loader()->setPluginNotification(m_pluginNotification);
> +    m_pluginNotification.clear();

ditto