Bug 183345

Created attachment 335028 [details] WIP

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?

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.

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

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

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

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

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

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

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

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

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

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

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

Created attachment 335115 [details] WIP patch

Created attachment 335116 [details] WIP patch

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

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

Created attachment 335125 [details] WIP patch

Created attachment 335130 [details] Patch

Comment on attachment 335130 [details] Patch Clearing flags on attachment: 335130 Committed r229341: <https://trac.webkit.org/changeset/229341>

All reviewed patches have been landed. Closing bug.

<rdar://problem/38193666>