1301966 - Intermittent dom/workers/test/serviceworkers/test_fetch_integrity.html | uncaught exception - SyntaxError: missing ; before statement at http://mochi.test:8888/tests/dom/workers/test/serviceworkers/sharedWorker_fetch.js:1:0

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=35665177&repo=mozilla-inbound

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/1473580756/mozilla-inbound_win7_vm_test-mochitest-e10s-4-bm137-tests1-windows-build24.txt.gz

How do you even?

Comment 1

[Josh Matthews [:jdm]]

I'm pretty sure that this is caused by shraredWorker_fetch.js getting intercepted by the service worker and getting the "Hello world" contents instead of the actual file.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Tom, can you take a look at this?  See Josh's theory in comment 1.

Comment 3

[Tom Tung [:tt, :ttung]]

Sure.

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: [WIP] Prevent serviceWorker to modify response for sharedWorker in different async tests.

Thanks for josh's comment. That help me figure out this bug quickly.

I guess the reason is that I expected the async tests in test_fetch_integrity.html should be run in independently. The sharedWorker's fetch should not be intercepted by serviceWorker(fetch.js) but I was wrong. 

The solution is just not to handle general cases in serviceWorker and I cannot reproduce this bug by rr chaos mode locally after applying this patch. I'll send a review request after passing try.

try result for patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e22900e8c8c1

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: [WIPv2] Prevent serviceWorker to modify response for sharedWorker in different async tests.

Hi Ben,

I find out the first error is happened as josh's comment but I expect that fetch event should not be intercepted by a unregistered serviceworker(in first test). 

Besides, I also find out, in sometimes, message event will be intercepted by a unregistered serviceworker. The other two errors are happened because of that. I don't know how to do so I add three if conditions to unregister serviceworker again and add a todo for each of them. I guess the service worker's mPendingUnstall is set though it call unregister() and somehow listen events when events is coming. 

Current this patch prevent these three errors from happening again (at least in my local machine with rr chaos mode) but it doesn't fix the reason that causing this bug.

Could you give me some hints for them? Thanks!

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8791091 [details] [diff] [review]
[WIPv2] Prevent serviceWorker to modify response for sharedWorker in different async tests.

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

::: dom/workers/test/serviceworkers/test_fetch_integrity.html
@@ +105,5 @@
>    info("Attch main window to a SharedWorker.");
>    let sharedWorker = new SharedWorker(filename);
>    let waitForConnected = new Promise((resolve) => {
>      sharedWorker.port.onmessage = function (e) {
> +      if (SpecialPowers.isServiceWorkerRegistered()) {

Can you add debug to see which registration it is?  Does it have its pending uninstall flag set?

Also, where is a test failure that shows the messages you are talking about?  Comment 4 try doesn't seem to have a failure for this test AFAICT.

Comment 7

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #6)
> Can you add debug to see which registration it is?  Does it have its pending
> uninstall flag set?
> 
> Also, where is a test failure that shows the messages you are talking about?
> Comment 4 try doesn't seem to have a failure for this test AFAICT.

Sorry for forgetting to post the error messages and provide enough information. I found those error by running rr chaos mode until failure happening in my computer but I cannot reproduce them by re-running try few times. And, I cannot access that computer right now. :( 

I will provide more information and answer your questions after the moon festival(holiday in Taiwan).

Comment 8

[Tom Tung [:tt, :ttung]]

Sorry for the late reply. It was holidays and I spent too much time on getting much familiar with rr and gdb.

> Can you add debug to see which registration it is?  Does it have its pending
> uninstall flag set?

We will call ServiceWorkerManager::Register()[1] twice and both of them are in content process in the first task, which is for serviceworker. The first time happens in [2] and it is reasonable. The second time happens after flushing the reports but I don't know why. 

I check the call stack for the ServiceWorkerManager::Register() in the second time it is called and I found that it is called from webidl. However I cannot find another serviceWorker.register().
Should we have the second registrations after flushing reports even I didn't call navigator.serviceWorker.register twice? Could you give me some hints of this, Ben? 

The intermittent happens when pending uninstall flag is unset after unregister() is called.

In normal cases, the pending uninstall flag is set due to ServiceWorkerUnregisterJob::Unregister()[3] and we will get no registration from ServiceWorkerManager::GetAllRegistrations() [4] (it is called at the end mochitest).

In unexpected failure cases, after ServiceWorkerManager::Register() is called in second, somehow ServiceWorkerUnregisterJob::Unregister() is executed faster than ServiceWorkerRegistraterJob::AsyncExecute() [5] so the pending uninstall flag is unset then we get a unexpected registration without clean it up at the end of this test [4].  
I guess we need to prevent that [5] is called slower than [3] but I don't know how to do? Could you give me some hints about this, too? Thanks!

[1] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#491
[2] http://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/test_fetch_integrity.html#56
[3] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerUnregisterJob.cpp#135
[4] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#3170
[5] http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerRegisterJob.cpp#46

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

We explicitly only allow one job to run at a time.  So the UnregisterJob must complete before the next RegisterJob can run.

You said you can see that the second register() is coming from webidl.  You can get the js stack trace as well by using:

  DumpJSStack()

from gdb.  Can you see what javascript is calling that?

Comment 10

[Tom Tung [:tt, :ttung]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> We explicitly only allow one job to run at a time.  So the UnregisterJob
> must complete before the next RegisterJob can run.
> 
> You said you can see that the second register() is coming from webidl.  You
> can get the js stack trace as well by using:
> 
>   DumpJSStack()
> 
> from gdb.  Can you see what javascript is calling that?

Oh, it's useful! Thanks! It helps me to find out the stupid mistake I made.

I found out the second register() happens in serviceworker.html[1] and I didn't notice that it do the registration without doing unregister. That explains why I saw two registration and the failure happens.

[1] http://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/serviceworker.html#6

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Prevent serviceWorker to modify response for sharedWorker and remove the second unexpected registration for the serviceWorker test.

Hi Ben,

In this patch, to avoid serviceWorker to modify the sharedWorker's response to "Hello World", I remove the code for it in serviceWorker's fetch event listener. Besides, I also add another file(hello.html), which only prints "Hello", to replace serviceworker.html, which do the register() without unregister().

Could you help me to review this patch? Thanks!

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

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug-1301966 - Prevent serviceWorker to modify response for sharedWorker and remove the second unexpected registration for the serviceWorker test. r=bkelly.

Thanks for your review and your time, Ben!

Rebased the patch for adding newline at end of create_another_sharedWorker.html and removing extra line for sharedWorker_fetch.js.

Comment 13

[Tom Tung [:tt, :ttung]]

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/baef41d6504c
Prevent serviceWorker to modify response for sharedWorker and remove the second unexpected registration for the serviceWorker test. r=bkelly.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/baef41d6504c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dac2d9a70f6d