1457157 - fix MozPromise usage in Clients API on worker thread

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1456466 I sorted out some issues with MozPromise on worker threads.  I need to go back to fix these issues in Clients API.  Its a bit complicated, though, because clients API uses a mixture of promise types in the same chain.

We need to fix this in order to land the memory leak fix in bug 1451381.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: clients-wip.patch

Very early work-in-progress.  I don't expect this to be green yet.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Use DOMMozPromiseRequestHolder in the clients API binding objects. r=baku

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Clear a worker's ClientSource when it reaches Terminating. r=baku

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Don't call DisconnectFromOwner() in DOMMozPromiseRequestHolder::Complete(). r=baku

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Don't call DisconnectFromOwner() in DOMMozPromiseRequestHolder::Complete(). r=baku

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Use DOMMozPromiseRequestHolder in the clients API binding objects. r=baku

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Clear a worker's ClientSource when it reaches Terminating. r=baku

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Replace ClientHandleOpChild MozPromise direct std::function callbacks. r=baku

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Replace ClientHandleOpChild MozPromise direct std::function callbacks. r=baku

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Use DOMMozPromiseRequestHolder in ClientSource. r=baku

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Clear a worker's ClientSource when it reaches Terminating. r=baku

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Replace ClientHandleOpChild MozPromise direct std::function callbacks. r=baku

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Use DOMMozPromiseRequestHolder in ClientSource. r=baku

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make ClientManagerChild use a WeakWorkerRef instead of a WorkerHolderToken. r=baku

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

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make ClientManager keep its actor alive until the worker reaches Terminating. r=baku

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

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8971628 [details] [diff] [review]
P0 Don't call DisconnectFromOwner() in DOMMozPromiseRequestHolder::Complete(). r=baku

Andrea, the original DOMMozPromiseRequestMolder disconnected its from the global when Complete() was called.  This is somewhat inconvenient since often a promise reaction will need to access the global and getting it from the holder would be nice.

This patch removes the disconnect from the Complete() call.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8971629 [details] [diff] [review]
P1 Use DOMMozPromiseRequestHolder in the clients API binding objects. r=baku

This makes the various code in clients/api use DOMMozPromiseRequestHolder.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8972329 [details] [diff] [review]
P2 Clear a worker's ClientSource when it reaches Terminating. r=baku

When a window hits FreeInnerObjects it first clears its ClientSource and then disconnects bound DETH objects.  Workers, however, do it in the reverse order.  They disconnect DETH objects at Terminating, but don't remove the ClientSource until the run loop exits.

Lets align workers with window behavior.  When we disconnect DETH objects we also should clear the ClientSource.

This patch also includes some fallout fixes from this.  We now need to handle mClientSource being nullptr when the worker thread is alive, but the status is >= Terminating.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8972331 [details] [diff] [review]
P3 Replace ClientHandleOpChild MozPromise direct std::function callbacks. r=baku

This removes a MozPromise from the ClientHandleOpChild that did not have any global available to attach a holder to.  Instead we make the actor just use std::function callbacks directly instead.  This also avoids an unnecessary runnable dispatch.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8972332 [details] [diff] [review]
P4 Use DOMMozPromiseRequestHolder in ClientSource. r=baku

This makes ClientSource thenables use DOMMozPromiseRequestHolder.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8972370 [details] [diff] [review]
P5 Make ClientManager keep its actor alive until the worker reaches Terminating. r=baku

Finally, this makes ClientManager hold the worker alive until Terminating.

Note, I did try to convert to WorkerRef, but I didn't see a way to do what I needed.  I need:

1. A strong ref to keep the worker alive until ActorDestroy.
2. AllowIdleShutdownStart behavior.  Every worker thread has a ClientManager since it has to create a ClientSource.  These manager instances must not keep every worker alive through GC.

StrongWorkerRef provides (1), but not (2).  WeakWorkerRef provides (2), but not (1).

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Note, the Thenables in ClientSourceOpChild are already ok.  They use a holder that is disconnected by the actor shutdown.  On worker threads the actor shutdown will be triggered by the ClientManager's holder and actor, etc.

Also note that I did not explicitly try your leak fix patch on top of this stuff.  I just think this should all be more correct.

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8972329 [details] [diff] [review]
P2 Clear a worker's ClientSource when it reaches Terminating. r=baku

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

::: dom/workers/WorkerPrivate.cpp
@@ +3277,5 @@
>        }
>  
>        // If we're supposed to die then we should exit the loop.
>        if (currentStatus == Killing) {
> +        // The ClientSource should be clared in NotifyInternal() when we reach

cleared

Comment 27

[Andrea Marchesini [:baku]]

Comment on attachment 8972370 [details] [diff] [review]
P5 Make ClientManager keep its actor alive until the worker reaches Terminating. r=baku

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

Great!

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Clear a worker's ClientSource when it reaches Terminating. r=baku

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f9dd911845c
P0 Don't call DisconnectFromOwner() in DOMMozPromiseRequestHolder::Complete(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d667f861e2d1
P1 Use DOMMozPromiseRequestHolder in the clients API binding objects. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c86679a9bf83
P2 Clear a worker's ClientSource when it reaches Terminating. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9184164d3c6c
P3 Replace ClientHandleOpChild MozPromise direct std::function callbacks. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bac87ce995f
P4 Use DOMMozPromiseRequestHolder in ClientSource. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/409f43dceb0e
P5 Make ClientManager keep its actor alive until the worker reaches Terminating. r=baku

Comment 30

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/3f9dd911845c
https://hg.mozilla.org/mozilla-central/rev/d667f861e2d1
https://hg.mozilla.org/mozilla-central/rev/c86679a9bf83
https://hg.mozilla.org/mozilla-central/rev/9184164d3c6c
https://hg.mozilla.org/mozilla-central/rev/0bac87ce995f
https://hg.mozilla.org/mozilla-central/rev/409f43dceb0e