Closed
Bug 1299375
Opened 5 years ago
Closed 5 years ago
Simplify CompositableClient memory management
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(2 files, 2 obsolete files)
8.50 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
42.14 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
CompositableClient has all sorts of confusing manual memory management. It will not work well if we need to reliably destroy and restart ImageBridgeChild. The most egregious aspect is that CompositableClient refs are held outside of a RefPtr, and all Release() calls are forwarded to the ImageBridge thread. This is done because (1) CompositableClient owns an IPDL actor (CompositableChild), which must be destroyed on the ImageBridge thread, and (2) ImageBridge does not retain references to the CompositableClient while it's in the event queue. All of this relies on very careful ordering and thread access assumptions that either aren't well commented or are not asserted. So this bug aims to fix as much of this as we can.
![]() |
Assignee | |
Comment 1•5 years ago
|
||
High-level changes: * CompositableClient is now refcounted with RefPtr. ImageBridge does not own an implicit reference, and instead explicitly holds a reference while it's in the event queue. * CompositableChild is now also reference counted. * CompositableClient can be freed on any thread. * CompositableChild can be freed on any thread, but IPDL owns a reference. To make this work, CompositableChild has been split into two variations: the base CompositableChild, which is only ever owned on the main thread. The other variation is AsyncCompositableChild, which can be owned on any thread. LayerTransactionChild creates CompositableChild on the main thread, whereas ImageBridgeChild creates AsyncCompositableChild on the ImageBridge thread. Similarly, LayerTransactionChild will free CompositableChild normally on the main thread. ImageBridgeChild will free AsyncCompositableChild only on the ImageBridge thread, routing destruction through the event queue if needed. Additionally this patch adds a lot of asserts that certain functions are checking state on the correct thread. For example, it is not valid to check CompositableClient::mCanSend on anything but the forwarder thread, otherwise it's racy. There are some things I'm not 100% sure about: (1) SendDestroySync's purpose is not clear. It's not a sync message. (2) Are transactions atomic? I'm guessing they have to be - if the thread's event loop can run while we build a transaction, it's possible for actors inside the transaction to go away. Nothing keeps them alive. This patch could potentially expose problems there. (3) CompositableChild::FromIPDLActor looks all but unused in the async case, which is good since this function is dangerous off the ImageBridge thread. I'm not sure why we cleanly free these actors when shutting ImageBridge down. Is there texture stuff that needs to be coordinated? If it's possible to just close the channel that would be nicer.
![]() |
Assignee | |
Comment 2•5 years ago
|
||
This splits CompositableChild out into its own header and source files. I also took the opportunity to make it less open-coded, i.e., wrap its member variables in setters/accessors.
Attachment #8786618 -
Attachment is obsolete: true
Attachment #8786871 -
Flags: review?(nical.bugzilla)
![]() |
Assignee | |
Comment 3•5 years ago
|
||
This is looking not bad on try, waiting for one more run to request review.
![]() |
Assignee | |
Comment 4•5 years ago
|
||
Patch description is in comment #1.
Attachment #8786873 -
Attachment is obsolete: true
Attachment #8786925 -
Flags: review?(nical.bugzilla)
Comment 5•5 years ago
|
||
CC Sotaro since he's been involved a lot in compositable related stuff as well.
Updated•5 years ago
|
Attachment #8786871 -
Flags: review?(nical.bugzilla) → review+
Updated•5 years ago
|
Attachment #8786925 -
Flags: review?(nical.bugzilla) → review+
Pushed by danderson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3c6ff07a6d Give CompositableChild into its own header and source files. (bug 1299375 part 1, r=nical) https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a471152f32 Refactor CompositableClient memory management. (bug 1299375 part 2, r=nical)
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a3c6ff07a6d https://hg.mozilla.org/mozilla-central/rev/b7a471152f32
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•