1047663 - Disable Cache doesn't disable cache for web workers, nothing does

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P1)

Description

[:Harald Kirschner :digitarald]

Caching for a web worker is an uncontrollable beast, even hard refresh doesn't invalidate the web worker file after making changes.

Steps:
- Use a web worker in your site
- Make a change (change a console log or fix an error)
- Disable Cache in devtools settings and/or Hard Refresh

Expected behavior:
Change shows up.

Actual result:
Even after various ways of refreshing the changed file never gets picked up.

This impacts developers the most but might also impact users that execute outdated web workers when developers already pushed a new version.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Disable cache only currently disables the cache until a page's load event is fired.

We want to add a flag to do that in bug 1027579.

Comment 2

[(not reading bugmail) Nick Desaulniers [:\n]]

There's definitely some bizarre behavior going on here.  After some confusion, I found out that the browser was used a cached version of a script imported into a web worker.  The browser was not making requests to my server for the asset.  I had to navigate to the url of the asset (after deleting it on the server) to trigger a cache bust.  After a restart, I'm not seeing this behavior (yet, possibly).  Harald also mentioned that private browsing seemed to help, but I haven't confirmed since I can't reproduce again.  Will post if I see it again.

Comment 3

[(not reading bugmail) Nick Desaulniers [:\n]]

Bump, this issue is super frustrating.  Whatever is being passed to importScripts is being cached.  I have to kill the server, then enter the URL passed to importScripts to bust the cache.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Are web workers behaving differently than XHR?  I don't think we're doing any special caching.  We're just relying on Necko.

Comment 5

[Honza Bambas (:mayhemer)]

This really won't be in Necko.  The problem definitely is in sending down to http channels proper flags.

Putting back to DOM: Workers.  If any help needed on the Necko side with diagnosing this, feel free to ni? me.

Comment 6

[Kumar Harsh]

@Nick: I think you can just use "Ctrl+Shift+Del" to clear your cache. That works (atleast it does for other resources like XHR) instead of restarting your server...

Agree, this bug is super-frustrating.

Comment 7

[jweathersby]

Bump - +1 on frustrating

Comment 8

[Eddy Bruel [:ejpbruel]]

Marking this as P1. This is one of those basic features that should just work, and make the debugging experience frustrating when it doesn't.

Comment 9

[Eddy Bruel [:ejpbruel]]

I did a quick investigation of ScriptLoader.cpp. Looks like this is where we set the load flag for the channel that will load the script:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?case=true&from=ScriptLoader.cpp%3A890#890

I'd like to confirm this with Kyle before I start writing a patch, but it seems to me that this should be as simple as adding the same flags we use here when we want caching to be disabled:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1516

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Answered on IRC ("yes")

Comment 11

[Eddy Bruel [:ejpbruel]]

Attachment: WIP

Here's a WIP patch that copies the load flags from the doc shell for dedicated workers. I've ignored shared workers for now, but the most sensible solution there seems to be to use a pref instead. Kyle, do you agree?

Two additional questions: apparently, a top-level dedicated worker can have a null window. How is this possible?

What would be a good way to test that caching is indeed disabled for a worker? Can we somehow change the contents of a cached file during a mochitest?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Eddy Bruel [:ejpbruel] from comment #11)
> Created attachment 8726750 [details] [diff] [review]
> WIP
> 
> Here's a WIP patch that copies the load flags from the doc shell for
> dedicated workers. I've ignored shared workers for now, but the most
> sensible solution there seems to be to use a pref instead. Kyle, do you
> agree?

I don't know about using a pref for SharedWorker, but I do agree that ignoring them is best for now.

> Two additional questions: apparently, a top-level dedicated worker can have
> a null window. How is this possible?

If it's created by code that lives outside a window.  I expect the OS.File worker thread will have a null window.

> What would be a good way to test that caching is indeed disabled for a
> worker? Can we somehow change the contents of a cached file during a
> mochitest?

I think you should be able to do a .sjs that returns a different response the second.  Be sure to test it both and without the fix (we may need to set caching headers/etc, I don't actually know how caching works :P) to make sure that the test works.

Comment 13

[Eddy Bruel [:ejpbruel]]

Attachment: Test for caching in workers

I've been working on a test for this, but I can't get caching to trigger.

The test does the following:
- Create a worker (from a sjs file)
- Post a message to the worker, and check the reply
- Terminate the worker
- Create a second worker (from the same sjs file)
- Post a message to the worker, and check the reply
- Terminate the worker

The sjs file is set up so that it returns a different file between the first and subsequent get requests, so without caching, I expect both workers to reply with a different message, even though they are loaded from the same sjs file.

However, note that the point of this bug was that caching is always enabled for workers, so the expected behavior here is that each worker sends the same reply, even though the contents of the file have changed. That's not what happens on my machine, however. The test behaves as if caching is disabled.

Based on comment 2, the browser should not even attempt to make a new network request for the second worker, but I can confirm that the sjs file receives a request for both.

Is there anything else I need to do here for caching to trigger? Do I need to set some HTTP headers in the sjs file? Is setting the load flags to LOAD_NORMAL in ScriptLoader.cpp enough for caching to trigger? Is it possible for the browser to make a network request even if caching is enabled?

Some help from the necko folks would be appreciated here.

Comment 14

[Eddy Bruel [:ejpbruel]]

Harald, does the behavior you describe also trigger when you create and terminate a worker, then recreate it with the same file after changing its contents? Or does it only trigger when you refresh the tab? I'm having trouble reproducing the issue, as you can see in comment 13.

Comment 15

[Honza Bambas (:mayhemer)]

Just add bug1047663_worker.sjs^headers^ file with the following content:

Cache-control: max-age=3600

This will make HTTP cache reuse bug1047663_worker.sjs w/o revalidation for 60 minutes.

Comment 16

[Honza Bambas (:mayhemer)]

What parts of the patch do you want me to feedback?  I'm not familiar with the code pieces in the patch.

Comment 17

[Eddy Bruel [:ejpbruel]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> What parts of the patch do you want me to feedback?  I'm not familiar with
> the code pieces in the patch.

The original bug report was that the cache always triggers, and can not be disabled, when a worker is loaded in a tab that is refreshed, or when importing a script in a worker. Both these mechanisms use the ScriptLoader to load their scripts.

As far as I can tell, within the ScriptLoader, we set the load flags for each request here:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?case=true&from=ScriptLoader.cpp#893

What I'd like to know is whether LOAD_NORMAL should always trigger the cache, and if so, why caching does not seem to happen in the test that I wrote.

Comment 18

[Eddy Bruel [:ejpbruel]]

Jason said he'd take a closer look at this.

Comment 19

[Eddy Bruel [:ejpbruel]]

Per Jason's request, I did some debugging.

When the first worker is loaded, we eventually end up at
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?from=ScriptLoader.cpp#165

This is right before the call to NS_NewChannel. At this point, the value of aLoadFlags is 4194304. This is the value of nsIChannel::LOAD_CLASSIFY_URI, which is set here:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp?from=ScriptLoader.cpp#133

As far as I can ascertain, this is the only load flag that is set when we create the channel. The same set of load flags is use for the second worker.

Since this is the only flag that is set, caching *should* be enabled for this channel, correct? I did add the header like Honza suggested in comment 14, but the cache still doesn't seem to kick in (i.e. the second worker has different contents).

Note that the original bug report mentions that the script is cached when refreshing a tab that contains a worker, or importing a script from a worker. My test doesn't *quite* test the same thing: instead of refreshing the tab, it creates two workers with the same url. To the best of my knowledge, this shouldn't matter, since in both cases this creates two workers, using the same code in ScriptLoader. However, I'm mentioning it here because it might be relevant somehow.

My main question at this point is why caching doesn't kick in with my test. The most likely scenario is that my test is broken somehow.

Another option is that caching actually never happened here (but why not?), and the caching behavior that the bug reporter sees is caused by the debugger requesting the source for the worker via a separate network request (which shouldn't happen) and caching that (which also shouldn't happen). That's something I can look into.

In the meantime, any help with this test would be appreciated!

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: patch-work.txt

Sorry... forgot ^headers^ files don't work for sjs.

Comment 21

[Eddy Bruel [:ejpbruel]]

(In reply to Honza Bambas (:mayhemer) from comment #20)
> Created attachment 8729011 [details] [diff] [review]
> patch-work.txt
> 
> Sorry... forgot ^headers^ files don't work for sjs.

Okay then. What does?

Comment 22

[Honza Bambas (:mayhemer)]

Look into the patch! :)

Comment 23

[Eddy Bruel [:ejpbruel]]

(In reply to Honza Bambas (:mayhemer) from comment #22)
> Look into the patch! :)

Going to try that now, thanks Honza!

Comment 24

[Eddy Bruel [:ejpbruel]]

(In reply to Eddy Bruel [:ejpbruel] from comment #23)
> (In reply to Honza Bambas (:mayhemer) from comment #22)
> > Look into the patch! :)
> 
> Going to try that now, thanks Honza!

That did the trick! I can get the cache to trigger, and make sure it doesn't trigger when it's disabled in the tab that contains the worker. Should be able to put up something for review soon.

Thanks for your help on this, Honza and Jason. I appreciate your quick replies!

Comment 25

[Honza Bambas (:mayhemer)]

Do you still need f? on your patch from me?

Comment 26

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8728338 [details] [diff] [review]
Test for caching in workers

Nope! All good!

Comment 27

[Eddy Bruel [:ejpbruel]]

Attachment: Bug 1047663 - Disabling the cache in a tab should also disable it for all workers in that tab.

Took me a while to come up with a good way to test this. I had to use a browser mochitest for this, because chrome mochitests don't support server side Javascript, apparently.

Anyway, most of the complexity is in the test. The patch itself is actually rather simple.

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8729034 [details] [diff] [review]
Bug 1047663 - Disabling the cache in a tab should also disable it for all workers in that tab.

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

::: dom/workers/ScriptLoader.cpp
@@ +893,5 @@
>  
>      nsLoadFlags loadFlags = nsIRequest::LOAD_NORMAL;
>  
> +    // Get the top-level worker.
> +    WorkerPrivate* workerPrivate = mWorkerPrivate;

nit: please name this topWorkerPrivate or something so that we don't later confuse it with mWorkerPrivate.

::: dom/workers/test/browser.ini
@@ +1,5 @@
>  [DEFAULT]
> +support-files =
> +  bug1047663_tab.html
> +  bug1047663_worker.sjs
> +  bug1047663_worker.sjs^headers^

You didn't add the ^headers^ to the patch.

Comment 29

[Eddy Bruel [:ejpbruel]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #28)
> Comment on attachment 8729034 [details] [diff] [review]
> Bug 1047663 - Disabling the cache in a tab should also disable it for all
> workers in that tab.
> 
> Review of attachment 8729034 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ScriptLoader.cpp
> @@ +893,5 @@
> >  
> >      nsLoadFlags loadFlags = nsIRequest::LOAD_NORMAL;
> >  
> > +    // Get the top-level worker.
> > +    WorkerPrivate* workerPrivate = mWorkerPrivate;
> 
> nit: please name this topWorkerPrivate or something so that we don't later
> confuse it with mWorkerPrivate.
> 
> ::: dom/workers/test/browser.ini
> @@ +1,5 @@
> >  [DEFAULT]
> > +support-files =
> > +  bug1047663_tab.html
> > +  bug1047663_worker.sjs
> > +  bug1047663_worker.sjs^headers^
> 
> You didn't add the ^headers^ to the patch.

Thanks for the quick review!

See comment 20. ^headers^ doesnt work for sjs files, so I had to add the header in the sjs file itself.

Comment 30

[Eddy Bruel [:ejpbruel]]

Try push for this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c19d5d0f4d

Comment 31

[Eddy Bruel [:ejpbruel]]

I left the ^headers^ file in the browser.ini file, which is probably what khuey tried to warn me for :-(

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3249f57d6ef5

Comment 32

[Eddy Bruel [:ejpbruel]]

More try failures. Apparently, not every window with workers has a docshell.

I changed the patch to check if a (instead of assert that) docshell is present on the window. Here's a try run for the update patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07a38fc7d7dc

Before I land this, I'd like to double check if it actually makes sense for workers to be created inside a window that has no docshell. Kyle, can you confirm this?

Comment 33

[Eddy Bruel [:ejpbruel]]

Ugh, forgot to amend the patch before pushing it to try. Cancelled that try run and created a new one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbdd17e6f48f

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Seems a little odd, can you figure out what script we're running in that worker (off the WorkerPrivate)?

Comment 35

[Eddy Bruel [:ejpbruel]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #34)
> Seems a little odd, can you figure out what script we're running in that
> worker (off the WorkerPrivate)?

The test that triggers the assertion tries to add a tab with a pdf file, which causes pdf.js to be loaded. The script that we're trying to load in ScriptLoader.cpp is ../build/pdf.worker.js, so that checks out.

Is it possible that pdf.worker.js does something special that causes it to load a worker in a window that doesn't have a docshell?

Comment 36

[Eddy Bruel [:ejpbruel]]

Boris, perhaps you can shed some light on this. I assumed that every window that has a worker must also have a docshell but the try run in comment 31 shows at least one test for which this isn't the case. Apparently the ScriptLoader tries to load pdf.worker.js in a worker whose top-level worker is owned by a window without a docshell.

I can easily work around this by checking if the docshell property is not null, but I'd like to make sure that this actually makes sense.

Comment 37

[Boris Zbarsky [:bzbarsky]]

I don't know offhand.  In general, window with no docshell means that the relevant frame element was removed from the DOM or so.  What I don't know is what happens with workers in that situation.  I tried this testcase:

  <iframe></iframe>
  <script>
  onload = function() {
    var script = "onmessage = function() { importScripts('data:application/javascript,postMessage(5)') }"
    var url = URL.createObjectURL(new Blob([script]));
    var w = new frames[0].Worker(url);
    w.onmessage = function(e) { alert(e.data); }
    w.postMessage("go");
    document.querySelector("iframe").remove();
  }
  </script>

and the message is never received.  But that tells me nothing about whether the load happened and then the posting back of the message was just suppressed.  In general, I don't know enough about worker canceling/shutdown to tell you whether in cases like this we would race and end up doing the load anyway or what.

Not doing the load if there is a window but no docshell does make some sense to me, if the actual results of that load would not be observable anyway (this would need to be confirmed).

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, knowing that it's pdf.js I'm less worried.  Add the null check and land.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4087892d7da8

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4087892d7da8