Bug 174385 - REGRESSION (r219013): Compute source frame info for frameless document
Summary: REGRESSION (r219013): Compute source frame info for frameless document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar, Regression
Depends on: 174386
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-11 13:35 PDT by Daniel Bates
Modified: 2017-07-17 18:44 PDT (History)
6 users (show)

See Also:


Attachments
Patch and layout test (8.47 KB, patch)
2017-07-11 13:42 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (8.52 KB, patch)
2017-07-12 11:23 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.72 MB, application/zip)
2017-07-12 12:18 PDT, Build Bot
no flags Details
Patch and layout test (9.17 KB, patch)
2017-07-12 12:31 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (7.72 MB, application/zip)
2017-07-12 13:54 PDT, Build Bot
no flags Details
Patch and layout test (8.16 KB, patch)
2017-07-12 15:55 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and layout test (8.16 KB, patch)
2017-07-12 15:57 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (10.71 MB, application/zip)
2017-07-12 17:17 PDT, Build Bot
no flags Details
Patch and layout test (8.67 KB, patch)
2017-07-14 09:13 PDT, Daniel Bates
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2017-07-11 13:35:39 PDT
WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction() can be called with a source document that has no frame.
Comment 1 Daniel Bates 2017-07-11 13:36:07 PDT
<rdar://problem/33217736>
Comment 2 Daniel Bates 2017-07-11 13:42:41 PDT
Created attachment 315158 [details]
Patch and layout test
Comment 3 Daniel Bates 2017-07-12 11:23:11 PDT
Created attachment 315262 [details]
Patch and layout test
Comment 4 Build Bot 2017-07-12 12:18:06 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-07-12 12:18:07 PDT Comment hidden (obsolete)
Comment 6 Daniel Bates 2017-07-12 12:31:33 PDT
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 7 Build Bot 2017-07-12 13:54:19 PDT
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
Comment 8 Build Bot 2017-07-12 13:54:21 PDT
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
Comment 9 Daniel Bates 2017-07-12 14:03:44 PDT
(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
Comment 10 Daniel Bates 2017-07-12 15:55:23 PDT
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.
Comment 11 Daniel Bates 2017-07-12 15:57:32 PDT
Created attachment 315295 [details]
Patch and layout test
Comment 12 Build Bot 2017-07-12 17:17:08 PDT
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
Comment 13 Build Bot 2017-07-12 17:17:10 PDT
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 14 Sam Weinig 2017-07-12 19:13:29 PDT
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).
Comment 15 Daniel Bates 2017-07-12 19:48:06 PDT
(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 16 Brady Eidson 2017-07-14 09:09:48 PDT
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.
Comment 17 Daniel Bates 2017-07-14 09:13:04 PDT
Created attachment 315426 [details]
Patch and layout test
Comment 18 Daniel Bates 2017-07-14 15:27:02 PDT
(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.
Comment 19 Daniel Bates 2017-07-14 15:28:15 PDT
Committed r219528: <http://trac.webkit.org/changeset/219528>
Comment 20 Darin Adler 2017-07-15 23:41:43 PDT
It was not practical to make a regression test?
Comment 21 Daniel Bates 2017-07-16 03:44:51 PDT
(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.
Comment 22 Darin Adler 2017-07-17 18:44:09 PDT
(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.