1167808 - chrome service worker 'read-through-caching' demo does not load correctly

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

+++ This bug was initially created as a clone of Bug #1157619 +++

From bug 1157619 comment 13:

> I am seeing a number of issues, none of which are the original issue in the
> description.
> 
> 1) I visit the website, the page is loaded from the network as expected.
> 2) Reload the page (not force reload!). At this point the spinner keeps
> spinning, while the console has logs about the SW trying to fetch from
> network since there are no cached resources.
> 3) Step 2 does not finish in a reasonable time.
> 4) Reload the page again (again not a force reload), interrupting the
> earlier load. Now it loads fine and you can see console logs that stuff was
> retrieved from the cache. BUT, the images are broken (shows the gecko broken
> image icon), presumably because the caching was stopped halfway or because
> of encoding issues or similar (have to dig into this)
> 5) If I open the same URL in a new tab, for some reason it is not controlled
> until I reload that tab. I would expect that since an active worker is
> available, the first load is also controlled.

If you apply the patch from that bug and use force-refresh, most of these symptoms go away.  We need to determine what is breaking in the first load.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I think the case where you load with the SW controlling and the tab icon continues to spin is a slow load of the analytics.js.  Its unclear to me why this would be taking a long time.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This is possibly cache related, so I will take it.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

The only symptom I see with current nightly is the slow loading spinner now.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

It appears some of the resources have no body coming out of cache.  Investigating if its a problem with putting in or getting out.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

We are not handling opaque responses properly.  The body is null in js script, but things like respondWith() and cache.put() should be accessing the internal body.  This is not clear from the steps, but the spec text says:

"First, unlike same-origin resources which are managed in the Cache as Response objects with the type attribute set to "basic", the objects stored are Response objects with the type attribute set to "opaque". Responses typed "opaque" provide a much less expressive API than Responses typed "basic"; the bodies and headers cannot be read or set, nor many of the other aspects of their content inspected. They can be passed to event.respondWith(r) method in the same manner as the Responses typed "basic", but cannot be meaningfully created programmatically. These limitations are necessary to preserve the security invariants of the platform. Allowing Caches to store them allows applications to avoid re-architecting in most cases."

And also:

"If the request is a top-level navigation and the return value is a Response whose type attribute is "opaque" (i.e., an opaque response body), a network error is returned to Fetch."

I wrote a spec issue to clarify this in the steps:

https://github.com/slightlyoff/ServiceWorker/issues/710

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 FetchEvent.respondWith() and Cache.put() should use internal body of opaque Response. r=nsm

This patch makes respondWith() and cache use the internal body.  I will add a test in a follow-up.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

We should fix bug 1173912 before landing this.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8620747 [details] [diff] [review]
P1 FetchEvent.respondWith() and Cache.put() should use internal body of opaque Response. r=nsm

Review of attachment 8620747 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalResponse.h
@@ +144,5 @@
>    void
> +  GetBody(nsIInputStream** aStream)
> +  {
> +    if (Type() == ResponseType::Opaque) {
> +      printf_stderr("### ### InternalResponse::GetBody() return null for opaque\n");

leftover or NS_WARNING?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that respondWith() for a browser load reads the body correctly. r=ehsan

Add a test that intercepts a <script> tag with a cross-origin opaque response.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cf6f04d81c

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8621391 [details] [diff] [review]
P2 Test that respondWith() for a browser load reads the body correctly. r=ehsan

Try shows that this does not work quite right in automation.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Oh, a previously registered SW is replacing my controlled iframe.  Let me make sure that is unregistered before completing the previous test.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test that respondWith() for a browser load reads the body correctly. r=ehsan

The failures in the last try were due to reusing the cache name from a previous test which resulted in a changed page I was not expecting.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 FetchEvent.respondWith() and Cache.put() should use internal body of opaque Response. r=nsm

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32852f4f923f

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Cache.put() should use internal body of opaque Response. r=nsm

Split the cache.put() change off from the respondWith() change.  We need to wait for the dependent bugs for the respondWith(), but I can land the cache.put side now.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 FetchEvent.responseWith() should use opaque response internal body. r=nsm

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f489150f45

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e7f489150f45

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

We're going to use a pref to disable opaque interception for now.  This will let us land the patches here and close the bug while continuing to work on security issues.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add dom.serviceWorkers.interception.opaque.enabled pref. r=ehsan

Add the dom.serviceWorkers.interception.opaque.enabled pref to control if opaque responses should be allowed in FetchEvent.respondWith().

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Block opaque response interception if pref is disabled. r=ehsan

Short-circuit FetchEvent.respondWith() if the response is opaque and the pref is not set.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 FetchEvent.respondWith() should use opaque response internal body. r=nsm

Re-number patch.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Test that respondWith() for a browser load reads the body correctly. r=ehsan

Re-number patch.  Also enable the pref in the test.  Also fix an infinite fail loop that happens when the pref is not set.

Comment 26

[(no longer active)]

Comment on attachment 8629048 [details] [diff] [review]
P1 Add dom.serviceWorkers.interception.opaque.enabled pref. r=ehsan

Review of attachment 8629048 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +135,5 @@
>  // Service workers
>  pref("dom.serviceWorkers.enabled", false);
>  
> +// Allow service workers to intercept network requests using the fetch event
> +pref("dom.serviceWorkers.interception.enable", false);

You have misspelled this: *enabled.

@@ +138,5 @@
> +// Allow service workers to intercept network requests using the fetch event
> +pref("dom.serviceWorkers.interception.enable", false);
> +
> +// Allow service workers to intercept opaque (cross origin) responses
> +pref("dom.serviceWorkers.interception.opaque.enable", false);

Ditto.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48abf470ef5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ac89eafb1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c94c759a0fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9c72d93973

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for mulet mochitest-4 orange: https://hg.mozilla.org/integration/mozilla-inbound/rev/48347c4899df

https://treeherder.mozilla.org/logviewer.html#?job_id=11325482&repo=mozilla-inbound

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Not just on mulet: https://treeherder.mozilla.org/logviewer.html#?job_id=11326528&repo=mozilla-inbound

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Test that respondWith() for a browser load reads the body correctly. r=ehsan

Updated the test patch to enable the new pref in existing tests that use opaque responses.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53dbf04781d5

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb107d007f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff32b1d206bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd90691926c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9901be6c59c

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0bb107d007f2
https://hg.mozilla.org/mozilla-central/rev/ff32b1d206bb
https://hg.mozilla.org/mozilla-central/rev/0cd90691926c
https://hg.mozilla.org/mozilla-central/rev/e9901be6c59c