ServiceWorker tests time out with SpiderMonkey Promise implementation

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: till, Assigned: bkelly)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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)

Updated

a year ago
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(Assignee)

Comment 1

a year ago
Created attachment 8760303 [details] [diff] [review]
Hold service worker alive until event dispatching code is notified. r=asuth

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)
(Reporter)

Comment 2

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=caedb31ba6a8
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+
(Assignee)

Comment 4

a year ago
(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.
(Assignee)

Comment 5

a year ago
Try looks fairly green.  It appears to have passed the failing tests in the SM promise try as well.  Going to land.

Comment 6

a year ago
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
(Reporter)

Comment 7

a year ago
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+

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4fd4188b0bff
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.