Bug 186349 - Don't start service worker fetch when there is substitute data
Summary: Don't start service worker fetch when there is substitute data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-06 07:12 PDT by Antti Koivisto
Modified: 2018-06-07 07:59 PDT (History)
7 users (show)

See Also:


Attachments
patch (4.81 KB, patch)
2018-06-06 08:16 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (4.61 KB, patch)
2018-06-06 08:51 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (7.31 KB, patch)
2018-06-07 00:47 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.72 MB, application/zip)
2018-06-07 04:28 PDT, Build Bot
no flags Details
patch (7.23 KB, patch)
2018-06-07 05:17 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (7.20 KB, patch)
2018-06-07 05:19 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.83 MB, application/zip)
2018-06-07 07:03 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2018-06-06 07:12:34 PDT
Loading content via WKWebView.loadData may also end up starting a service worker fetch. This breaks DocumentWriter assumptions.
Comment 1 Antti Koivisto 2018-06-06 08:12:30 PDT
<rdar://problem/38881568>
Comment 2 Antti Koivisto 2018-06-06 08:16:33 PDT
Created attachment 342046 [details]
patch
Comment 3 youenn fablet 2018-06-06 08:40:34 PDT
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'.
Comment 4 Antti Koivisto 2018-06-06 08:51:17 PDT
Created attachment 342049 [details]
patch
Comment 5 Antti Koivisto 2018-06-06 08:58:25 PDT
> See Tools/TestWebKitAPI/Tests/WebKitCocoa/ServiceWorkerBasic.mm for some
> existing SW API tests.

Is there an API test for the existing tryLoadingRequestFromApplicationCache() case?
Comment 6 youenn fablet 2018-06-06 09:05:28 PDT
(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.
Comment 7 Alexey Proskuryakov 2018-06-06 13:14:55 PDT
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.
Comment 8 Antti Koivisto 2018-06-07 00:47:29 PDT
Created attachment 342136 [details]
patch
Comment 9 Build Bot 2018-06-07 04:28:42 PDT
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
Comment 10 Build Bot 2018-06-07 04:28:53 PDT
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
Comment 11 Antti Koivisto 2018-06-07 05:17:13 PDT
Created attachment 342150 [details]
patch
Comment 12 Antti Koivisto 2018-06-07 05:19:45 PDT
Created attachment 342151 [details]
patch
Comment 13 Antti Koivisto 2018-06-07 05:59:54 PDT
Comment on attachment 342151 [details]
patch

Now with API test.
Comment 14 Build Bot 2018-06-07 07:03:29 PDT
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
Comment 15 Build Bot 2018-06-07 07:03:41 PDT
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
Comment 16 WebKit Commit Bot 2018-06-07 07:59:08 PDT
Comment on attachment 342151 [details]
patch

Clearing flags on attachment: 342151

Committed r232580: <https://trac.webkit.org/changeset/232580>
Comment 17 WebKit Commit Bot 2018-06-07 07:59:10 PDT
All reviewed patches have been landed.  Closing bug.