fix MozPromise usage in Clients API on worker thread

RESOLVED FIXED in Firefox 61

Status

()

P2
normal
RESOLVED FIXED
10 months ago
6 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
(Assignee)

Description

10 months ago
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.

Updated

10 months ago
Priority: -- → P2
(Assignee)

Updated

10 months ago
Depends on: 1457187
No longer depends on: 1456466
(Assignee)

Comment 1

10 months ago
Created attachment 8971404 [details] [diff] [review]
clients-wip.patch

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

Comment 2

10 months ago
Created attachment 8971607 [details] [diff] [review]
P1 Use DOMMozPromiseRequestHolder in the clients API binding objects. r=baku
Attachment #8971404 - Attachment is obsolete: true
(Assignee)

Comment 3

10 months ago
Created attachment 8971608 [details] [diff] [review]
P2 Clear a worker's ClientSource when it reaches Terminating. r=baku
(Assignee)

Comment 4

10 months ago
Created attachment 8971625 [details] [diff] [review]
P0 Don't call DisconnectFromOwner() in DOMMozPromiseRequestHolder::Complete(). r=baku
(Assignee)

Comment 5

10 months ago
Created attachment 8971628 [details] [diff] [review]
P0 Don't call DisconnectFromOwner() in DOMMozPromiseRequestHolder::Complete(). r=baku
Attachment #8971625 - Attachment is obsolete: true
(Assignee)

Comment 6

10 months ago
Created attachment 8971629 [details] [diff] [review]
P1 Use DOMMozPromiseRequestHolder in the clients API binding objects. r=baku
Attachment #8971607 - Attachment is obsolete: true
(Assignee)

Comment 7

10 months ago
Created attachment 8971630 [details] [diff] [review]
P2 Clear a worker's ClientSource when it reaches Terminating. r=baku
Attachment #8971608 - Attachment is obsolete: true
(Assignee)

Comment 8

10 months ago
Created attachment 8971707 [details] [diff] [review]
P3 Replace ClientHandleOpChild MozPromise direct std::function callbacks. r=baku
(Assignee)

Comment 9

10 months ago
Created attachment 8971715 [details] [diff] [review]
P3 Replace ClientHandleOpChild MozPromise direct std::function callbacks. r=baku
Attachment #8971707 - Attachment is obsolete: true
(Assignee)

Comment 10

10 months ago
Created attachment 8971719 [details] [diff] [review]
P4 Use DOMMozPromiseRequestHolder in ClientSource. r=baku
(Assignee)

Comment 14

10 months ago
Created attachment 8972329 [details] [diff] [review]
P2 Clear a worker's ClientSource when it reaches Terminating. r=baku
Attachment #8971630 - Attachment is obsolete: true
(Assignee)

Comment 15

10 months ago
Created attachment 8972331 [details] [diff] [review]
P3 Replace ClientHandleOpChild MozPromise direct std::function callbacks. r=baku
Attachment #8971715 - Attachment is obsolete: true
(Assignee)

Comment 16

10 months ago
Created attachment 8972332 [details] [diff] [review]
P4 Use DOMMozPromiseRequestHolder in ClientSource. r=baku
Attachment #8971719 - Attachment is obsolete: true
(Assignee)

Comment 17

10 months ago
Created attachment 8972333 [details] [diff] [review]
P5 Make ClientManagerChild use a WeakWorkerRef instead of a WorkerHolderToken. r=baku

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

Comment 18

10 months ago
Created attachment 8972370 [details] [diff] [review]
P5 Make ClientManager keep its actor alive until the worker reaches Terminating. r=baku

https://treeherder.mozilla.org/#/jobs?repo=try&revision=673bdbad79079904b86d77484aa827f2c12f924e
Attachment #8972333 - Attachment is obsolete: true
(Assignee)

Comment 19

10 months ago
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)
(Assignee)

Comment 20

10 months ago
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)
(Assignee)

Comment 21

10 months ago
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)
(Assignee)

Comment 22

10 months ago
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)
(Assignee)

Comment 23

10 months ago
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)
(Assignee)

Comment 24

10 months ago
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)
(Assignee)

Comment 25

10 months ago
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+
(Assignee)

Comment 28

10 months ago
Created attachment 8972567 [details] [diff] [review]
P2 Clear a worker's ClientSource when it reaches Terminating. r=baku
Attachment #8972329 - Attachment is obsolete: true
Attachment #8972567 - Flags: review+

Comment 29

10 months ago
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.