Closed Bug 1436763 Opened 6 years ago Closed 6 years ago

fix detached-context.https.html to not time out

Categories

(Core :: DOM: Service Workers, enhancement, P2)

Product:
Core
Component:
DOM: Service Workers
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Fix detached-context.https.html to not timeout due to promise reaction callbacks not firing for detached globals. r=catalinb
10.78 KB, patch
bkelly
: review+
Details | Diff | Splinter Review 
The detached-context.https.html tries to verify that calling unregister() or update() on a ServiceWorkerRegistration from a dead global reject.  In firefox, though, we do not fire promise callbacks at all in this situation.

Google indicates they are ok with loosening this test check here:

https://github.com/w3c/ServiceWorker/issues/1221#issuecomment-364098533

I'm going to fix this because bug 1434701 makes us pass more conditions here and ultimately start timing out on the promise check.
Catalin, this fixes a problem in the detached-context.https.html test where it was expecting some promises owned by a detached global to reject.  See this for context:

https://github.com/w3c/ServiceWorker/issues/1221#issuecomment-363515314

I changed the test to simply verify that the promises don't resolve, but not require rejecting.  Rather than use a `setTimeout()` delay to wait for the "resolve to not happen", I steal the helper code from activation.https.html to wait the amount of time it takes for a service worker on a dummy scope to install, activate, and unregister.
Attachment #8949425 - Flags: review?(catalin.badea392)
Priority: -- → P2
Comment on attachment 8949425 [details] [diff] [review]
Fix detached-context.https.html to not timeout due to promise reaction callbacks not firing for detached globals. r=catalinb

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

lgtm
Attachment #8949425 - Flags: review?(catalin.badea392) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cda1f013a78a
Fix detached-context.https.html to not timeout due to promise reaction callbacks not firing for detached globals. r=catalinb
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9485 for changes under testing/web-platform/tests
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a16ba1ee8371
P2 Update wpt lint.whitelist to account for wait_for_activation_on_dummy_scope() moving to a new file. r=me
Upstream web-platform-tests status error for continuous-integration/travis-ci/pr. This will block the upstream PR from merging. See https://travis-ci.org/w3c/web-platform-tests/builds/340620106?utm_source=github_status&utm_medium=notification for more information
Update for linting fix.  I think I figured out how to run it locally, but here's a try run as well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22a5c94b4a70d63ac3be60d25358bb57cdc88ab7
Attachment #8949425 - Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8950338 - Flags: review+
Ha, the linting is failing because I wrote a comment about *not* using setTimeout().
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b32778925cf
Backed out changeset a16ba1ee8371 linting failure at builds/worker/checkouts/gecko/tools/lint/wpt.yml:None
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8d461ef1437
Fix detached-context.https.html to not timeout due to promise reaction callbacks not firing for detached globals. r=catalinb
https://hg.mozilla.org/mozilla-central/rev/cda1f013a78a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Can't merge web-platform-tests PR due to failing upstream tests
James, did our backout and reland confuse the wpt bot?  On my mobile, so not sure what the upstream failure is at the moment.
Flags: needinfo?(james)
Can't merge web-platform-tests PR due to failing upstream tests
Can't merge web-platform-tests PR due to failing upstream tests
The upstream PR still doesn't reflect the actual code that landed in m-c.  It still has the comment that reference setTimeout() which triggers the linting failure.  I don't know how to fix it.
OK, so what happened here is there was a stupid bug in the sync code that meant that even though it knew  how to handle backouts correctly it would open a new sync for each relanding. Then I *think* there was abotched attempt to fix it by hand owning to not really having worked out that there were two syncs. In any case, I think I've now fixed it by hand, and the original bug is fixed. But it's *possible* that there's still a bug and we aren't removing all previous commits after a backout in all cases (there's a test covering that but who knows if reality is slightly, but importantly, different). So I'll make sure this lands but please let me know if there's another similar issue after a backout in the future.
Flags: needinfo?(james)
OK, that was just us hitting the timeout limits. The duplicate notifications were supposed to be fixed, but apparently aren't :(
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: