Bug 183345 - fast/loader/redirect-to-invalid-url-using-meta-refresh-disallowed.html fails with async policy delegates
Summary: fast/loader/redirect-to-invalid-url-using-meta-refresh-disallowed.html fails ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 180568
  Show dependency treegraph
 
Reported: 2018-03-05 14:21 PST by Ali Juma
Modified: 2018-03-06 14:02 PST (History)
11 users (show)

See Also:


Attachments
WIP (6.31 KB, patch)
2018-03-05 14:26 PST, Ali Juma
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
WIP patch (chris) (19.59 KB, patch)
2018-03-05 15:38 PST, Chris Dumez
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.37 MB, application/zip)
2018-03-05 15:38 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.07 MB, application/zip)
2018-03-05 16:19 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.88 MB, application/zip)
2018-03-05 16:42 PST, EWS Watchlist
no flags Details
WIP patch (37.33 KB, patch)
2018-03-06 10:52 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (39.64 KB, patch)
2018-03-06 10:58 PST, Chris Dumez
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.61 MB, application/zip)
2018-03-06 11:50 PST, EWS Watchlist
no flags Details
WIP patch (39.66 KB, patch)
2018-03-06 12:03 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (48.12 KB, patch)
2018-03-06 13:09 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2018-03-05 14:21:05 PST
This fails for two reasons:
1) With async policy delegates, FrameLoader::m_quickRedirectComing gets cleared before the policy response is received, so continueLoadAfterNavigationPolicy doesn't call clientRedirectCancelledOrFinished, which means WebFrameLoaderClient::dispatchDidCancelClientRedirect never gets called.
2) The test doesn't wait long enough to receive an async policy response; it just uses setTimeout(_, 0).
Comment 1 Ali Juma 2018-03-05 14:26:31 PST
Created attachment 335028 [details]
WIP
Comment 2 Ali Juma 2018-03-05 14:28:02 PST
Comment on attachment 335028 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=335028&action=review

> LayoutTests/fast/loader/redirect-to-invalid-url-using-meta-refresh-disallowed-async-delegate.html:26
> +}, 250);

Is there a better way than setTimeout to make the test wait for the async policy response?
Comment 3 Chris Dumez 2018-03-05 14:41:06 PST
I am away from my machine now but I do not think this is the right fix. If I remember correctly, the issue is that this flag gets set, then we call a method that used to be synchronous, then the flag gets reset. Now that the method is async, I fixed this by passing in a lambda. I will confirm when I get back to my computer that this is the same issue I was working on.
Comment 4 Chris Dumez 2018-03-05 15:37:16 PST
(In reply to Chris Dumez from comment #3)
> I am away from my machine now but I do not think this is the right fix. If I
> remember correctly, the issue is that this flag gets set, then we call a
> method that used to be synchronous, then the flag gets reset. Now that the
> method is async, I fixed this by passing in a lambda. I will confirm when I
> get back to my computer that this is the same issue I was working on.

I was referring to this call site:
    bool isRedirect = m_quickRedirectComing;
    loadWithNavigationAction(request, action, lockHistory, newLoadType, formState, allowNavigationToInvalidURL);
    if (isRedirect) {
        m_quickRedirectComing = false;

loadWithNavigationAction() now gets the policy decision asynchronously, and thus relies on m_quickRedirectComing asynchronously. This is an issue here because this call site sets it back to false synchronously.
Comment 5 Chris Dumez 2018-03-05 15:38:23 PST
Created attachment 335038 [details]
WIP patch (chris)

This is the work in progress patch I had locally. I had not uploaded it yet because the test was still failing after the change.

Do you agree with my analysis?
Comment 6 EWS Watchlist 2018-03-05 15:38:38 PST
Comment on attachment 335028 [details]
WIP

Attachment 335028 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6816984

New failing tests:
fast/loader/redirect-to-invalid-url-using-meta-refresh-disallowed-async-delegate.html
Comment 7 EWS Watchlist 2018-03-05 15:38:39 PST
Created attachment 335040 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 8 Chris Dumez 2018-03-05 16:07:05 PST
Comment on attachment 335028 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=335028&action=review

>> LayoutTests/fast/loader/redirect-to-invalid-url-using-meta-refresh-disallowed-async-delegate.html:26
>> +}, 250);
> 
> Is there a better way than setTimeout to make the test wait for the async policy response?

Unfortunately we have to find a better way (not sure how yet). Relying on a timer like this will almost certainly cause the test to be flaky on our bots.
Comment 9 Ali Juma 2018-03-05 16:16:52 PST
(In reply to Chris Dumez from comment #5)
> Created attachment 335038 [details]
> WIP patch (chris)
> 
> This is the work in progress patch I had locally. I had not uploaded it yet
> because the test was still failing after the change.
> 
> Do you agree with my analysis?

Yes, your analysis makes sense to me.
Comment 10 Chris Dumez 2018-03-05 16:18:18 PST
(In reply to Ali Juma from comment #9)
> (In reply to Chris Dumez from comment #5)
> > Created attachment 335038 [details]
> > WIP patch (chris)
> > 
> > This is the work in progress patch I had locally. I had not uploaded it yet
> > because the test was still failing after the change.
> > 
> > Do you agree with my analysis?
> 
> Yes, your analysis makes sense to me.

Ok, then we just need to find a reliable way to update tests. Unfortunately, there are several tests that are missing such "didCancelClientRedirectForFrame" line.
Comment 11 EWS Watchlist 2018-03-05 16:19:35 PST
Comment on attachment 335028 [details]
WIP

Attachment 335028 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6817026

New failing tests:
fast/loader/redirect-to-invalid-url-using-meta-refresh-disallowed-async-delegate.html
imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html
Comment 12 EWS Watchlist 2018-03-05 16:19:45 PST
Created attachment 335046 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 13 Ali Juma 2018-03-05 16:20:51 PST
(In reply to Chris Dumez from comment #10)
> (In reply to Ali Juma from comment #9)
> > (In reply to Chris Dumez from comment #5)
> > > Created attachment 335038 [details]
> > > WIP patch (chris)
> > > 
> > > This is the work in progress patch I had locally. I had not uploaded it yet
> > > because the test was still failing after the change.
> > > 
> > > Do you agree with my analysis?
> > 
> > Yes, your analysis makes sense to me.
> 
> Ok, then we just need to find a reliable way to update tests. Unfortunately,
> there are several tests that are missing such
> "didCancelClientRedirectForFrame" line.

Maybe we need to add something like a TestRunner API that takes a callback and calls it after an IPC round-trip to/from the UIProcess?
Comment 14 Chris Dumez 2018-03-05 16:27:47 PST
(In reply to Ali Juma from comment #13)
> (In reply to Chris Dumez from comment #10)
> > (In reply to Ali Juma from comment #9)
> > > (In reply to Chris Dumez from comment #5)
> > > > Created attachment 335038 [details]
> > > > WIP patch (chris)
> > > > 
> > > > This is the work in progress patch I had locally. I had not uploaded it yet
> > > > because the test was still failing after the change.
> > > > 
> > > > Do you agree with my analysis?
> > > 
> > > Yes, your analysis makes sense to me.
> > 
> > Ok, then we just need to find a reliable way to update tests. Unfortunately,
> > there are several tests that are missing such
> > "didCancelClientRedirectForFrame" line.
> 
> Maybe we need to add something like a TestRunner API that takes a callback
> and calls it after an IPC round-trip to/from the UIProcess?

Do you mind if I take this over? We don't even need to involve the UIProcess since the line in question is logged in the WebProcess via InjectedBundle.
Comment 15 EWS Watchlist 2018-03-05 16:42:42 PST
Comment on attachment 335038 [details]
WIP patch (chris)

Attachment 335038 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6817569

New failing tests:
imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name.html
Comment 16 EWS Watchlist 2018-03-05 16:42:43 PST
Created attachment 335051 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 17 Ali Juma 2018-03-05 16:51:05 PST
(In reply to Chris Dumez from comment #14)
> (In reply to Ali Juma from comment #13)
> > (In reply to Chris Dumez from comment #10)
> > > (In reply to Ali Juma from comment #9)
> > > > (In reply to Chris Dumez from comment #5)
> > > > > Created attachment 335038 [details]
> > > > > WIP patch (chris)
> > > > > 
> > > > > This is the work in progress patch I had locally. I had not uploaded it yet
> > > > > because the test was still failing after the change.
> > > > > 
> > > > > Do you agree with my analysis?
> > > > 
> > > > Yes, your analysis makes sense to me.
> > > 
> > > Ok, then we just need to find a reliable way to update tests. Unfortunately,
> > > there are several tests that are missing such
> > > "didCancelClientRedirectForFrame" line.
> > 
> > Maybe we need to add something like a TestRunner API that takes a callback
> > and calls it after an IPC round-trip to/from the UIProcess?
> 
> Do you mind if I take this over? We don't even need to involve the UIProcess
> since the line in question is logged in the WebProcess via InjectedBundle.

Sure, go ahead :)
Comment 18 Chris Dumez 2018-03-06 09:23:51 PST
Tests failing in a similar way:
fast/loader/redirect-to-invalid-url-using-javascript-disallowed.html
fast/loader/redirect-to-invalid-url-using-meta-refresh-disallowed.html
fast/loader/window-open-to-invalid-url-disallowed.html
Comment 19 Chris Dumez 2018-03-06 10:52:44 PST
Created attachment 335115 [details]
WIP patch
Comment 20 Chris Dumez 2018-03-06 10:58:26 PST
Created attachment 335116 [details]
WIP patch
Comment 21 EWS Watchlist 2018-03-06 11:50:19 PST
Comment on attachment 335116 [details]
WIP patch

Attachment 335116 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6829103

New failing tests:
imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name.html
Comment 22 EWS Watchlist 2018-03-06 11:50:20 PST
Created attachment 335120 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 23 Chris Dumez 2018-03-06 12:03:49 PST
Created attachment 335125 [details]
WIP patch
Comment 24 Chris Dumez 2018-03-06 13:09:52 PST
Created attachment 335130 [details]
Patch
Comment 25 Chris Dumez 2018-03-06 14:01:32 PST
Comment on attachment 335130 [details]
Patch

Clearing flags on attachment: 335130

Committed r229341: <https://trac.webkit.org/changeset/229341>
Comment 26 Chris Dumez 2018-03-06 14:01:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2018-03-06 14:02:33 PST
<rdar://problem/38193666>