Bug 174385 – REGRESSION (r219013): Compute source frame info for frameless document

• Component: WebKit2
• Status: RESOLVED FIXED

[Daniel Bates]

WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() can be called with a source document that has no frame.

[Daniel Bates]

<rdar://problem/33217736>

[Daniel Bates]

Created attachment 315158 [details]
Patch and layout test

[Daniel Bates]

Created attachment 315262 [details]
Patch and layout test

[Build Bot]

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

[Build Bot]

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

[Daniel Bates]

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.

[Build Bot]

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

[Build Bot]

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

[Daniel Bates]

(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

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 315295 [details]
Patch and layout test

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

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).

[Daniel Bates]

(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.

[Brady Eidson]

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.

[Daniel Bates]

Created attachment 315426 [details]
Patch and layout test

[Daniel Bates]

(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.

[Daniel Bates]

Committed r219528: <http://trac.webkit.org/changeset/219528>

[Darin Adler]

It was not practical to make a regression test?

[Daniel Bates]

(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.

[Darin Adler]

(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.