Bug 186349 – Don't start service worker fetch when there is substitute data

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

[Antti Koivisto]

Loading content via WKWebView.loadData may also end up starting a service worker fetch. This breaks DocumentWriter assumptions.

[Antti Koivisto]

<rdar://problem/38881568>

[Antti Koivisto]

Created attachment 342046 [details]
patch

[youenn fablet]

Comment on attachment 342046 [details]
patch

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

> Source/WebCore/ChangeLog:10
> +        This breaks DocumentWriter assumptions.

This is an edge case but could we try writing an API test for that?
Something like:
- Load a first content that would register the service worker
- Call WKWebView.loadData on an URL that should be controlled by the service worker
- Ensure that the newly loaded page is controlled by the service worker

See Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm for some existing SW API tests.

> Source/WebCore/loader/DocumentLoader.cpp:1739
> +            loadMainResource(WTFMove(request));

WPE/GTK are usually not happy if removing 'this'.

[Antti Koivisto]

Created attachment 342049 [details]
patch

[Antti Koivisto]

> See Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm for some
> existing SW API tests.

Is there an API test for the existing tryLoadingRequestFromApplicationCache() case?

[youenn fablet]

(In reply to Antti Koivisto from comment #5)
> > See Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm for some
> > existing SW API tests.
> 
> Is there an API test for the existing
> tryLoadingRequestFromApplicationCache() case?

API tests do not have access to an HTTP server yet and I believe appcache would require such an infrastructure. It might be yet another reason to work on bug 180915.

That said, this specific tryLoadingRequestFromApplicationCache() case is somehow covered by some WPT layout test ensuring that service worker is taking precedence over appcache.

[Alexey Proskuryakov]

It may be easier to add functionality to WebKitTestRunner (or even Internals) than to API tests. Layout tests have much better infrastructure around them too.

[Antti Koivisto]

Created attachment 342136 [details]
patch

[EWS Watchlist]

Comment on attachment 342136 [details]
patch

Attachment 342136 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8051454

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 342148 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Antti Koivisto]

Created attachment 342150 [details]
patch

[Antti Koivisto]

Created attachment 342151 [details]
patch

[Antti Koivisto]

Comment on attachment 342151 [details]
patch

Now with API test.

[EWS Watchlist]

Comment on attachment 342151 [details]
patch

Attachment 342151 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8053449

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 342157 [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

[WebKit Commit Bot]

Comment on attachment 342151 [details]
patch

Clearing flags on attachment: 342151

Committed r232580: <https://trac.webkit.org/changeset/232580>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.