Bug 183345 – fast/loader/redirect-to-invalid-url-using-meta-refresh-disallowed.html fails with async policy delegates

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Ali Juma]

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

[Ali Juma]

Created attachment 335028 [details]
WIP

[Ali Juma]

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?

[Chris Dumez]

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.

[Chris Dumez]

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

[Chris Dumez]

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?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

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.

[Ali Juma]

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

[Chris Dumez]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ali Juma]

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

[Chris Dumez]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ali Juma]

(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 :)

[Chris Dumez]

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

[Chris Dumez]

Created attachment 335115 [details]
WIP patch

[Chris Dumez]

Created attachment 335116 [details]
WIP patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 335125 [details]
WIP patch

[Chris Dumez]

Created attachment 335130 [details]
Patch

[Chris Dumez]

Comment on attachment 335130 [details]
Patch

Clearing flags on attachment: 335130

Committed r229341: <https://trac.webkit.org/changeset/229341>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38193666>