1299621 - Simplify ImageContainerChild memory management

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Similar to bug 1299375, the goal here is to make memory management of an IPDL object more rigorous. ImageContainerChild should use reference counting since it's owned on a separate thread from the IPDL thread. We should also retain references to it while it's in the event queue.

Comment 1

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

Attachment: part 1, ImageContainerChild.h/.cpp

This moves ImageContainerChild into .h/.cpp files, and also hides some of the member access behind functions. Members not covered by this patch will be handled in the next patch.

Comment 2

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

Attachment: part 2, simplify ImageContainerChild memory management

Since ImageContainerChild is owned on multiple threads, it makes sense to reference count it. The IPDL interaction gets a lot cleaner as a result.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=031738545872e35cad214590df0b7858273bfc68

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8787511 [details] [diff] [review]
part 1, ImageContainerChild.h/.cpp

Review of attachment 8787511 [details] [diff] [review]:
-----------------------------------------------------------------

I already gave r+, but why separate the ImageContainerChild into its own file? It is currently hidden in ImageContainer.cpp on purpose because other code should not temper with it (it's been a source of problem in the past). Same question for CompositableChild in fact (when I reviewed the latter I assumed it was because of ImageContainerChild has two variants used in different places, but retrospectively that does not explain moving it to its own file.

Comment 4

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

(In reply to Nicolas Silva [:nical] from comment #3)
> Comment on attachment 8787511 [details] [diff] [review]
> part 1, ImageContainerChild.h/.cpp
> 
> Review of attachment 8787511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I already gave r+, but why separate the ImageContainerChild into its own
> file? It is currently hidden in ImageContainer.cpp on purpose because other
> code should not temper with it (it's been a source of problem in the past).
> Same question for CompositableChild in fact (when I reviewed the latter I
> assumed it was because of ImageContainerChild has two variants used in
> different places, but retrospectively that does not explain moving it to its
> own file.

Without a separate header then ImageBridgeChild has to go through ImageContainer instead of ImageContainerChild directly, which is odd. I guess it's not that bad, but it makes it a lot harder to release the IPDL reference on the ImageBridge thread. Only ImageContainer would know how to hold the ref, and only ImageBridgeChild would know how to post the event.

If other code shouldn't be allowed to tamper with it, then we shouldn't expose GetPImageContainerChild. It looks like it's only there so ImageBridgeChild can send a constructor message. Maybe instead we should just pass the ImageContainerChild to CreateImageClient?

Comment 5

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

Actually I don't know what I meant in that first paragraph. It looks like it could be moved back into ImageContainer just fine - I don't have a particular opinion here.

Comment 6

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

I take back my taking that back. ImageBridgeChild::DispatchReleaseImageContainer looks annoying to implement without a separate header, unless we change ImageBridge to hold a ref to the ImageContainer, and then call a method on ImageContainer that it's okay to call SendAsyncDelete on the ImageContainerChild.

Comment 7

[Nicolas Silva [:nical]]

The pattern we use with PTexture for example is to have a static method in TextureClient when the forwarder has to do something specific with the actor (like invoking Destroy). I think that it's best to not expose the IPDL actor because people will eventually abuse it.
It's not worth arguing over for PImageContainer and PCompositable because they are simple actors and the eventual hacks that abuse poking into the actor's internals should be easy to fix, but I would not agree to let this happen with PTexture at any cost (we had too many issues).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3972080f1899
Move ImageContainerChild to its own file. (bug 1299621 part 1, r=nical)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d2271c836ce
Simplify ImageContainerChild memory management. (bug 1299621 part 2, r=nical)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3972080f1899
https://hg.mozilla.org/mozilla-central/rev/9d2271c836ce