1299375 - Simplify CompositableClient memory management

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: WIP v0

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.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, CompositableChild.h/cpp

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.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, WIP

This is looking not bad on try, waiting for one more run to request review.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, use RefPtr consistently

Patch description is in comment #1.

Comment 5

[Nicolas Silva [:nical]]

CC Sotaro since he's been involved a lot in compositable related stuff as well.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a3c6ff07a6d
https://hg.mozilla.org/mozilla-central/rev/b7a471152f32