Bug 36721 – Return correct load result to NPAPI plugins calling getUrlNotify on target frames

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Robert Hogan]

plugins/get-url-with-blank-target.html is currently skipped by all platforms except Mac. Chromium introduced a new version of test that works for Qt and possibly other platforms.

The Mac result expects an NP_ERROR_GENERIC whereas Chromium/Qt expect NP_NO_ERROR, which looking at PluginView.cpp is how it should be:

NPError PluginView::load(const FrameLoadRequest& frameLoadRequest, bool sendNotification, void* notifyData)
{
    ASSERT(frameLoadRequest.resourceRequest().httpMethod() == "GET" || frameLoadRequest.resourceRequest().httpMethod() == "POST");

    KURL url = frameLoadRequest.resourceRequest().url();
    
    if (url.isEmpty())
        return NPERR_INVALID_URL;

    // Don't allow requests to be made when the document loader is stopping all loaders.
    if (m_parentFrame->loader()->documentLoader()->isStopping())
        return NPERR_GENERIC_ERROR;

    const String& targetFrameName = frameLoadRequest.frameName();
    String jsString = scriptStringIfJavaScriptURL(url);

    if (!jsString.isNull()) {
        // Return NPERR_GENERIC_ERROR if JS is disabled. This is what Mozilla does.
        if (!m_parentFrame->script()->canExecuteScripts(NotAboutToExecuteScript))
            return NPERR_GENERIC_ERROR;

        // For security reasons, only allow JS requests to be made on the frame that contains the plug-in.
        if (!targetFrameName.isNull() && m_parentFrame->tree()->find(targetFrameName) != m_parentFrame)
            return NPERR_INVALID_PARAM;
    } else if (!SecurityOrigin::canLoad(url, String(), m_parentFrame->document()))
            return NPERR_GENERIC_ERROR;

    PluginRequest* request = new PluginRequest(frameLoadRequest, sendNotification, notifyData, arePopupsAllowed());
    scheduleRequest(request);

    return NPERR_NO_ERROR;
}

[Robert Hogan]

Created attachment 51859 [details]
Patch

I did a series of git mv commands but I don't think the message reflects what I actually did, hopefully it's OK.

Also, not sure I'm approaching this the right way - there are quite a lot of tests in Chromium that replace mac-specific tests that should really be in platform/mac. So hopefully the procedure agreed on for this one can be used for the others.

[Eric Seidel (no email)]

Comment on attachment 51859 [details]
Patch

Your ChangeLog format is slightly strange.  At least one extra newline.

Seems we should modify the skipped lists of the various platforms if we're making this change.

[Eric Seidel (no email)]

Looks like andersca wrote the original:
http://trac.webkit.org/browser/trunk/LayoutTests/plugins/get-url-with-blank-target.html

[Robert Hogan]

Created attachment 53051 [details]
Updated Patch

Updated per Eric's comments. Only unskipping on Qt for now.

[Alexey Proskuryakov]

Comment on attachment 53051 [details]
Updated Patch

> Chromium introduced a new version of test that works for Qt and possibly other platforms.

What prevents the original test from working on other platforms?

-    if (result == NPERR_GENERIC_ERROR)
+    if (result == NPERR_NO_ERROR)
         d.innerHTML = "SUCCESS"

> The Mac result expects an NP_ERROR_GENERIC whereas 
> Chromium/Qt expect NP_NO_ERROR, which looking at PluginView.cpp
> is how it should be:

I don't understand the logic here. Why does implementation tell us which behavior is correct?

+        layoutTestController.setCanOpenWindows();
...
+This tests that we won't crash when a plugin tries to open an URL in a new window when the application does not create the window. If this test is successful, the word SUCCESS should be seen below.

The description is correct. This test verifies what happens when the window fails to open, so the test shouldn't call setCanOpenWindows().

[Robert Hogan]

(In reply to comment #5)
> (From update of attachment 53051 [details])
> > Chromium introduced a new version of test that works for Qt and possibly other platforms.
> 
> What prevents the original test from working on other platforms?
> 

See also https://bugs.webkit.org/show_bug.cgi?id=32886, which is a duplicate of this I think - though strictly it's the other way round!

The difference is the customized behaviour of Mac for handling load requests from plugins that have a _blank target. 

For Mac the load happens in (void)loadPluginRequest:(WebPluginRequest *)pluginRequest and if a new webview can't be created for the request, which is how targeted requests are handled in Mac, it does this:

1657	            if (!newWebView) {
1658	                if ([pluginRequest sendNotification]) {
1659	                    [self willCallPlugInFunction];
1660	                    {
1661	                        JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly);
1662	                        [_pluginPackage.get() pluginFuncs]->urlnotify(plugin, [[[pluginRequest request] URL] _web_URLCString], NPERR_GENERIC_ERROR, [pluginRequest notifyData]);
1663	                    }
1664	                    [self didCallPlugInFunction];
1665	                }
1666	                return;
1667	            }

Note the NPERR_GENERIC_ERROR in line 1662 - this is what generates the expected result on Mac. It aborts the targeted request when the client can't create a webview to contain it.

All other ports  use the generic WebCore code in PluginView.cpp, which is:

void PluginView::performRequest(PluginRequest* request)
{
<..>
            m_parentFrame->loader()->load(request->frameLoadRequest().resourceRequest(), targetFrameName, false);

            if (request->sendNotification()) {
                PluginView::setCurrentPluginView(this);
                JSC::JSLock::DropAllLocks dropAllLocks(JSC::SilenceAssertionsOnly);
                setCallingPlugin(true);
                m_plugin->pluginFuncs()->urlnotify(m_instance, requestURL.string().utf8().data(), NPRES_DONE, request->notifyData());
<..>
}

Again note the NPRES_DONE, the request is handled normally rather than made dependent on the ability to create a new webview.

My understanding is that the behaviour of ports other than Mac is correct on its own terms - they have no need to create a new window/webview for targeted requests so will not fail, all other things being equal, and don't end up in the same possible crash situation as Mac.

Also, the Mac behaviour that is the subject of a fixme in WebNetscapePluginView.mm:

1646	        // FIXME - need to get rid of this window creation which
1647	        // bypasses normal targeted link handling



So I think there is strong case for saying the Chromium results should be be made platform-independent and the current platform-independent (i.e. Mac) results moved to platform/mac.

[Alexey Proskuryakov]

> Also, the Mac behaviour that is the subject of a fixme in WebNetscapePluginView.mm:

So it appears that the key to understanding this is to dig up why this code is present on Mac, and why someone thought that it's wrong.

[Robert Hogan]

(In reply to comment #7)
> > Also, the Mac behaviour that is the subject of a fixme in WebNetscapePluginView.mm:
> 
> So it appears that the key to understanding this is to dig up why this code is present on Mac, and why someone thought that it's wrong.

The code with the comment was added in 2003 under rdar://problem/3106525 by cblu.

The situation where a null webview is returned for the delegate method seems to happen only in Mac Mail, not sure why that situation exists (rdar://problem/5025212). This is the situation the test is ensuring does not cause a crash. It doesn't arise for other ports within WebKit code as far as I can tell since there doesn't appear to be a situation in which WebCore will fail to create a page for the targeted request.

[Alexey Proskuryakov]

Presumably every port has client callbacks where a client can reject the request to open a new window for any client-specific reason. A mail application obviously doesn't want new windows to be spawned by e-mails (even if it allows plug-ins in those).

The difference to explain (if I'm following your investigation correctly) is why only Mac seems to special case "_blank".

[Robert Hogan]

Created attachment 74021 [details]
Patch

[Robert Hogan]

(In reply to comment #10)
> Created an attachment (id=74021) [details]
> Patch

A couple of points on this:

- I think the code added to FrameLoader needs to be guarded out for Mac Safari builds - since it implements something similar to this in WebBaseNetscapePluginView.mm. I'd be grateful is someone could confirm this.
- The alternative to the impementation in the patch is to pass PluginView and some other details to frameloader and call back when the load completes. Given that the load to a target frame can destroy the PluginView this seemed a risky option so I avoided it.

[Eric Seidel (no email)]

Attachment 74021 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6070054

[Robert Hogan]

Created attachment 74026 [details]
Patch

[Eric Seidel (no email)]

Attachment 74026 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5984093

[WebKit Review Bot]

Attachment 74021 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6084031

[WebKit Review Bot]

Attachment 74026 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6055097

[Robert Hogan]

(In reply to comment #16)
> Attachment 74026 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/6055097

I'll fix these if the patch is r+'d. The mac build may take a bit of trial and error as the output isn't very informative.

[Eric Seidel (no email)]

It's better to post a new patch and let the EWS bots munch on it again.  Then you don't break the tree when landing. :)

[Robert Hogan]

(In reply to comment #13)
> Created an attachment (id=74026) [details]
> Patch

Per Alexey's comments in IRC a bit more context is required for why the patch is needed.

Currently, if an NPAPI plugin sends a getURLNotify() with a target frame specified in the request PluginView passes it off to FrameLoader and immediately returns an NPRES_DONE notification to the plugin without waiting to hear how the request actually got on.

Mac gets around this by implementing platform-specific logic (it doesn't use PluginView for a start) in WebKit/mac/plugins/.

This WebCore behaviour means that this test fails for users of PluginView - including qt, gtk and chromium. Chromium passes the test by changing its platform-specific expected result to match the fake NPRES_DONE currently returned by PluginView::performRequest.

So fixing this early return in WebCore should allow other ports to start passing this test as long as they support all the DRT bits and pieces required to do a getUrlNotify() layout test and haven't implemented similar workarounds in platform code.

[Eric Seidel (no email)]

Attachment 74026 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6187042

[Robert Hogan]

Could someone take a look?

[Alexey Proskuryakov]

Comment on attachment 74026 [details]
Patch

Sorry for a long wait. This patch is on a right track.

+        Unskip plugins/get-url-with-blank-target.html on Qt.

I'd love to see a more ambitious patch, even if it could mean temporary buildbot breakage. This fix is supposed to affect most platforms, so I'd start with unskipping the test on all, and cleaning up misleading Chromium platform-specific results.

As you could see, the current situation is too confusing.

+        When an NPAPI plugin performs a request on a targe frame with

Typo: target.

+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::FrameLoader):
+        (WebCore::FrameLoader::load):
+        (WebCore::FrameLoader::checkLoadCompleteForThisFrame):
+        (WebCore::FrameLoader::notifyPlugin):
+        (WebCore::FrameLoader::continueLoadAfterNewWindowPolicy):

Per-method comments would have made reviewing easier.

+void FrameLoader::load(const ResourceRequest& request, bool lockHistory, bool notifyPlugin)

We try to not use boolean arguments in cases like this in new code. Seeing a call site like call(request, true, false) tells one nothing about what's being called, and it's super easy to make a mistake - especially when there are tons of overrides.

The common solutions are adding an enum for policy options, or adding a method with a different name.

-        frame->loader()->load(request, lockHistory);
+        frame->loader()->load(request, lockHistory, m_shouldNotifyPlugin);

Won't the current frame have a stale m_shouldNotifyPlugin after passing the request to target frame?

+        if (m_requestsAwaitingNotification[i]->request() == request
+             && m_requestsAwaitingNotification[i]->sendNotification()) {

This line isn't particularly long, I think that the code would be easier to read without wrapping.

More importantly, I don't see why comparing requests works. There can be both false negatives and false positives:
- what if two plug-in instances make a request for the same URL?
- what if a request has been modified by a delegate call, changing it platform object?

And of course, traversing all plug-in views and all their requests isn't very elegant. Perhaps FrameLoader could just keep a PluginRequest pointer?

+void PluginView::sendUrlNotify(PluginRequest* request, int result)

WebKit style is "sendURLNotify".

-            // FIXME: <rdar://problem/4807469> This should be sent when the document has finished loading

You could add <rdar://problem/4807469> to ChangeLog as a link to one of the fixed bugs.

[Robert Hogan]

Created attachment 75628 [details]
Patch

[Eric Seidel (no email)]

Attachment 75628 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6786056

[WebKit Review Bot]

Attachment 75628 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6817061

[Eric Seidel (no email)]

Attachment 75628 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6849051

[Robert Hogan]

(In reply to comment #22)
> (From update of attachment 74026 [details])
> Sorry for a long wait. This patch is on a right track.
> 
> +        Unskip plugins/get-url-with-blank-target.html on Qt.
> 
> I'd love to see a more ambitious patch, even if it could mean temporary buildbot breakage. This fix is supposed to affect most platforms, so I'd start with unskipping the test on all, and cleaning up misleading Chromium platform-specific results.
> 
I've unskipped on gtk for now. This will hopefully allow a soft landing.

> As you could see, the current situation is too confusing.
> 
> +        When an NPAPI plugin performs a request on a targe frame with
> 
> Typo: target.
> 
> +        * loader/FrameLoader.cpp:
> +        (WebCore::FrameLoader::FrameLoader):
> +        (WebCore::FrameLoader::load):
> +        (WebCore::FrameLoader::checkLoadCompleteForThisFrame):
> +        (WebCore::FrameLoader::notifyPlugin):
> +        (WebCore::FrameLoader::continueLoadAfterNewWindowPolicy):
> 
> Per-method comments would have made reviewing easier.
> 

Fixed.

> +void FrameLoader::load(const ResourceRequest& request, bool lockHistory, bool notifyPlugin)
> 
> We try to not use boolean arguments in cases like this in new code. Seeing a call site like call(request, true, false) tells one nothing about what's being called, and it's super easy to make a mistake - especially when there are tons of overrides.
> 
> The common solutions are adding an enum for policy options, or adding a method with a different name.
> 
> -        frame->loader()->load(request, lockHistory);
> +        frame->loader()->load(request, lockHistory, m_shouldNotifyPlugin);
> 
> Won't the current frame have a stale m_shouldNotifyPlugin after passing the request to target frame?
> 
> +        if (m_requestsAwaitingNotification[i]->request() == request
> +             && m_requestsAwaitingNotification[i]->sendNotification()) {
> 
> This line isn't particularly long, I think that the code would be easier to read without wrapping.
> 

All fixed.

> More importantly, I don't see why comparing requests works. There can be both false negatives and false positives:
> - what if two plug-in instances make a request for the same URL?
> - what if a request has been modified by a delegate call, changing it platform object?
> 
> And of course, traversing all plug-in views and all their requests isn't very elegant. Perhaps FrameLoader could just keep a PluginRequest pointer?
> 

Fixed this by passing PluginView instead. 

> +void PluginView::sendUrlNotify(PluginRequest* request, int result)
> 
> WebKit style is "sendURLNotify".
> 
> -            // FIXME: <rdar://problem/4807469> This should be sent when the document has finished loading
> 
> You could add <rdar://problem/4807469> to ChangeLog as a link to one of the fixed bugs.

Fixed.

[Alexey Proskuryakov]

Comment on attachment 75628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75628&action=review

> LayoutTests/ChangeLog:14
> +        * platform/qt/Skipped: unskip plugins/get-url-with-blank-target.html
> +        * platform/gtk/Skipped: ditto

What about platform/win?

> WebCore/loader/FrameLoader.cpp:207
>      , m_sandboxFlags(SandboxAll)
>      , m_forcedSandboxFlags(SandboxNone)
>  {
> +    m_PluginToNotify = 0;

This should be initialized, not assigned.

Also, wrong style, should be m_pluginToNotify.

> WebCore/loader/FrameLoader.cpp:1403
>      Frame* frame = findFrameForNavigation(frameName);
>      if (frame) {
> -        frame->loader()->load(request, lockHistory);
> +        frame->loader()->load(request, lockHistory, m_PluginToNotify);
> +        m_PluginToNotify = 0;
>          return;

What guarantees that frame->loader() != this?

> WebCore/loader/FrameLoader.h:494
> +    PluginView* m_PluginToNotify;

Can the view be destroyed before FrameLoader? Will we end up with a stale pointer then?

I imagine that this may not be too bad, as the request will be stopped when destroying plug-in, too. But to program defensively, we should not keep a stale pointer, even if we're not going to use it.

> WebCore/plugins/PluginView.cpp:426
> +        if (m_requestsAwaitingNotification[i]->request() == request && m_requestsAwaitingNotification[i]->sendNotification()) {

I still don't believe that comparing requests can ever be good. Am I missing something?

> WebCore/plugins/PluginView.h:117
> +        ResourceRequest m_resourceRequest;

If we actually don't need to compare requests, then this will go away, too.

[Robert Hogan]

Created attachment 75840 [details]
Patch

[Robert Hogan]

(In reply to comment #28)
> (From update of attachment 75628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75628&action=review
> 
> > LayoutTests/ChangeLog:14
> > +        * platform/qt/Skipped: unskip plugins/get-url-with-blank-target.html
> > +        * platform/gtk/Skipped: ditto
> 
> What about platform/win?
> 
> > WebCore/loader/FrameLoader.cpp:207
> >      , m_sandboxFlags(SandboxAll)
> >      , m_forcedSandboxFlags(SandboxNone)
> >  {
> > +    m_PluginToNotify = 0;
> 
> This should be initialized, not assigned.
> 
> Also, wrong style, should be m_pluginToNotify.
> 
> > WebCore/loader/FrameLoader.cpp:1403
> >      Frame* frame = findFrameForNavigation(frameName);
> >      if (frame) {
> > -        frame->loader()->load(request, lockHistory);
> > +        frame->loader()->load(request, lockHistory, m_PluginToNotify);
> > +        m_PluginToNotify = 0;
> >          return;
> 
> What guarantees that frame->loader() != this?
> 
> > WebCore/loader/FrameLoader.h:494
> > +    PluginView* m_PluginToNotify;
> 
> Can the view be destroyed before FrameLoader? Will we end up with a stale pointer then?
> 
> I imagine that this may not be too bad, as the request will be stopped when destroying plug-in, too. But to program defensively, we should not keep a stale pointer, even if we're not going to use it.
> 

All fixed!

> > WebCore/plugins/PluginView.cpp:426
> > +        if (m_requestsAwaitingNotification[i]->request() == request && m_requestsAwaitingNotification[i]->sendNotification()) {
> 
> I still don't believe that comparing requests can ever be good. Am I missing something?

No, I was just reluctant to pass the PluginRequest as well as the PluginView but it now seems much cleaner that way.

> 
> > WebCore/plugins/PluginView.h:117
> > +        ResourceRequest m_resourceRequest;
> 
> If we actually don't need to compare requests, then this will go away, too.

It can't go away entirely, but I now just keep the url instead. PluginRequest::frameLoadRequest() seems to get destroyed before the request completes in some situations, causing crashes in the layout tests, so I can't reference it when sending the notification from PluginView.

Thanks for the detailed reviews!

[Eric Seidel (no email)]

Attachment 75840 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6735095

[WebKit Review Bot]

Attachment 75840 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 75840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75840&action=review

I haven't had the time to review the patch in detail yet. I'm feeling uneasy about the PluginRequest pointer that might also become stale, and about adding so many data members to various classes, meaning that information gets duplicated. Duplicated information is the source of all evil, as the copies tend to get out of sync.

> WebCore/loader/FrameLoader.cpp:1406
> +        frame->loader()->load(request, lockHistory, m_pluginToNotify, m_pluginRequest);
> +        if (frame->loader() != this)
> +            clearPluginReferences();

What guarantees that the frame pointer isn't stale after calling load()? I think that a cleaner approach would be to not store the references to member variables at all in this code path (meaning that load() wouldn't get another override).

> WebCore/loader/FrameLoader.h:340
> +    void clearPluginReferences();

Commonly, we'd call this pluginViewDestroyed(), and pass a plugin view pointer. This also makes me wonder about what exactly happens if two plug-in instances try to load in a frame (surely one request will be stopped, but does it work right?)

[Robert Hogan]

(In reply to comment #33)
> (From update of attachment 75840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75840&action=review
> 
> I haven't had the time to review the patch in detail yet. I'm feeling uneasy about the PluginRequest pointer that might also become stale, and about adding so many data members to various classes, meaning that information gets duplicated. Duplicated information is the source of all evil, as the copies tend to get out of sync.
> 
> > WebCore/loader/FrameLoader.cpp:1406
> > +        frame->loader()->load(request, lockHistory, m_pluginToNotify, m_pluginRequest);
> > +        if (frame->loader() != this)
> > +            clearPluginReferences();
> 
> What guarantees that the frame pointer isn't stale after calling load()? 

I see what you mean.

> I think that a cleaner approach would be to not store the references to member variables at all in this code path (meaning that load() wouldn't get another override).

Do you mean:

if (frame->loader() != this) {
  frame->loader()->load(request, lockHistory, m_pluginToNotify, m_pluginRequest);
  clearPluginReferences();
} else
  frame->loader()->load(request, lockHistory);

Or do you mean if a target frame is specified, don't wait for the request result to notify the plugin?


> 
> > WebCore/loader/FrameLoader.h:340
> > +    void clearPluginReferences();
> 
> Commonly, we'd call this pluginViewDestroyed(), and pass a plugin view pointer. This also makes me wonder about what exactly happens if two plug-in instances try to load in a frame (surely one request will be stopped, but does it work right?)

Would something like this remove the uncertainty?

void FrameLoader::load(const ResourceRequest& request, bool lockHistory, PluginView* plugin, PluginRequest* pluginRequest)
{
    if (m_pluginToNotify){
        notifyPlugin(ResourceError(errorDomainWebKitInternal, 0, request.url().string(), ""));
        clearPluginReferences();
    }
    m_pluginToNotify = plugin;
    m_pluginRequest = pluginRequest;
    load(request, lockHistory);
}

[Alexey Proskuryakov]

> Do you mean:

I actually meant what you posted below, void FrameLoader::load(const ResourceRequest& request, bool lockHistory, PluginView* plugin, PluginRequest* pluginRequest). This function never stores the variables in a wrong frame, so it doesn't have to clean up them.

[Eric Seidel (no email)]

Comment on attachment 75840 [details]
Patch

This breaks FrameLoader compile on Mac.

[Robert Hogan]

Created attachment 78308 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 78308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78308&action=review

Thanks for getting rid of searching by URL! I think that this is getting pretty close.

> Source/WebCore/loader/FrameLoader.cpp:1355
> +    if (m_pluginToNotify)
> +        notifyPlugin(ResourceError(errorDomainWebKitInternal, 0, request.url().string(), ""));

Is request.url() the correct URL to report here? Also, this looks like it should use FrameLoaderClient::cancelledError().

Is this code path tested?

> Source/WebCore/loader/FrameLoader.cpp:1395
>      Frame* frame = findFrameForNavigation(frameName);
>      if (frame) {
> -        frame->loader()->load(request, lockHistory);
> +        frame->loader()->load(request, lockHistory, m_pluginToNotify, m_pluginRequest);

What clears these member variables in the current frame?

> Source/WebCore/loader/FrameLoader.cpp:2439
> +    if (!m_pluginToNotify)
> +        return;

I'd assert that m_pluginRequest is null here.

> Source/WebCore/loader/FrameLoader.cpp:3036
> +        notifyPlugin(ResourceError(errorDomainWebKitInternal, 0, request.url().string(), ""));

Maybe FrameLoaderClient::interruptForPolicyChangeError()?

[Alexey Proskuryakov]

it would be great to have this finished, the latest patch is almost there.

[Robert Hogan]

Created attachment 91801 [details]
Patch

[Robert Hogan]

Wow, let this one slide a bit! :-)

(In reply to comment #38)
> Is this code path tested?
> 
It's not and I've failed to come up with a way of loading a url with a notification, then loading another and catching the cancellation. This blocked me resubmitting the last few times I got round to revising the patch.

Any thoughts on how to do this appreciated.

[Alexey Proskuryakov]

Comment on attachment 91801 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91801&action=review

Now that I'm looking at this with a fresh eye, I don't see how this can be correct when multiple plug-ins are making multiple requests at the same time. There is only one pair of variables in FrameLoader!

> LayoutTests/ChangeLog:9
> +        When an NPAPI plugin performs a request on a target frame with
> +        getUrlNotify, wait for the actual request result and pass it to
> +        the plugin.

This is LayoutTest/ChangeLog, so the explanation should mention what has changed here, not in WebCore.

> LayoutTests/ChangeLog:12
> +        https://bugs.webkit.org/show_bug.cgi?id=36721
> +        <rdar://problem/4807469>

This should be higher in ChangeLog, right below the bug description. The reason is that one wants to have a bug URL to click in CIA bot announcement on #webkit.

> Source/WebCore/ChangeLog:12
> +        https://bugs.webkit.org/show_bug.cgi?id=36721
> +        <rdar://problem/4807469>

Same comment about positioning.

> Source/WebCore/loader/FrameLoader.cpp:1412
> +
> +

Extra blank line here.

> Source/WebCore/loader/FrameLoader.cpp:2516
> +    pluginViewDestroyed();

But it is not destroyed!

> Source/WebCore/loader/FrameLoader.cpp:2519
> +
> +

Another extra blank line. We just use one.

> Source/WebCore/loader/FrameLoader.cpp:3628
> +
> +

Again, too many blank lines.

> Source/WebCore/plugins/PluginView.h:110
> -            , m_shouldAllowPopups(shouldAllowPopups) { }
> +            , m_shouldAllowPopups(shouldAllowPopups)
> +            , m_url(frameLoadRequest.resourceRequest().url()) { }
>      public:

This has a pre-existing style error. Braces should go on separate lines when there is a multi-line initializer list, and there should be a blank line before "public:".

> Source/WebCore/plugins/PluginView.h:121
> +        KURL m_url;

I forget what the difference between m_url and m_frameLoadRequest.resourceRequest().url() is. Did we discuss that already? If so, please add a comment explaining the difference (or better yet, give the data member a name that makes the difference clear). Otherwise, let's investigate if a separate variable is needed.

Does either URL get updated on redirect?

[Robert Hogan]

Created attachment 92739 [details]
Patch

[Robert Hogan]

Slight change of approach. :-)

The test coverage is much better now. I think the Changelog is quite clear about the details of the new way of solving this.

[Adam Roben (:aroben)]

Note that returning the result for targetted javascript: requests can lead to bugs like bug 60568.

[Robert Hogan]

(In reply to comment #45)
> Note that returning the result for targetted javascript: requests can lead to bugs like bug 60568.

This patch is compatible with 60568 and passes the test it added.

This is because targetted url requests are protected with this in PluginView:


    KURL requestURL = request->frameLoadRequest().resourceRequest().url();
    String jsString = scriptStringIfJavaScriptURL(requestURL);

    if (jsString.isNull()) {

[Robert Hogan]

This doesn't compile with WK2 because WK2 also has a PluginView.h. I am adding two classes to WK1's PluginView.h, and adding it as an include to FrameLoader.h - the absence of the two new classes from WK2's PluginView.h causes the WK2 build to fail when it attempts to compile FrameLoader.h.

What's the right thing to do? Just add stubs to WK2?

[Robert Hogan]

Note that WK2 handles url notifications using a load listener so doesn't share this bug with WK1.

[Alexey Proskuryakov]

Why you need to include PluginView.h in FrameLoader.h?

[Robert Hogan]

(In reply to comment #49)
> Why you need to include PluginView.h in FrameLoader.h?

Because FrameLoader now references a number of classes there: PluginView, PluginRequest and PluginNotification (a new one).

[Alexey Proskuryakov]

You also add forward declarations for these classes in FrameLoader.h. Why isn't that sufficient?

[Robert Hogan]

(In reply to comment #51)
> You also add forward declarations for these classes in FrameLoader.h. Why isn't that sufficient?

I think because I'm using a RefPtr.

[Darin Adler]

(In reply to comment #52)
> (In reply to comment #51)
> > You also add forward declarations for these classes in FrameLoader.h. Why isn't that sufficient?
> 
> I think because I'm using a RefPtr.

It’s only because the functions manipulating the RefPtr are inlines. You should change that around so you only need forward declarations.

[Robert Hogan]

Created attachment 94489 [details]
Patch

[Robert Hogan]

(In reply to comment #53)
> (In reply to comment #52)
> > (In reply to comment #51)
> > > You also add forward declarations for these classes in FrameLoader.h. Why isn't that sufficient?
> > 
> > I think because I'm using a RefPtr.
> 
> It’s only because the functions manipulating the RefPtr are inlines. You should change that around so you only need forward declarations.

Ahhhh! Thanks guys!

[WebKit Review Bot]

Comment on attachment 94489 [details]
Patch

Attachment 94489 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8729324

[Gyuyoung Kim]

Comment on attachment 94489 [details]
Patch

Attachment 94489 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8658036

[WebKit Review Bot]

Comment on attachment 94489 [details]
Patch

Attachment 94489 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8733014

New failing tests:
plugins/get-url-multiple.html
http/tests/plugins/geturlnotify-cancel-load.html

[WebKit Review Bot]

Created attachment 94521 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Commit Bot]

Comment on attachment 94489 [details]
Patch

Attachment 94489 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8727483

[Robert Hogan]

(In reply to comment #56)
> (From update of attachment 94489 [details])
> Attachment 94489 [details] did not pass cr-mac-ews (chromium):
> Output: http://queues.webkit.org/results/8729324

This appears unrelated to my patch:

../src/WebSearchableFormData.cpp:55: error: reference to 'TextEncoding' is ambiguous
../Headers/TextCommon.h:567: error: candidates are: typedef UInt32 TextEncoding

[Robert Hogan]

(In reply to comment #58)
> (From update of attachment 94489 [details])
> Attachment 94489 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/8733014
> 
> New failing tests:
> plugins/get-url-multiple.html
> http/tests/plugins/geturlnotify-cancel-load.html

These are new tests so I think they may reveal bugs in chromium.

[Robert Hogan]

Created attachment 94656 [details]
Patch

[WebKit Review Bot]

Comment on attachment 94656 [details]
Patch

Attachment 94656 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8734163

New failing tests:
plugins/get-url-multiple.html
http/tests/plugins/geturlnotify-cancel-load.html

[WebKit Review Bot]

Created attachment 94669 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Commit Bot]

Comment on attachment 94656 [details]
Patch

Attachment 94656 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8733284

[Robert Hogan]

Created attachment 94820 [details]
Patch

[WebKit Review Bot]

Comment on attachment 94820 [details]
Patch

Attachment 94820 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8730918

New failing tests:
http/tests/plugins/geturlnotify-multiple-simultaneous-requests-to-blank-target.html

[WebKit Review Bot]

Created attachment 94839 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Commit Bot]

Comment on attachment 94820 [details]
Patch

Attachment 94820 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8734515

[Alexey Proskuryakov]

Comment on attachment 94820 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=94820&action=review

I still don't understand how a single PluginNotification object per document can handle multiple simultaneous requests. I can see that you now have a HashSet of pending notifications, but I don't see it ever being consulted (other than to remove items from it).

> LayoutTests/ChangeLog:12
> +        When an NPAPI plugin performs a request on a target frame with
> +        getUrlNotify, wait for the actual request result and pass it to
> +        the plugin.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=36721
> +        <rdar://problem/4807469>

Please put bug URLs first (right after the title, without blank lines in between), and detailed explanation later.

> Source/WebCore/ChangeLog:17
> +        https://bugs.webkit.org/show_bug.cgi?id=36721
> +        <rdar://problem/4807469>

Ditto - please put the URLs right after bug title.

> Source/WebCore/loader/DocumentLoader.h:249
> +        void setPluginNotification(PassRefPtr<PluginNotification>);
> +        PassRefPtr<PluginNotification> pluginNotification();

These should not use PassRefPtr. Ownership is not being passed here, so you should use a plain pointer.

> Source/WebCore/loader/FrameLoader.cpp:209
> +    , m_pluginNotification(0)

There is no need to initialize a RefPtr with null, please remove this line.

> Source/WebCore/loader/FrameLoader.cpp:1021
> +    childFrame->loader()->setPluginNotification(m_pluginNotification);
> +    m_pluginNotification = 0;

Please use clear() (here and elsewhere in the patch).

[Robert Hogan]

(In reply to comment #71)
> (From update of attachment 94820 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94820&action=review
> 
> I still don't understand how a single PluginNotification object per document can handle multiple simultaneous requests. 

This works because attempting a request to the same target as a request in progress will cancel the existing one. This is tested by geturlnotify-cancel-load-main.htm. If the request is to a different target it will be used by a different document object.

>I can see that you now have a HashSet of pending notifications, but I don't see it ever being consulted (other than to remove items from it).

Looking at it again I think the use of RefPtr has made this HashSet redundant.

[Robert Hogan]

Created attachment 97697 [details]
Patch

[Robert Hogan]

(In reply to comment #71)
> > Source/WebCore/loader/DocumentLoader.h:249
> > +        void setPluginNotification(PassRefPtr<PluginNotification>);
> > +        PassRefPtr<PluginNotification> pluginNotification();
> 
> These should not use PassRefPtr. Ownership is not being passed here, so you should use a plain pointer.

I want to pass ownership here, and that's why I have :

RefPtr<PluginNotification> m_pluginNotification;

Frameloader's reference to the class is cleared whenever this is called. I went back to http://www.webkit.org/coding/RefPtr.html and looked at similar uses elsewhere in WebCore and AFAICT I am passing ownership.

I've updated everything else per your comments.

[Adam Barth]

Comment on attachment 97697 [details]
Patch

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

[Adam Barth]

I would like to look at this patch before it lands to make sure that it doesn't regress the work we're doing to improve FrameLoader.

[Robert Hogan]

(In reply to comment #76)
> I would like to look at this patch before it lands to make sure that it doesn't regress the work we're doing to improve FrameLoader.

Thanks for clearing up the use of RefPtr for me.

I'll take a look at using FrameLoaderClient instead.

[Ahmad Saleem]

I don't think Safari supports NPAPI plugins but I don't know about Webkit. Is it something should be closed, if NPAPI support has been dropped already? If I am mistaken about context of bug, please ignore. Thanks!

[Alexey Proskuryakov]

Yes, I believe it's fully gone from WebKit.