1456466 - MozPromisify ServiceWorkerRegistration::Inner::Update()

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

Split off some patches from bug 1455695 to land.  These make Update() use a MozPromise internally.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Expose a GetPrincipal() convenience method on service worker descriptor classes. r=baku

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku

Comment 4

[Pulsebot]

Pushed by [e-mail]:
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

Comment 5

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10602 for changes under testing/web-platform/tests

Comment 6

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 7

[Noemi Erli[:noemi_erli]]

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

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku

Updated to make ~SWRUpdateRunnable() reject the promise if it never ran.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make service worker MozPromise types reject with CopyableErrorResult. r=baku

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42251f6de6f25dbfc024b6b22ded9f9bfdb1eceb

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make ServiceWorkerRegistration::Inner::Update() use MozPromise and IPC-safe types. r=baku

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Fix tests to expect ServiceWorkerRegistration.update() to resolve with a registration. r=baku

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make service worker MozPromise types reject with CopyableErrorResult. r=baku

Let's take another shot at CopyableErrorResult:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0596f9983a024c83796bbeb7b256d32ce8ae301b

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make service worker MozPromise types reject with CopyableErrorResult. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9b73ccf493b57be1e2b1a3bb76333a52380f026

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add a DOMMozPromiseRequestHolder helper class to auto-disconnect MozPromise Thenables when the global dies. r=baku

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add a DOMMozPromiseRequestHolder helper class to auto-disconnect MozPromise Thenables when the global dies. r=baku

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Call NoteTerminating() from WorkerPrivate::NotifyInternal(). r=baku

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku

This still trips some assertions, but its better.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make service worker MozPromise types reject with CopyableErrorResult. r=baku

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ce917ed8ad6bb82f5423a28aa3dac8d76f5ca0

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Call NoteTerminating() from WorkerPrivate::NotifyInternal(). r=baku

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Make WorkerEventTarget::Dispatch() always consume the already_AddRefed<> runnable. r=baku

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

This makes things happy locally.  Lets check try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5792cd2143a4735207e95ebae39faf8c186adb3d

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Add a DOMMozPromiseRequestHolder helper class to auto-disconnect MozPromise Thenables when the global dies. r=baku

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Call NoteTerminating() from WorkerPrivate::NotifyInternal(). r=baku

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Make WorkerEventTarget::Dispatch() always consume the already_AddRefed<> runnable. r=baku

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 38

[Andrea Marchesini [:baku]]

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.

Comment 39

[Andrea Marchesini [:baku]]

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);

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

I'm going to land P5 and P6 in a separate bug so I can use them elsewhere without waiting for CopyableErrorResult to land.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make ServiceWorkerRegistration::Update() use DOMMozPromiseRequestHolder. r=baku

Comment 42

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

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

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 45

[James Graham [:jgraham]]

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.

Comment 46

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged