Closed Bug 1299375 Opened 5 years ago Closed 5 years ago

Simplify CompositableClient memory management


(Core :: Graphics: Layers, defect)

Not set



Tracking Status
firefox51 --- fixed


(Reporter: dvander, Assigned: dvander)




(2 files, 2 obsolete files)

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.
Attached patch WIP v0 (obsolete) — Splinter Review
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.
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)
Attached patch part 2, WIP (obsolete) — Splinter Review
This is looking not bad on try, waiting for one more run to request review.
Patch description is in comment #1.
Attachment #8786873 - Attachment is obsolete: true
Attachment #8786925 - Flags: review?(nical.bugzilla)
CC Sotaro since he's been involved a lot in compositable related stuff as well.
Attachment #8786871 - Flags: review?(nical.bugzilla) → review+
Attachment #8786925 - Flags: review?(nical.bugzilla) → review+
Pushed by
Give CompositableChild into its own header and source files. (bug 1299375 part 1, r=nical)
Refactor CompositableClient memory management. (bug 1299375 part 2, r=nical)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.