Closed
Bug 1456466
Opened 5 years ago
Closed 5 years ago
MozPromisify ServiceWorkerRegistration::Inner::Update()
Categories
(Core :: DOM: Service Workers, enhancement, P2)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(5 files, 19 obsolete files)
5.03 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
19.17 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
4.67 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
6.09 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
7.84 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
Split off some patches from bug 1455695 to land. These make Update() use a MozPromise internally.
Assignee | ||
Comment 1•5 years ago
|
||
Attachment #8970526 -
Flags: review+
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/89ce7df05344 P1 Expose a GetPrincipal() convenience method on service worker descriptor classes. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/e3a50afee79e P2 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0feb0058fc P3 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10602 for changes under testing/web-platform/tests
Assignee | ||
Updated•5 years ago
|
Attachment #8970527 -
Flags: review+
Assignee | ||
Updated•5 years ago
|
Attachment #8970528 -
Flags: review+
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Comment 7•5 years ago
|
||
Backed out 3 changesets (bug 1456466) for mochitest failures in /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/MozPromise.h on a CLOSED TREE Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fd0feb0058fc93361dc94df776a9fea43c0aab82&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=175339447 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175339447&repo=mozilla-inbound&lineNumber=13513 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a75639bd93809b0fb7d1be9393d18e7c85fd92
Flags: needinfo?(bkelly)
Upstream PR was closed without merging
Assignee | ||
Comment 9•5 years ago
|
||
Updated to make ~SWRUpdateRunnable() reject the promise if it never ran.
Attachment #8970527 -
Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8970625 -
Flags: review+
Assignee | ||
Comment 10•5 years ago
|
||
Attachment #8970528 -
Attachment is obsolete: true
Attachment #8970626 -
Flags: review+
Assignee | ||
Comment 11•5 years ago
|
||
Assignee | ||
Comment 12•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42251f6de6f25dbfc024b6b22ded9f9bfdb1eceb
Assignee | ||
Updated•5 years ago
|
Attachment #8970627 -
Attachment is obsolete: true
Updated•5 years ago
|
Priority: -- → P2
Assignee | ||
Comment 13•5 years ago
|
||
Attachment #8970625 -
Attachment is obsolete: true
Assignee | ||
Comment 14•5 years ago
|
||
Attachment #8970626 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #8970671 -
Flags: review+
Assignee | ||
Updated•5 years ago
|
Attachment #8970672 -
Flags: review+
Assignee | ||
Comment 15•5 years ago
|
||
Let's take another shot at CopyableErrorResult: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0596f9983a024c83796bbeb7b256d32ce8ae301b
Assignee | ||
Comment 16•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9b73ccf493b57be1e2b1a3bb76333a52380f026
Attachment #8970678 -
Attachment is obsolete: true
Assignee | ||
Comment 17•5 years ago
|
||
Assignee | ||
Comment 18•5 years ago
|
||
Attachment #8970688 -
Attachment is obsolete: true
Assignee | ||
Comment 19•5 years ago
|
||
Assignee | ||
Comment 20•5 years ago
|
||
This still trips some assertions, but its better.
Assignee | ||
Comment 21•5 years ago
|
||
Attachment #8970686 -
Attachment is obsolete: true
Assignee | ||
Comment 22•5 years ago
|
||
Attachment #8970709 -
Attachment is obsolete: true
Assignee | ||
Comment 23•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ce917ed8ad6bb82f5423a28aa3dac8d76f5ca0
Assignee | ||
Comment 24•5 years ago
|
||
Notes to self: 1. Figure out why test_workerUpdate.html is failing. 2. Check if the assertion trigger is due to not consuming already_AddRefed<> here: https://searchfox.org/mozilla-central/source/dom/workers/WorkerEventTarget.cpp#106 3. Consider calling NoteTerminated() on any status >= Terminated.
Assignee | ||
Comment 25•5 years ago
|
||
Attachment #8970708 -
Attachment is obsolete: true
Assignee | ||
Comment 26•5 years ago
|
||
Attachment #8970717 -
Attachment is obsolete: true
Assignee | ||
Comment 27•5 years ago
|
||
Assignee | ||
Comment 28•5 years ago
|
||
This makes things happy locally. Lets check try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5792cd2143a4735207e95ebae39faf8c186adb3d
Assignee | ||
Comment 29•5 years ago
|
||
Attachment #8970694 -
Attachment is obsolete: true
Assignee | ||
Comment 30•5 years ago
|
||
Attachment #8970911 -
Attachment is obsolete: true
Assignee | ||
Comment 31•5 years ago
|
||
Attachment #8970912 -
Attachment is obsolete: true
Assignee | ||
Comment 32•5 years ago
|
||
Attachment #8970913 -
Attachment is obsolete: true
Assignee | ||
Comment 33•5 years ago
|
||
Comment on attachment 8970923 [details] [diff] [review] P5 Add a DOMMozPromiseRequestHolder helper class to auto-disconnect MozPromise Thenables when the global dies. r=baku Currently its quite difficult to use MozPromise safely on worker threads. MozPromise objects assert that any Thenable has been fired or disconnected before its destroyed. Firing a thenable requires dispatching a runnable to the target thread. Disconnecting can only be done on the target thread. This means that if you try to wait for a WorkerRef shutdown callback to reject a MozPromise its already too late. The rejection runnable can fail to dispatch on the worker's HybridEventTarget since its shutting down. Disconnecting could take place at the Thenable based on a WorkerRef callback, but it would be annoying to require each and every Then() call to create its own WorkerRef. Instead, this patch creates a helper class that will auto-disconnect the MozPromise Thenable when the global calls DisconnectFromOwner() on its bound DETH objects. This is done on the proper thread and can happen before we neuter the HybridEventTarget. It also provides an API that is completely window/worker agnostic. The same code will DTRT in both contexts. Note, there is a later patch in this queue that is needed to call DisconnectFromOwner() on workers slightly differently than we are doing currently.
Attachment #8970923 -
Flags: review?(amarchesini)
Assignee | ||
Comment 34•5 years ago
|
||
Comment on attachment 8970924 [details] [diff] [review] P6 Call NoteTerminating() from WorkerPrivate::NotifyInternal(). r=baku Sometimes our current NoteTerminating() call is made too late or perhaps not at all. We want to call it before we neuter the HybridEventTarget which is done in NotifyInternal(Killing). To that end this patch moves the NoteTerminating() call also into NotifyInternal(). We invoke it whenever the status changes to something greater than or equal to Terminating. This means it may be called more than once if we hit Terminating+Killing as separate steps. This is ok. Its better to be sure we call it at least once in case we go straight to Killing.
Attachment #8970924 -
Flags: review?(amarchesini)
Assignee | ||
Comment 35•5 years ago
|
||
Comment on attachment 8970925 [details] [diff] [review] P7 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku This patch makes the ServiceWorkerRegistration::Update() code use the new DOMMozPromiseRequestHolder helper class. It also has to re-work the worker-specific Inner implementation. Previously I was using a secondary MozPromise so I could hold the worker ref alive via a lambda. Instead, this patch makes the SWRUpdateRunnable hold a ThreadSafeWorkerRef instead. This worker ref is passed on to the update callback, etc. I also add a mutex so I can safely reject the promise in cases where the runnable is being delayed and held alive by the timer, but the timer is canceled without ever firing. Since the destructor often runs on a different thread from its ::Run() method we need the mutex to safely access the promise in both places.
Attachment #8970925 -
Flags: review?(amarchesini)
Assignee | ||
Comment 36•5 years ago
|
||
Comment on attachment 8970926 [details] [diff] [review] P8 Make WorkerEventTarget::Dispatch() always consume the already_AddRefed<> runnable. r=baku While testing this I got already_AddRefed<> assertions due to WorkerEventTarget::Dispatch() not always consuming it. We should be consistent in consuming the ref and leaking if an error occurs.
Attachment #8970926 -
Flags: review?(amarchesini)
Assignee | ||
Comment 37•5 years ago
|
||
Comment on attachment 8970714 [details] [diff] [review] P4 Make service worker MozPromise types reject with CopyableErrorResult. r=baku This patch converts the service worker promise types over to the new CopyableErrorResult being added in bug 1357463. This is necessary to get a auto-suppressed and thread-safe error result type. Trying to use MozPromise with the default ErrorResult is much too fragile as its very easy to trigger one of its assertions. Note, we have to use the pattern: + }, [self, outer] (const CopyableErrorResult& aRv) { + outer->MaybeReject(CopyableErrorResult(aRv)); Because all error result types are MOZ_NON_PARAM due to an internal union type. This is basically a work around for that issue.
Attachment #8970714 -
Flags: review?(amarchesini)
Comment 38•5 years ago
|
||
Comment on attachment 8970926 [details] [diff] [review] P8 Make WorkerEventTarget::Dispatch() always consume the already_AddRefed<> runnable. r=baku Review of attachment 8970926 [details] [diff] [review]: ----------------------------------------------------------------- I landed bug 1457073 for this.
Attachment #8970926 -
Flags: review?(amarchesini)
Updated•5 years ago
|
Attachment #8970714 -
Flags: review?(amarchesini) → review+
Updated•5 years ago
|
Attachment #8970923 -
Flags: review?(amarchesini) → review+
Updated•5 years ago
|
Attachment #8970924 -
Flags: review?(amarchesini) → review+
Comment 39•5 years ago
|
||
Comment on attachment 8970925 [details] [diff] [review] P7 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku Review of attachment 8970925 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/serviceworkers/ServiceWorkerRegistrationImpl.cpp @@ +342,5 @@ > > + RefPtr<ServiceWorkerRegistrationPromise::Private> promise; > + { > + MutexAutoLock lock(mMutex); > + promise = mPromise.forget(); promise.swap(mPromise);
Attachment #8970925 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 40•5 years ago
|
||
I'm going to land P5 and P6 in a separate bug so I can use them elsewhere without waiting for CopyableErrorResult to land.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Attachment #8970926 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #8970924 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #8970923 -
Attachment is obsolete: true
Assignee | ||
Comment 41•5 years ago
|
||
Attachment #8970925 -
Attachment is obsolete: true
Attachment #8971379 -
Flags: review+
Comment 42•5 years ago
|
||
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c321bfd64e4d P1 Expose a GetPrincipal() convenience method on service worker descriptor classes. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/d195a2a5de09 P2 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/6f316e11d1fd P3 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/45c4b55aedbb P4 Make service worker MozPromise types reject with CopyableErrorResult. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/89c938649297 P5 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku
Comment 43•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c321bfd64e4d https://hg.mozilla.org/mozilla-central/rev/d195a2a5de09 https://hg.mozilla.org/mozilla-central/rev/6f316e11d1fd https://hg.mozilla.org/mozilla-central/rev/45c4b55aedbb https://hg.mozilla.org/mozilla-central/rev/89c938649297
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 44•5 years ago
|
||
James, WPT sync bot opened a PR for this initially, but closed it when it was first backed out. Now that I have re-landed the WPT changes the bot has not created a new PR. Can you poke it?
Flags: needinfo?(james)
Comment 45•5 years ago
|
||
So, reopen PR and push new commits don't commute (you have to reopen the PR with the old commits and then push the new ones). Sorry about that.
Flags: needinfo?(james)
Upstream PR merged
You need to log in
before you can comment on or make changes to this bug.
Description
•