Comment on attachment 328779[details]
Patch
WIP, almost certainly has problems. Double check all API tests, especially WebKit.JavaScriptDuringNavigation and download tests without NETWORK_SESSION.
Will this change help resolve some of the regressions caused by the async policy delegate change?
I ask because I don't think we should push further in the direction of design changes that increase async behavior until we've first finished resolving the regressions caused by the last batch of async behavior.
Created attachment 331575[details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 331579[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 333625[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 333630[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
WebResourceLoader::didReceiveResponse() sends the NetworkResourceLoader::ContinueDidReceiveResponse IPC back to the network process right after calling ResourceLosder::didReceiveResponse(). Unfortunately, if the client replies asynchronously to the decidePolicyForNavigationResponse delegate, then this IPC will be sent before receiving the policy response. As a result, we'll start receiving data and even potentially commit the provisional load before the client responds to the policy delegate. I think we need to delay the NetworkResourceLoader::ContinueDidReceiveResponse IPC until we get a policy response from the client.
(In reply to Chris Dumez from comment #15)
> WebResourceLoader::didReceiveResponse() sends the
> NetworkResourceLoader::ContinueDidReceiveResponse IPC back to the network
> process right after calling ResourceLosder::didReceiveResponse().
> Unfortunately, if the client replies asynchronously to the
> decidePolicyForNavigationResponse delegate, then this IPC will be sent
> before receiving the policy response. As a result, we'll start receiving
> data and even potentially commit the provisional load before the client
> responds to the policy delegate. I think we need to delay the
> NetworkResourceLoader::ContinueDidReceiveResponse IPC until we get a policy
> response from the client.
Will address this via Bug 182720.
Created attachment 334391[details]
Archive of layout-test-results from ews105 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 334392[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 334840[details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 334843[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.6
Created attachment 334909[details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 334912[details]
Archive of layout-test-results from ews115 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 334914[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 334922[details]
Archive of layout-test-results from ews202 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 334941[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
Created attachment 334942[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.6
Created attachment 335156[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 335162[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
Created attachment 335226[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
Created attachment 335228[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 335252[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 335255[details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 335337[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 335338[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.6
Created attachment 335667[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 335668[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
Created attachment 335674[details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 335676[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 335681[details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 335682[details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 335719[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Looking at URLSchemeHandler.Redirection API test, I see that the API test scheme handler does:
auto redirectResponse = adoptNS([[NSURLResponse alloc] initWithURL:task.request.URL MIMEType:nil expectedContentLength:0 textEncodingName:nil]);
auto request = adoptNS([[NSURLRequest alloc] initWithURL:[NSURL URLWithString:@"testing:///redirected"]]);
[(id<WKURLSchemeTaskPrivate>)task _didPerformRedirection:redirectResponse.get() newRequest:request.get()];
ASSERT_FALSE(receivedRedirect);
ASSERT_STREQ(task.request.URL.absoluteString.UTF8String, "testing:///redirected");
NSString *html = @"<script>window.webkit.messageHandlers.testHandler.postMessage('Document URL: ' + document.URL);</script>";
auto finalResponse = adoptNS([[NSURLResponse alloc] initWithURL:task.request.URL MIMEType:@"text/html" expectedContentLength:html.length textEncodingName:nil]);
[task didReceiveResponse:finalResponse.get()];
[task didReceiveData:[html dataUsingEncoding:NSUTF8StringEncoding]];
[task didFinish];
All synchronously.
Now that policy delegates are async, the call to didReceiveResponse() happens while we're still processing the redirect from the _didPerformRedirection call. I thus see logging like so:
Received response during redirect processing.
Redirected scheme task would have been sent to a different URL.
Not sure what the best way to fix this is. My bet is that to maintain API compatibility, we'd have to make WebURLSchemeTaskProxy robust against this happening. E.g. if didReceiveResponse is called while we're still processing a redirect, then queue the response until the redirect has been processed.
Alex, please let me know if I'm wrong since I am not familiar with this code.
(In reply to Chris Dumez from comment #92)
> Alex, please let me know if I'm wrong since I am not familiar with this code.
This test uses SPI on WKURLSchemeTaskPrivate and it's probably not critical to maintain complete API compatibility because that SPI should only be used with internal tests.
(In reply to Alex Christensen from comment #96)
> (In reply to Chris Dumez from comment #92)
> > Alex, please let me know if I'm wrong since I am not familiar with this code.
> This test uses SPI on WKURLSchemeTaskPrivate and it's probably not critical
> to maintain complete API compatibility because that SPI should only be used
> with internal tests.
I see. Well, it does not add much complexity as you can see in my latest patch.
I don't think we should maintain an assumption that redirection on WKURLSchemeTasks is effectively synchronous. I'd prefer to just change the test to call didReceiveResponse, didReceiveData, and didFinish after decidePolicyForNavigationAction has finished. If that doesn't work because didReceiveResponse is now asynchronous, we might need to take your solution to keep API compatibility with WKURLSchemeHandler.
(In reply to Alex Christensen from comment #98)
> I don't think we should maintain an assumption that redirection on
> WKURLSchemeTasks is effectively synchronous. I'd prefer to just change the
> test to call didReceiveResponse, didReceiveData, and didFinish after
> decidePolicyForNavigationAction has finished. If that doesn't work because
> didReceiveResponse is now asynchronous, we might need to take your solution
> to keep API compatibility with WKURLSchemeHandler.
We're not talking about a single API test here given how many failures were resolved by my change.
Ah, decidePolicyForNavigationResponse is also now always asynchronous, so the queueing is necessary. Makes sense. This was needed before if you had an asynchronous decidePolicyForNavigationResponse but nobody realized it. It might be worth adding a regression test for that case that is now fixed.
r=me
Comment on attachment 335881[details]
Patch
The cleanups in DocumentLoader and WebURLSchemeTaskProxy should each be done in separate patches with unit tests similar to the API tests they fix but with a WKNavigationDelegate that asynchronously calls the completion handlers. They are fixing existing bugs that are unrelated to the switch to make everything asynchronous.
(In reply to Chris Dumez from comment #101)
> Created attachment 335881[details]
> Patch
With this latest iteration, I am down to 2 API tests failing:
WebKit.WebsitePoliciesAutoplayQuirks
WebKit.WebsitePoliciesPlayAfterPreventedAutoplay
(In reply to Alex Christensen from comment #102)
> Comment on attachment 335881[details]
> Patch
>
> The cleanups in DocumentLoader and WebURLSchemeTaskProxy should each be done
> in separate patches with unit tests similar to the API tests they fix but
> with a WKNavigationDelegate that asynchronously calls the completion
> handlers. They are fixing existing bugs that are unrelated to the switch to
> make everything asynchronous.
Ok.
Created attachment 335892[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
Created attachment 335897[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.6
Created attachment 335993[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
Created attachment 335995[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 336061[details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 336062[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.6
Created attachment 336065[details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 336066[details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Chris Dumez from comment #132)
> Looks like there are some iOS-specific API test failures.
Tests that failed:
DataInteractionTests.AdditionalLinkAndImageIntoContentEditable
DataInteractionTests.CanStartDragOnDivWithDraggableAttribute
DataInteractionTests.DoNotCrashWhenSelectionMovesOffscreenAfterDragStart
DataInteractionTests.DragEventClientCoordinatesBasic
DataInteractionTests.DragImageFromContentEditable
DataInteractionTests.DragLiftPreviewDataTransferSetDragImage
DataInteractionTests.ImageDoesNotUseElementSizeAsEstimatedSize
DataInteractionTests.ImageToContentEditable
DataInteractionTests.ImageToTextarea
DataInteractionTests.InjectedBundleAttachmentElementData
DataInteractionTests.InjectedBundleImageElementData
DataInteractionTests.LargeImageToTargetDiv
ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash
WKWebView.SnapshotImageLargeAsyncDecoding
Tests that timed out:
QuickLook.NavigationDelegate
(In reply to Chris Dumez from comment #133)
> (In reply to Chris Dumez from comment #132)
> > Looks like there are some iOS-specific API test failures.
>
> Tests that failed:
> DataInteractionTests.AdditionalLinkAndImageIntoContentEditable
> DataInteractionTests.CanStartDragOnDivWithDraggableAttribute
> DataInteractionTests.DoNotCrashWhenSelectionMovesOffscreenAfterDragStart
> DataInteractionTests.DragEventClientCoordinatesBasic
> DataInteractionTests.DragImageFromContentEditable
> DataInteractionTests.DragLiftPreviewDataTransferSetDragImage
> DataInteractionTests.ImageDoesNotUseElementSizeAsEstimatedSize
> DataInteractionTests.ImageToContentEditable
> DataInteractionTests.ImageToTextarea
> DataInteractionTests.InjectedBundleAttachmentElementData
> DataInteractionTests.InjectedBundleImageElementData
> DataInteractionTests.LargeImageToTargetDiv
> ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash
> WKWebView.SnapshotImageLargeAsyncDecoding
>
> Tests that timed out:
> QuickLook.NavigationDelegate
Looks like fast/loader/javascript-url-iframe-remove-on-navigate-async-delegate.html was flaky too.
Comment on attachment 336210[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=336210&action=review> Source/WebKit/UIProcess/WebPageProxy.cpp:-2377
> - return;
My patch conflicted with Brady's and I do not think I resolved the conflict correctly. It seems we used to return early here if we were deciding a policy for a response. Therefore, Brady's code below would only be called for for navigation policy decisions. However, now it is called for both :(
(In reply to Alex Christensen from comment #0)
> Make policy decisions asynchronous
Can someone please explain what this means?
The reason I’m puzzled is that in the WebKit API, clients have always been able to make policy decisions asynchronously (in the legacy API, using a WebPolicyDecisionListener, in the modern API, using a decision handler block), and the web view has always remained responsive and interactive while waiting for the client to make its decision.
So it doesn’t sound like “make policy decisions asynchronous” refers to new API or new behavior of the existing API. What is it about, then?
Thanks!
(In reply to mitz from comment #149)
> (In reply to Alex Christensen from comment #0)
> > Make policy decisions asynchronous
>
> Can someone please explain what this means?
>
> The reason I’m puzzled is that in the WebKit API, clients have always been
> able to make policy decisions asynchronously (in the legacy API, using a
> WebPolicyDecisionListener, in the modern API, using a decision handler
> block), and the web view has always remained responsive and interactive
> while waiting for the client to make its decision.
>
> So it doesn’t sound like “make policy decisions asynchronous” refers to new
> API or new behavior of the existing API. What is it about, then?
>
> Thanks!
I responded on the other bug you commented on.
2017-12-07 19:30 PST, Alex Christensen
2018-01-17 18:06 PST, Alex Christensen
2018-01-17 18:49 PST, EWS Watchlist
2018-01-17 19:14 PST, EWS Watchlist
2018-02-12 12:43 PST, Chris Dumez
2018-02-12 13:35 PST, EWS Watchlist
2018-02-12 14:05 PST, EWS Watchlist
2018-02-21 08:46 PST, Chris Dumez
2018-02-21 10:32 PST, EWS Watchlist
2018-02-21 10:39 PST, EWS Watchlist
2018-03-01 12:00 PST, Chris Dumez
2018-03-01 12:32 PST, EWS Watchlist
2018-03-01 13:13 PST, EWS Watchlist
2018-03-02 10:09 PST, Chris Dumez
2018-03-02 10:53 PST, EWS Watchlist
2018-03-02 11:08 PST, EWS Watchlist
2018-03-02 11:56 PST, EWS Watchlist
2018-03-02 13:47 PST, EWS Watchlist
2018-03-02 15:28 PST, Chris Dumez
2018-03-02 15:42 PST, Chris Dumez
2018-03-02 16:44 PST, EWS Watchlist
2018-03-02 17:12 PST, EWS Watchlist
2018-03-06 14:35 PST, Chris Dumez
2018-03-06 16:21 PST, EWS Watchlist
2018-03-06 17:34 PST, EWS Watchlist
2018-03-07 13:24 PST, Chris Dumez
2018-03-07 14:43 PST, EWS Watchlist
2018-03-07 14:58 PST, EWS Watchlist
2018-03-07 16:38 PST, Chris Dumez
2018-03-07 20:04 PST, EWS Watchlist
2018-03-07 20:31 PST, EWS Watchlist
2018-03-08 11:47 PST, Chris Dumez
2018-03-08 11:55 PST, Chris Dumez
2018-03-08 13:37 PST, EWS Watchlist
2018-03-08 13:42 PST, EWS Watchlist
2018-03-12 15:27 PDT, Chris Dumez
2018-03-12 16:08 PDT, Chris Dumez
2018-03-12 17:43 PDT, EWS Watchlist
2018-03-12 17:47 PDT, EWS Watchlist
2018-03-12 18:52 PDT, Chris Dumez
2018-03-12 20:04 PDT, EWS Watchlist
2018-03-12 20:18 PDT, EWS Watchlist
2018-03-12 20:25 PDT, Chris Dumez
2018-03-12 22:20 PDT, EWS Watchlist
2018-03-12 22:44 PDT, EWS Watchlist
2018-03-13 12:07 PDT, Chris Dumez
2018-03-13 13:25 PDT, EWS Watchlist
2018-03-13 18:19 PDT, Chris Dumez
2018-03-15 10:09 PDT, Chris Dumez
2018-03-15 10:25 PDT, Chris Dumez
2018-03-15 14:09 PDT, Chris Dumez
2018-03-15 15:23 PDT, EWS Watchlist
2018-03-15 15:51 PDT, EWS Watchlist
2018-03-16 11:35 PDT, Chris Dumez
2018-03-16 19:05 PDT, Chris Dumez
2018-03-16 20:20 PDT, EWS Watchlist
2018-03-16 20:28 PDT, EWS Watchlist
2018-03-19 09:59 PDT, Chris Dumez
2018-03-19 10:17 PDT, Chris Dumez
2018-03-19 11:27 PDT, EWS Watchlist
2018-03-19 11:41 PDT, EWS Watchlist
2018-03-19 11:55 PDT, EWS Watchlist
2018-03-19 12:02 PDT, EWS Watchlist
2018-03-19 12:47 PDT, Chris Dumez
2018-03-19 15:31 PDT, Chris Dumez
2018-03-21 10:14 PDT, Chris Dumez
2018-03-21 10:18 PDT, Chris Dumez
2018-03-21 10:53 PDT, Chris Dumez