Closed Bug 1299621 Opened 3 years ago Closed 3 years ago

Simplify ImageContainerChild memory management

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(2 files)

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.
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.
Attachment #8787511 - Flags: review?(nical.bugzilla)
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
Attachment #8787513 - Flags: review?(nical.bugzilla)
Attachment #8787511 - Flags: review?(nical.bugzilla) → review+
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.
(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?
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.
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.
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).
Attachment #8787513 - Flags: review?(nical.bugzilla) → review+
Pushed by danderson@mozilla.com:
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)
https://hg.mozilla.org/mozilla-central/rev/3972080f1899
https://hg.mozilla.org/mozilla-central/rev/9d2271c836ce
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.