1202895 - serviceWorker.controller is null on reload with disabled caches

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Brendan Dahl [:bdahl]]

If the check box in devtools "Disable Cache (when toolbox is open)" is checked the serviceWorker.controller is null on page reloads (not a force-reload).

STR:
1) Disable caches in the devtools settings
2) Load a page with service worker
3) Refresh the page

Expected:
navigator.serviceWorker.controller is the active service worker

Actual:
navigator.serviceWorker.controller is null

Comment 1

[Brendan Dahl [:bdahl]]

Attachment: cache.patch

Comment 2

[Brendan Dahl [:bdahl]]

Comment on attachment 8658416 [details] [diff] [review]
cache.patch

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

I started writing a test case for this thinking I could reproduce the issue with http no-cache headers, but it only happens when devtools is open and the above option is selected. bkelly: Do you have any thoughts on how to write a test for this? Or if we should even bother making a test for it?

Comment 3

[Brendan Dahl [:bdahl]]

Some more info that was requested: This is broken in both e10s and non-e10s. The relevant dev tools code that disables the cache http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#1463

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Brendan Dahl [:bdahl] from comment #3)
> Some more info that was requested: This is broken in both e10s and non-e10s.
> The relevant dev tools code that disables the cache
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#1463

Sorry, I didn't realize you had already tracked it down to the nsDocument code.  I thought the patch was a proposed test.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8658416 [details] [diff] [review]
cache.patch

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

This looks right to me, but I think Ollie would prefer if we unified the definition of "shift-reload" in docshell.

We do have a shift-reload test already:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/browser_force_refresh.js?offset=200

To be honest, I'm not sure how to write a devtools test to verify the usage of "disable cache" checkbox.  It seems to adjust the load flags directly.  I guess maybe you could make a browser chrome test that creates a channel directly with LOAD_BYPASS_CACHE to see if its intercepted.

Thanks for your help with this!

::: dom/base/nsDocument.cpp
@@ +4745,5 @@
>      // If we are shift-reloaded, don't associate with a ServiceWorker.
> +    if (loadType == LOAD_RELOAD_BYPASS_CACHE ||
> +        loadType == LOAD_RELOAD_BYPASS_PROXY ||
> +        loadType == LOAD_RELOAD_BYPASS_PROXY_AND_CACHE ||
> +        loadType == LOAD_RELOAD_ALLOW_MIXED_CONTENT) {

Can you create a helper routine on nsDocShell that wraps this check up?  Something like nsDocShell::IsForcedRelad()?

Ollie should probably review it.

Comment 6

[Brendan Dahl [:bdahl]]

Attachment: cache.patch

Comment 7

[Brendan Dahl [:bdahl]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12c9d114b193

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8660843 [details] [diff] [review]
cache.patch

I'm not familiar enough with service workers that I could say whether using docshell's loadtype and check that here is the right thing.

I don't understand the old code in nsDocument either.
Why do we possibly call MaybeStartControlling, even if we don't have mScriptGlobalObject anymore (we're sort-of destroying the whole document.)
The patch does in fact make that even worse. Since docshell<->document relationship is one to many, some to-be-destroyed document checks here that docshell's loadtype is something and then calls MaybeStartControlling.
So at least we want to prevent MaybeStartControlling calls when 
mScriptGlobalObject is null.


r+ for the docshell part though, but the nsDocument part doesn't look right. Add that mScriptGlobalObject check.
and then something... I don't actually understand why we don't want to control the document if we're doing reload.
This clearly needs feedback from bkelly or ehsan or nsm or someone.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Ah, bkelly had commented here.
Maybe he wants to comment some more :)

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8660843 [details] [diff] [review]
> cache.patch
> 
> I'm not familiar enough with service workers that I could say whether using
> docshell's loadtype and check that here is the right thing.
> 
> I don't understand the old code in nsDocument either.
> Why do we possibly call MaybeStartControlling, even if we don't have
> mScriptGlobalObject anymore (we're sort-of destroying the whole document.)
> The patch does in fact make that even worse. Since docshell<->document
> relationship is one to many, some to-be-destroyed document checks here that
> docshell's loadtype is something and then calls MaybeStartControlling.
> So at least we want to prevent MaybeStartControlling calls when 
> mScriptGlobalObject is null.

I'm guessing that was just a bug.  Checking for null mScriptGlobalObject sounds reasonable to me.

> r+ for the docshell part though, but the nsDocument part doesn't look right.
> Add that mScriptGlobalObject check.
> and then something... I don't actually understand why we don't want to
> control the document if we're doing reload.
> This clearly needs feedback from bkelly or ehsan or nsm or someone.

The spec says to do it.  Step 12.1 here:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-fetch-request-algorithm

Force reload also affects the return value of navigator.serviceWorker:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#navigator-service-worker-controller

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

ok, thanks. Then that null check should be enough, I think

Comment 12

[Brendan Dahl [:bdahl]]

Attachment: cache.patch

Adds check for mScriptGlobalObject and also the channel.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f06e54c50818

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8661861 [details] [diff] [review]
cache.patch

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

::: dom/base/nsDocument.cpp
@@ +4731,5 @@
>      mTemplateContentsOwner->SetScriptGlobalObject(aScriptGlobalObject);
>    }
>  
>    nsCOMPtr<nsIChannel> channel = GetChannel();
> +  if (!mMaybeServiceWorkerControlled && mDocumentContainer && mScriptGlobalObject && channel) {

nit: We can avoid an AddRef() if you use GetChannel() directly in the boolean expression.

Comment 14

[Brendan Dahl [:bdahl]]

Attachment: cache.patch

Addresses nit and carries r+ forward.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2237e1a53be9

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/2237e1a53be9