Closed Bug 1456466 Opened 5 years ago Closed 5 years ago

MozPromisify ServiceWorkerRegistration::Inner::Update()

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(5 files, 19 obsolete files)

P1 Expose a GetPrincipal() convenience method on service worker descriptor classes. r=baku
5.03 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P2 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku
19.17 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P3 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku
4.67 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P4 Make service worker MozPromise types reject with CopyableErrorResult. r=baku
6.09 KB, patch
baku
: review+
Details | Diff | Splinter Review
P5 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku
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.
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
Attachment #8970527 - Flags: review+
Attachment #8970528 - Flags: review+
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR was closed without merging
Depends on: 1357463
Updated to make ~SWRUpdateRunnable() reject the promise if it never ran.
Attachment #8970527 - Attachment is obsolete: true
Flags: needinfo?(bkelly)
Attachment #8970625 - Flags: review+
No longer depends on: 1357463
Attachment #8970627 - Attachment is obsolete: true
Priority: -- → P2
Depends on: 1357463
Attachment #8970671 - Flags: review+
Attachment #8970672 - Flags: review+
This still trips some assertions, but its better.
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.
Blocks: 1456893
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)
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)
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)
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)
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 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)
Attachment #8970714 - Flags: review?(amarchesini) → review+
Attachment #8970923 - Flags: review?(amarchesini) → review+
Attachment #8970924 - Flags: review?(amarchesini) → review+
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+
Blocks: 1457157
Blocks: 1457073
I'm going to land P5 and P6 in a separate bug so I can use them elsewhere without waiting for CopyableErrorResult to land.
No longer blocks: 1457073
Depends on: 1457073
Depends on: 1457187
Attachment #8970926 - Attachment is obsolete: true
Attachment #8970924 - Attachment is obsolete: true
Attachment #8970923 - Attachment is obsolete: true
No longer blocks: 1456893
Blocks: 1456981
No longer blocks: 1457157
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
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)
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)
See Also: → 1587794
You need to log in before you can comment on or make changes to this bug.