1280715 - Refactor destruction code in TextureForwarder's subclasses

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[George Wright (:gw280) (needinfo me!)]

We rely on IPCOpen() to ensure that we can send IPC messages correctly. This is normally overridden and managed in the leaf class in a class hierarchy (e.g. ShadowLayerForwarder).

Sometimes we need to be able to send IPC messages into to do destruction-related cleanup (such as in TextureForwarder's dtor, where we dealloc shmems). At the moment, this also now checks to see if the shmem allocator is still available but ideally we should move this cleanup to Destroy() methods that are executed before destruction.

I opted not to fix this immediately in bug 1176011 as that code is already complex enough and I didn't want to involve more refactoring in the patch (and adding even more risk onto an already risky patch).

Comment 1

[George Wright (:gw280) (needinfo me!)]

Attachment: movetilelockallocator.patch

I had a long think about this, and it looks like the tile lock allocator is the only thing that complicates this. I think moving this into CompositorBridgeChild makes sense, and makes TextureForwarder an abstract class which is more in keeping with what it should be.

Comment 2

[George Wright (:gw280) (needinfo me!)]

Oops, looks like this patch isn't quite correct. TextureForwarder.cpp should be deleted, not just have some code removed. The rest of the patch is fine though.

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8787050 [details] [diff] [review]
movetilelockallocator.patch

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

::: gfx/layers/ipc/CompositorBridgeChild.cpp
@@ +101,5 @@
>      mTexturePools[i]->Destroy();
>    }
>  
> +  if (mSectionAllocator) {
> +    delete mSectionAllocator;

Please null out mSectionAllocator after deleting (I realize the original code did not), it'll be less footgun-prone that way.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9beac1f92bd0
Move FixedSizeShmemAllocator to CompositorBridgeChild to allow for a more sane destruction sequence r=nical

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9beac1f92bd0