WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() can be called with a source document that has no frame.
<rdar://problem/33217736>
Created attachment 315158 [details] Patch and layout test
Created attachment 315262 [details] Patch and layout test
Comment on attachment 315262 [details] Patch and layout test Attachment 315262 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4108713 New failing tests: http/tests/loading/window-open-redirect-and-remove-opener.html
Created attachment 315269 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 315271 [details] Patch and layout test Add missing file LayoutTests/http/tests/loading/resources/window-open-redirect-and-remove-opener.html, which the test LayoutTests/http/tests/loading/window-open-redirect-and-remove-opener.html depends on.
Comment on attachment 315271 [details] Patch and layout test Attachment 315271 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4109223 New failing tests: http/tests/loading/window-open-redirect-and-remove-opener.html
Created attachment 315280 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
(In reply to Build Bot from comment #7) > Comment on attachment 315271 [details] > Patch and layout test > > Attachment 315271 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/4109223 > > New failing tests: > http/tests/loading/window-open-redirect-and-remove-opener.html For some reason there is a difference in delegate callbacks on iOS compared to Mac (why?): --- /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-results/http/tests/loading/window-open-redirect-and-remove-opener-expected.txt +++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/layout-test-results/http/tests/loading/window-open-redirect-and-remove-opener-actual.txt @@ -12,8 +12,8 @@ frame "A" - didFinishDocumentLoadForFrame frame "A" - didFailLoadWithError main frame - didFinishLoadForFrame +main frame - didHandleOnloadEventsForFrame frame "B" - didStartProvisionalLoadForFrame -main frame - didHandleOnloadEventsForFrame frame "B" - didReceiveServerRedirectForProvisionalLoadForFrame frame "B" - didCancelClientRedirectForFrame frame "B" - didCommitLoadForFrame
Created attachment 315292 [details] Patch and layout test The different order of delegate callbacks is due to the asynchronous nature of frame loading. While we could fix up the test to enforce loads sequentially, the purpose of this test is to ensure that we do not crash in WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() when the frame that initiated that navigation no longer exists. That is, the purpose of this test is not to ensure an ordering of delegate callbacks. So, I moved the test to LayoutTests/http/tests/navigation to avoid dumping the frame loader callbacks.
Created attachment 315295 [details] Patch and layout test
Comment on attachment 315295 [details] Patch and layout test Attachment 315295 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4110035 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open.html
Created attachment 315312 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315295 [details] Patch and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=315295&action=review > Source/WebKit2/UIProcess/WebProcessProxy.cpp:210 > + if (!WebPageProxyMap::isValidKey(pageID)) > + return nullptr; This worries me. Can you explain a bit more about why this is necessary? Can we maybe do the checking for 0 before calling this since it seems like quite a special case? (Eventually, it would be nice if we had something like std::optional<Page::Id> as the type being passed).
(In reply to Sam Weinig from comment #14) > Comment on attachment 315295 [details] > Patch and layout test > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315295&action=review > > > Source/WebKit2/UIProcess/WebProcessProxy.cpp:210 > > + if (!WebPageProxyMap::isValidKey(pageID)) > > + return nullptr; > > This worries me. Can you explain a bit more about why this is necessary? WebProcess will send page ID 0 in the decidePolicyForNavigationAction message if the source frame's page no longer exists. Additional remarks: Note, as the proposed patch is written we will send page ID 0 if the source frame does not exist regardless of whether the source frame's page still exists. The layout test included in the patch is an example of this case - the source frame's page still exists even though the source frame was removed. An example of when both the source frame and source frame's page do not exist is when their containing window is closed. I will amend the patch to add a FIXME to track whether the source frame's page actually exists and follow up in a separate bug. > Can we maybe do the checking for 0 before calling this since it seems like > quite a special case? I can do this.
Comment on attachment 315295 [details] Patch and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=315295&action=review Besides that existing feedback this seems fine. >>> Source/WebKit2/UIProcess/WebProcessProxy.cpp:210 >>> + return nullptr; >> >> This worries me. Can you explain a bit more about why this is necessary? Can we maybe do the checking for 0 before calling this since it seems like quite a special case? (Eventually, it would be nice if we had something like std::optional<Page::Id> as the type being passed). > > WebProcess will send page ID 0 in the decidePolicyForNavigationAction message if the source frame's page no longer exists. > > Additional remarks: > > Note, as the proposed patch is written we will send page ID 0 if the source frame does not exist regardless of whether the source frame's page still exists. The layout test included in the patch is an example of this case - the source frame's page still exists even though the source frame was removed. An example of when both the source frame and source frame's page do not exist is when their containing window is closed. I will amend the patch to add a FIXME to track whether the source frame's page actually exists and follow up in a separate bug. Totally agree with Sam that this is the wrong place - please check outside of this method.
Created attachment 315426 [details] Patch and layout test
(In reply to Daniel Bates from comment #15) > I will amend the patch to add a FIXME to track whether the source frame's page actually exists and follow up in a separate bug. > Filed bug #174531.
Committed r219528: <http://trac.webkit.org/changeset/219528>
It was not practical to make a regression test?
(In reply to Darin Adler from comment #20) > It was not practical to make a regression test? I made a regression test and landed it as part of the fix.
(In reply to Daniel Bates from comment #21) > I made a regression test and landed it as part of the fix. Great news! Sorry I overlooked that.