serviceWorker.controller is null on reload with disabled caches

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bdahl, Unassigned)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
Posted patch cache.patch (obsolete) — Splinter Review
(Reporter)

Comment 2

4 years ago
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?
Attachment #8658416 - Flags: feedback?(bkelly)
(Reporter)

Comment 3

4 years ago
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
(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 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.
Attachment #8658416 - Flags: feedback?(bkelly) → feedback+
(Reporter)

Comment 6

4 years ago
Posted patch cache.patch (obsolete) — Splinter Review
Attachment #8658416 - Attachment is obsolete: true
Attachment #8660843 - Flags: review?(bugs)
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.
Attachment #8660843 - Flags: review?(bugs) → review-
Ah, bkelly had commented here.
Maybe he wants to comment some more :)
Flags: needinfo?(bkelly)
(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
Flags: needinfo?(bkelly)
ok, thanks. Then that null check should be enough, I think
(Reporter)

Comment 12

4 years ago
Posted patch cache.patch (obsolete) — Splinter Review
Adds check for mScriptGlobalObject and also the channel.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f06e54c50818
Attachment #8660843 - Attachment is obsolete: true
Attachment #8661861 - Flags: review?(bkelly)
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.
Attachment #8661861 - Flags: review?(bkelly) → review+
(Reporter)

Comment 14

4 years ago
Posted patch cache.patchSplinter Review
Addresses nit and carries r+ forward.
Attachment #8661861 - Attachment is obsolete: true
Attachment #8662080 - Flags: review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2237e1a53be9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.