| Summary: | Return correct load result to NPAPI plugins calling getUrlNotify on target frames | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Robert Hogan <robert> | ||||||||||||||||||||||||||||||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||
| Status: | RESOLVED INVALID | ||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, achristensen, ahmad.saleem792, andersca, ap, aroben, darin, dglazkov, dpranke, eric, p.jacquemart, robert, webkit.review.bot | ||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
| Hardware: | Other | ||||||||||||||||||||||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||
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 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.
Looks like andersca wrote the original: http://trac.webkit.org/browser/trunk/LayoutTests/plugins/get-url-with-blank-target.html Created attachment 53051 [details]
Updated Patch
Updated per Eric's comments. Only unskipping on Qt for now.
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(). (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. > 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.
(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. 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". Created attachment 74021 [details]
Patch
(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. Attachment 74021 [details] did not build on mac: Build output: http://queues.webkit.org/results/6070054 Created attachment 74026 [details]
Patch
Attachment 74026 [details] did not build on mac: Build output: http://queues.webkit.org/results/5984093 Attachment 74021 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6084031 Attachment 74026 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6055097 (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. 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. :) (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. Attachment 74026 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6187042 Could someone take a look? 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. Created attachment 75628 [details]
Patch
Attachment 75628 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6786056 Attachment 75628 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6817061 Attachment 75628 [details] did not build on mac: Build output: http://queues.webkit.org/results/6849051 (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 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. Created attachment 75840 [details]
Patch
(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! Attachment 75840 [details] did not build on mac: Build output: http://queues.webkit.org/results/6735095 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 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?) (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); } > 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 on attachment 75840 [details]
Patch
This breaks FrameLoader compile on Mac.
Created attachment 78308 [details]
Patch
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()? it would be great to have this finished, the latest patch is almost there. Created attachment 91801 [details]
Patch
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 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? Created attachment 92739 [details]
Patch
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. Note that returning the result for targetted javascript: requests can lead to bugs like bug 60568. (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()) { 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? Note that WK2 handles url notifications using a load listener so doesn't share this bug with WK1. Why you need to include PluginView.h in FrameLoader.h? (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). You also add forward declarations for these classes in FrameLoader.h. Why isn't that sufficient? (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. (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. Created attachment 94489 [details]
Patch
(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 on attachment 94489 [details] Patch Attachment 94489 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8729324 Comment on attachment 94489 [details] Patch Attachment 94489 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8658036 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 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 on attachment 94489 [details] Patch Attachment 94489 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8727483 (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 (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. Created attachment 94656 [details]
Patch
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 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 on attachment 94656 [details] Patch Attachment 94656 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8733284 Created attachment 94820 [details]
Patch
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 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 on attachment 94820 [details] Patch Attachment 94820 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8734515 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). (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. Created attachment 97697 [details]
Patch
(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 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 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. (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. 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! Yes, I believe it's fully gone from WebKit. |
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; }