fix MozPromise usage in Clients API on worker thread

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
9 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 2 bugs)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(6 attachments, 10 obsolete attachments)

1.64 KB, patch
baku
: review+
Details | Diff | Splinter Review
10.65 KB, patch
baku
: review+
Details | Diff | Splinter Review
8.74 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.95 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.49 KB, patch
baku
: review+
Details | Diff | Splinter Review
8.15 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
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.
Priority: -- → P2
Depends on: 1457187
No longer depends on: 1456466
Posted patch clients-wip.patch (obsolete) — Splinter Review
Very early work-in-progress.  I don't expect this to be green yet.
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.
Attachment #8971628 - Flags: review?(amarchesini)
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.
Attachment #8971629 - Flags: review?(amarchesini)
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.
Attachment #8972329 - Flags: review?(amarchesini)
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.
Attachment #8972331 - Flags: review?(amarchesini)
Comment on attachment 8972332 [details] [diff] [review]
P4 Use DOMMozPromiseRequestHolder in ClientSource. r=baku

This makes ClientSource thenables use DOMMozPromiseRequestHolder.
Attachment #8972332 - Flags: review?(amarchesini)
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).
Attachment #8972370 - Flags: review?(amarchesini)
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.
Attachment #8971628 - Flags: review?(amarchesini) → review+
Attachment #8971629 - Flags: review?(amarchesini) → review+
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
Attachment #8972329 - Flags: review?(amarchesini) → review+
Attachment #8972331 - Flags: review?(amarchesini) → review+
Attachment #8972332 - Flags: review?(amarchesini) → review+
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!
Attachment #8972370 - Flags: review?(amarchesini) → review+

Comment 29

Last year
Pushed by bkelly@mozilla.com:
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
Duplicate of this bug: 1454633
Depends on: 1458970
Depends on: 1458971
You need to log in before you can comment on or make changes to this bug.