Closed Bug 1278253 Opened 6 years ago Closed 6 years ago

ServiceWorker tests time out with SpiderMonkey Promise implementation

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: till, Assigned: bkelly)

References

Details

Attachments

(1 file)

bkelly's theory is that this is a race caused by dom.serviceWorkers.idle_timeout being set to 0 in tests.

Examples of test failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=807d47b26115&selectedJob=21934258
https://treeherder.mozilla.org/#/jobs?repo=try&revision=807d47b26115&selectedJob=21932025

IRC conversation about this:
16:07 <bkelly> till: this may be an issue with this pref setting: ["dom.serviceWorkers.idle_timeout", 0],
16:07 <bkelly> till: I wonder if its terminating the service worker before it can finish the registration
16:09 <till> bkelly: hmm, but why wouldn't that affect my local testing?
16:09 <bkelly> till: it would be a race...
16:09 <till> bkelly: or the DOM Promise implementation, for that matter
16:09 <till> ok, same answer
16:09 <bkelly> till: it could be considered a bug... but we don't ship that low of an idle timeout
16:09 <till> bkelly: but we use it in mochitests?
16:10 <bkelly> till: we also use it here: test_unresolved_fetch_interception.html
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
This patch just reorders things so our KeepAliveHandler is resolved after the handler provided by the code initiating the event.  This way we can make sure the worker is not terminated before the caller gets the correct success/failure status from running the event.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c7070ee1b0f
Attachment #8760303 - Flags: review?(bugmail)
Attachment #8760303 - Flags: feedback?(till)
Comment on attachment 8760303 [details] [diff] [review]
Hold service worker alive until event dispatching code is notified. r=asuth

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

Thanks for the excellent comment!  It warms my heart whenever I see a comment explaining why something is happening.

I sanity-checked the unchanged call-site (1), and confirm that SendPushSubscriptionChangeEventRunnable internally doesn't care what happens to its promise and its nullptr aWaitUntilPromise arg now works for aPromiseHandler because such things are untyped.  However, should you be doing "Unused <<" for that usage since the return value is uncheck, or is that idiom only for use in certain situations?
Attachment #8760303 - Flags: review?(bugmail) → review+
(In reply to Andrew Sutherland [:asuth] from comment #3)
> aPromiseHandler because such things are untyped.  However, should you be
> doing "Unused <<" for that usage since the return value is uncheck, or is
> that idiom only for use in certain situations?

I've only really seen "Unused <<" for cases where we have MOZ_MUST_USE static checking enabled for the return value.
Try looks fairly green.  It appears to have passed the failing tests in the SM promise try as well.  Going to land.
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd4188b0bff
Hold service worker alive until event dispatching code is notified. r=asuth
Comment on attachment 8760303 [details] [diff] [review]
Hold service worker alive until event dispatching code is notified. r=asuth

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

This already landed, but yes, I can confirm that it fixes the issue with SpiderMonkey Promises. Thanks for the quick work on this!
Attachment #8760303 - Flags: feedback?(till) → feedback+
https://hg.mozilla.org/mozilla-central/rev/4fd4188b0bff
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.