Closed Bug 1167808 Opened 5 years ago Closed 4 years ago

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


(Core :: DOM: Service Workers, defect)

Not set



Tracking Status
firefox41 --- affected
firefox42 --- fixed


(Reporter: bkelly, Assigned: bkelly)





(5 files, 6 obsolete files)

1.82 KB, patch
: review+
: checkin+
Details | Diff | Splinter Review
7.62 KB, patch
: review+
Details | Diff | Splinter Review
1.84 KB, patch
: review+
Details | Diff | Splinter Review
1.04 KB, patch
: review+
Details | Diff | Splinter Review
9.33 KB, patch
: review+
Details | Diff | Splinter Review
+++ 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.
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.
This is possibly cache related, so I will take it.
Assignee: nobody → bkelly
The only symptom I see with current nightly is the slow loading spinner now.
It appears some of the resources have no body coming out of cache.  Investigating if its a problem with putting in or getting out.
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:
This patch makes respondWith() and cache use the internal body.  I will add a test in a follow-up.
Attachment #8620747 - Flags: review?(nsm.nikhil)
We should fix bug 1173912 before landing this.
Depends on: 1173912
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?
Attachment #8620747 - Flags: review?(nsm.nikhil) → review+
Add a test that intercepts a <script> tag with a cross-origin opaque response.
Attachment #8621391 - Flags: review?(ehsan)
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.
Attachment #8621391 - Flags: review?(ehsan)
Oh, a previously registered SW is replacing my controlled iframe.  Let me make sure that is unregistered before completing the previous test.
Depends on: 1173811
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.
Attachment #8621391 - Attachment is obsolete: true
Attachment #8621657 - Flags: review?(ehsan)
Attachment #8621657 - Flags: review?(ehsan) → review+
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.
Attachment #8621659 - Attachment is obsolete: true
Attachment #8621780 - Flags: review+
Duplicate of this bug: 1174299
Keywords: leave-open
Attachment #8621780 - Flags: checkin+
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.
No longer depends on: 1173811, 1173912
Keywords: leave-open
Add the dom.serviceWorkers.interception.opaque.enabled pref to control if opaque responses should be allowed in FetchEvent.respondWith().
Attachment #8629048 - Flags: review?(ehsan)
Short-circuit FetchEvent.respondWith() if the response is opaque and the pref is not set.
Attachment #8629049 - Flags: review?(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.
Attachment #8621657 - Attachment is obsolete: true
Attachment #8629058 - Flags: review+
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);

Attachment #8629048 - Flags: review?(ehsan) → review+
Attachment #8629049 - Flags: review?(ehsan) → review+
Updated the test patch to enable the new pref in existing tests that use opaque responses.
Attachment #8629058 - Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8629176 - Flags: review+
You need to log in before you can comment on or make changes to this bug.