1217501 - theguardian.com offline serviceworker does not work

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The Guardian recently added a service worker that shows a cross-world puzzle when offline:

  https://twitter.com/OliverJAsh/status/657200027983585280

It does not work in our current firefox nightly, even with opaque interception enabled.

I don't see any obvious errors in the console, browser console, or debug stderr.

Comment 1

[(no longer active)]

Their service worker is doing the following:

    var doesRequestAcceptHtml = function (request) {
        return request.headers.get('Accept')
            .split(',')
            .some(function (type) { return type === 'text/html'; });
    };

    this.addEventListener('fetch', function (event) {
        if (doesRequestAcceptHtml(event.request)) {
            isCacheUpdated().then(function (isUpdated) {
                if (!isUpdated) {
                    updateCache().then(deleteOldCaches);
                }
            });
        }

        event.respondWith(
            fetch(event.request)
                .catch(function () {
                    // If a request is cached, respond with that. Otherwise respond
                    // with the shell, whose subresources will be in the cache.
                    return caches.match(event.request).then(function (response) {
                        if (response) {
                            return response;
                        } else {
                            if (doesRequestAcceptHtml(event.request)) {
                                return caches.match('/offline-page');
                            }
                        }
                    })
                })
        );
    });

doesRequestAcceptHtml() returns false here, so they end up resolving the promise to undefined, and we get the following warning: "FetchEvent::RespondWith was passed a promise resolved to a non-Object value".

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

The spec is a bit unclear to me here.  I'll write a spec issue and note the problem with the author of the site.

Comment 3

[oliverjash]

Hi, I am the author of this piece of work. I would like to help getting it to work in Firefox.

Do you know why `doesRequestAcceptHtml` returns false? I only want to serve the offline page if the request accepts HTML.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Actually, I think the spec is clear that respondWith(undefined) should be an error.  I wrote a spec issue anyway, though:

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

Although, now that I look at comment 1, I'm not sure what else could be done there.  The code has already attempted a fetch(event.request).

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Oh, I think the service-worker.js may be using a chrome-specific promise extension.  Its doing Promise.all().concat().

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Gah.  I can't read.  The .concat() is inside the Promise.all().  I'll go drink more coffee before trying to "help" more here.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to oliverjash from comment #3)
> Hi, I am the author of this piece of work. I would like to help getting it
> to work in Firefox.
> 
> Do you know why `doesRequestAcceptHtml` returns false? I only want to serve
> the offline page if the request accepts HTML.

Oliver, thanks for your help.  I think we're (or at least I am) still confused about whats failing, though.  I think its reasonable for your code to do what its doing in comment 1.

We'll keep looking.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Its possible we are killing the service worker before it can update the cache.  The update is done outside of the respondWith().

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #8)
> Its possible we are killing the service worker before it can update the
> cache.  The update is done outside of the respondWith().

This isn't our main problem.  I checked in my profile and the cache has all the resources stored.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Ehsan debugged more and discovered we are throwing in the FetchEvent handler.  This is because of this code:

    var doesRequestAcceptHtml = function (request) {
        return request.headers.get('Accept')
            .split(',')
            .some(function (type) { return type === 'text/html'; });
    };

It appears we are setting the 'Accept' header further down in the network code after interception.

Per step 3 of Fetching, however, we should set both Accept and Accept-Language before interception:

  https://fetch.spec.whatwg.org/#fetching

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Apparently we never did this correctly in e10s mode, but we regressed it in non-e10s in bug 1200337.

We need to set some standard headers after interception and some before interception.

Comment 12

[(no longer active)]

We had a lengthy discussion over how to fix this on IRC.  We agreed on exposing whatever the Accept header is set to by Necko (or the consumer).  Also it seems that fetch() isn't supposed to send an Accept header at all.  I filed https://github.com/whatwg/fetch/issues/142 to fix that.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose Accept and Accept-Language to service worker FetchEvent.request. r=mcmanus

It turns out we hid too many headers from the SW in bug 1200337.  We still need to expose the Accept and Accept-Language headers.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Send */* for fetch() default Accept header. r=ehsan

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Check Accept header visibility in service worker fetch event. r=ehsan

Comment 16

[(no longer active)]

Comment on attachment 8678270 [details] [diff] [review]
P2 Send */* for fetch() default Accept header. r=ehsan

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

r=me pending resolution to https://github.com/whatwg/fetch/issues/142.

::: dom/fetch/FetchDriver.cpp
@@ +306,2 @@
>      for (uint32_t i = 0; i < headers.Length(); ++i) {
> +      if (headers[i].mName == NS_LITERAL_CSTRING("accept")) {

Nit: please prepend |!hasAccept &&| to avoid string comparisons once we have found one header.

Comment 17

[(no longer active)]

Comment on attachment 8678271 [details] [diff] [review]
P3 Check Accept header visibility in service worker fetch event. r=ehsan

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

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-header-visibility-iframe.html
@@ +40,5 @@
> +  // Check for custom accept header
> +  fetch(uri, { headers: headers }).then(function(response) {
> +    return response.text();
> +  }).then(function(text) {
> +    dump('### ### custom accept ' + text + '\n');

Nit: debugging left-over

@@ +55,5 @@
> +  // Check for default accept header
> +  fetch(uri).then(function(response) {
> +    return response.text();
> +  }).then(function(text) {
> +    dump('### ### default accept ' + text + '\n');

Here too.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #16)
> Comment on attachment 8678270 [details] [diff] [review]
> P2 Send */* for fetch() default Accept header. r=ehsan
> 
> r=me pending resolution to https://github.com/whatwg/fetch/issues/142.

I don't think we should wait for the spec issue. Our current value is wrong no matter what.  I have a separate bug filed to possibly remove the header.

Comment 19

[(no longer active)]

Sure.  I just said that on the issue: <https://github.com/whatwg/fetch/issues/142#issuecomment-150969972>

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #16)
> >      for (uint32_t i = 0; i < headers.Length(); ++i) {
> > +      if (headers[i].mName == NS_LITERAL_CSTRING("accept")) {
> 
> Nit: please prepend |!hasAccept &&| to avoid string comparisons once we have
> found one header.

I'm also going to use .EqualsLiteral() here.  Thanks.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c7e876a69e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/86ac4feb0e38
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00f079f712b

Comment 22

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

This appears to have introduced a leak in mochitest-4: https://treeherder.mozilla.org/logviewer.html#?job_id=16268129&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c6cc9d610865

Comment 23

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

And on Windows, it seems to be failing like https://treeherder.mozilla.org/logviewer.html#?job_id=16268144&repo=mozilla-inbound

Comment 24

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I filed bug 1218496 for detecting leaks of ErrorResult::Message in normal debug builds.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

The error in comment 23 seems more related to the RequestCache change.  I don't see how changing the Accept header would trigger this.  Ehsan, do you have any ideas?

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Err, never mind.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

We need to fill the headers before setting the header guard in Cache API.

Comment 28

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/6c7c983bce46

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Fill headers before setting guard when reading response from Cache. r=ehsan

Someone shocked we hadn't hit this sooner.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2087abfb78b2

Comment 30

[(no longer active)]

Comment on attachment 8679190 [details] [diff] [review]
P4 Fill headers before setting guard when reading response from Cache. r=ehsan

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

Whoa!  Do you know how this ever worked?

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #30)
> Whoa!  Do you know how this ever worked?

No iterable headers makes it hard to test that a header isn't lost.

Comment 32

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #31)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #30)
> > Whoa!  Do you know how this ever worked?
> 
> No iterable headers makes it hard to test that a header isn't lost.

Fair enough.  Now that bug 1108181 is fixed, do we have tests that would have caught this?  If not, we should add them in a follow-up.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #32)
> Fair enough.  Now that bug 1108181 is fixed, do we have tests that would
> have caught this?  If not, we should add them in a follow-up.

The wpt tests do a little header asserting, but I guess they didn't catch this.

We have a chrome-only way to set the header guard, so we should be able to write a mochitest, though.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Relax guard checking on Headers with existing headers. r=ehsan

Try shows that we ran into some restrictive checks in InternalHeaders with P4.  We should loosen them as they are not required by the spec.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22c74aa09df6

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/778ad533a09d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8a47465467f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b421b2e429a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aabfb4e5bd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1829e3855b61

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/778ad533a09d
https://hg.mozilla.org/mozilla-central/rev/e8a47465467f
https://hg.mozilla.org/mozilla-central/rev/6b421b2e429a
https://hg.mozilla.org/mozilla-central/rev/1aabfb4e5bd4
https://hg.mozilla.org/mozilla-central/rev/1829e3855b61