Bug 36721 - Return correct load result to NPAPI plugins calling getUrlNotify on target frames
Summary: Return correct load result to NPAPI plugins calling getUrlNotify on target fr...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-28 06:08 PDT by Robert Hogan
Modified: 2022-06-01 08:26 PDT (History)
13 users (show)

See Also:


Attachments
Patch (9.16 KB, patch)
2010-03-28 06:24 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Updated Patch (9.99 KB, patch)
2010-04-10 13:03 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.52 KB, patch)
2010-11-16 12:19 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.54 KB, patch)
2010-11-16 12:54 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.71 KB, patch)
2010-12-05 07:16 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (14.06 KB, patch)
2010-12-07 13:55 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2011-01-08 02:55 PST, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (14.30 KB, patch)
2011-04-30 04:21 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (38.57 KB, patch)
2011-05-08 11:57 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (38.81 KB, patch)
2011-05-23 14:28 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.24 MB, application/zip)
2011-05-23 17:06 PDT, WebKit Review Bot
no flags Details
Patch (39.35 KB, patch)
2011-05-24 12:01 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.29 MB, application/zip)
2011-05-24 12:40 PDT, WebKit Review Bot
no flags Details
Patch (40.06 KB, patch)
2011-05-25 11:44 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.26 MB, application/zip)
2011-05-25 12:40 PDT, WebKit Review Bot
no flags Details
Patch (39.89 KB, patch)
2011-06-18 05:46 PDT, Robert Hogan
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-03-28 06:08:34 PDT
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;
}
Comment 1 Robert Hogan 2010-03-28 06:24:58 PDT
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.
Comment 2 Eric Seidel (no email) 2010-04-01 17:34:07 PDT
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.
Comment 3 Eric Seidel (no email) 2010-04-01 17:35:01 PDT
Looks like andersca wrote the original:
http://trac.webkit.org/browser/trunk/LayoutTests/plugins/get-url-with-blank-target.html
Comment 4 Robert Hogan 2010-04-10 13:03:42 PDT
Created attachment 53051 [details]
Updated Patch

Updated per Eric's comments. Only unskipping on Qt for now.
Comment 5 Alexey Proskuryakov 2010-05-13 09:32:16 PDT
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().
Comment 6 Robert Hogan 2010-06-13 11:17:04 PDT
(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.
Comment 7 Alexey Proskuryakov 2010-06-14 10:20:54 PDT
> 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.
Comment 8 Robert Hogan 2010-06-14 11:08:46 PDT
(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.
Comment 9 Alexey Proskuryakov 2010-06-14 11:22:50 PDT
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".
Comment 10 Robert Hogan 2010-11-16 12:19:44 PST
Created attachment 74021 [details]
Patch
Comment 11 Robert Hogan 2010-11-16 12:31:48 PST
(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.
Comment 12 Eric Seidel (no email) 2010-11-16 12:47:22 PST
Attachment 74021 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6070054
Comment 13 Robert Hogan 2010-11-16 12:54:21 PST
Created attachment 74026 [details]
Patch
Comment 14 Eric Seidel (no email) 2010-11-16 13:04:31 PST
Attachment 74026 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5984093
Comment 15 WebKit Review Bot 2010-11-16 13:11:29 PST
Attachment 74021 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6084031
Comment 16 WebKit Review Bot 2010-11-16 13:43:43 PST
Attachment 74026 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6055097
Comment 17 Robert Hogan 2010-11-16 14:31:47 PST
(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.
Comment 18 Eric Seidel (no email) 2010-11-16 14:52:01 PST
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. :)
Comment 19 Robert Hogan 2010-11-16 14:55:13 PST
(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.
Comment 20 Eric Seidel (no email) 2010-11-18 04:46:15 PST
Attachment 74026 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6187042
Comment 21 Robert Hogan 2010-11-29 13:56:03 PST
Could someone take a look?
Comment 22 Alexey Proskuryakov 2010-11-30 15:56:30 PST
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.
Comment 23 Robert Hogan 2010-12-05 07:16:42 PST
Created attachment 75628 [details]
Patch
Comment 24 Eric Seidel (no email) 2010-12-05 07:38:19 PST
Attachment 75628 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6786056
Comment 25 WebKit Review Bot 2010-12-05 07:41:02 PST
Attachment 75628 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6817061
Comment 26 Eric Seidel (no email) 2010-12-05 08:16:46 PST
Attachment 75628 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6849051
Comment 27 Robert Hogan 2010-12-06 11:58:20 PST
(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.
Comment 28 Alexey Proskuryakov 2010-12-06 21:04:15 PST
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.
Comment 29 Robert Hogan 2010-12-07 13:55:25 PST
Created attachment 75840 [details]
Patch
Comment 30 Robert Hogan 2010-12-07 14:05:49 PST
(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!
Comment 31 Eric Seidel (no email) 2010-12-07 20:27:53 PST
Attachment 75840 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6735095
Comment 32 WebKit Review Bot 2010-12-07 21:52:53 PST
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.
Comment 33 Alexey Proskuryakov 2010-12-08 12:18:20 PST
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?)
Comment 34 Robert Hogan 2010-12-08 13:09:47 PST
(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);
}
Comment 35 Alexey Proskuryakov 2010-12-08 13:26:04 PST
> 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.
Comment 36 Eric Seidel (no email) 2010-12-12 02:16:23 PST
Comment on attachment 75840 [details]
Patch

This breaks FrameLoader compile on Mac.
Comment 37 Robert Hogan 2011-01-08 02:55:59 PST
Created attachment 78308 [details]
Patch
Comment 38 Alexey Proskuryakov 2011-01-12 09:10:08 PST
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()?
Comment 39 Alexey Proskuryakov 2011-01-31 22:05:03 PST
it would be great to have this finished, the latest patch is almost there.
Comment 40 Robert Hogan 2011-04-30 04:21:15 PDT
Created attachment 91801 [details]
Patch
Comment 41 Robert Hogan 2011-04-30 04:24:10 PDT
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.
Comment 42 Alexey Proskuryakov 2011-05-02 12:11:25 PDT
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?
Comment 43 Robert Hogan 2011-05-08 11:57:58 PDT
Created attachment 92739 [details]
Patch
Comment 44 Robert Hogan 2011-05-08 12:00:58 PDT
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.
Comment 45 Adam Roben (:aroben) 2011-05-11 07:36:43 PDT
Note that returning the result for targetted javascript: requests can lead to bugs like bug 60568.
Comment 46 Robert Hogan 2011-05-20 03:04:28 PDT
(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()) {
Comment 47 Robert Hogan 2011-05-23 11:55:23 PDT
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?
Comment 48 Robert Hogan 2011-05-23 11:56:04 PDT
Note that WK2 handles url notifications using a load listener so doesn't share this bug with WK1.
Comment 49 Alexey Proskuryakov 2011-05-23 12:23:50 PDT
Why you need to include PluginView.h in FrameLoader.h?
Comment 50 Robert Hogan 2011-05-23 12:53:36 PDT
(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).
Comment 51 Alexey Proskuryakov 2011-05-23 13:04:12 PDT
You also add forward declarations for these classes in FrameLoader.h. Why isn't that sufficient?
Comment 52 Robert Hogan 2011-05-23 13:16:47 PDT
(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.
Comment 53 Darin Adler 2011-05-23 13:18:37 PDT
(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.
Comment 54 Robert Hogan 2011-05-23 14:28:23 PDT
Created attachment 94489 [details]
Patch
Comment 55 Robert Hogan 2011-05-23 14:28:57 PDT
(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!
Comment 56 WebKit Review Bot 2011-05-23 15:35:17 PDT
Comment on attachment 94489 [details]
Patch

Attachment 94489 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8729324
Comment 57 Gyuyoung Kim 2011-05-23 16:17:01 PDT
Comment on attachment 94489 [details]
Patch

Attachment 94489 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8658036
Comment 58 WebKit Review Bot 2011-05-23 17:05:58 PDT
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
Comment 59 WebKit Review Bot 2011-05-23 17:06:04 PDT
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
Comment 60 WebKit Commit Bot 2011-05-23 21:30:20 PDT
Comment on attachment 94489 [details]
Patch

Attachment 94489 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8727483
Comment 61 Robert Hogan 2011-05-24 11:02:46 PDT
(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
Comment 62 Robert Hogan 2011-05-24 11:58:52 PDT
(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.
Comment 63 Robert Hogan 2011-05-24 12:01:39 PDT
Created attachment 94656 [details]
Patch
Comment 64 WebKit Review Bot 2011-05-24 12:40:05 PDT
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
Comment 65 WebKit Review Bot 2011-05-24 12:40:11 PDT
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
Comment 66 WebKit Commit Bot 2011-05-24 13:42:34 PDT
Comment on attachment 94656 [details]
Patch

Attachment 94656 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8733284
Comment 67 Robert Hogan 2011-05-25 11:44:37 PDT
Created attachment 94820 [details]
Patch
Comment 68 WebKit Review Bot 2011-05-25 12:39:53 PDT
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
Comment 69 WebKit Review Bot 2011-05-25 12:40:00 PDT
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
Comment 70 WebKit Commit Bot 2011-05-25 12:52:26 PDT
Comment on attachment 94820 [details]
Patch

Attachment 94820 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8734515
Comment 71 Alexey Proskuryakov 2011-06-14 14:25:50 PDT
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).
Comment 72 Robert Hogan 2011-06-15 10:48:08 PDT
(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.
Comment 73 Robert Hogan 2011-06-18 05:46:38 PDT
Created attachment 97697 [details]
Patch
Comment 74 Robert Hogan 2011-06-18 05:55:22 PDT
(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.
Comment 75 Adam Barth 2011-06-18 06:18:49 PDT
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
Comment 76 Adam Barth 2011-06-18 06:20:21 PDT
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.
Comment 77 Robert Hogan 2011-06-18 06:46:22 PDT
(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.
Comment 78 Ahmad Saleem 2022-06-01 03:10:47 PDT
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!
Comment 79 Alexey Proskuryakov 2022-06-01 08:26:56 PDT
Yes, I believe it's fully gone from WebKit.