Closed Bug 1301966 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: tt)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 3 obsolete files)

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.
Tom, can you take a look at this?  See Josh's theory in comment 1.
Flags: needinfo?(ttung)
Sure.
Assignee: nobody → ttung
Flags: needinfo?(ttung)
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
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
Attachment #8790574 - Attachment is obsolete: true
Attachment #8791091 - Flags: feedback?(bkelly)
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.
Attachment #8791091 - Flags: feedback?(bkelly) → feedback-
(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).
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
Flags: needinfo?(bkelly)
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?
Flags: needinfo?(bkelly) → needinfo?(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
Flags: needinfo?(ttung)
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
Attachment #8791091 - Attachment is obsolete: true
Attachment #8793144 - Flags: review?(bkelly)
Attachment #8793144 - Flags: review?(bkelly) → review+
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.
Attachment #8793144 - Attachment is obsolete: true
Pushed by cbook@mozilla.com:
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/baef41d6504c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.