Closed Bug 1280715 Opened 5 years ago Closed 5 years ago

Refactor destruction code in TextureForwarder's subclasses

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: gw280, Assigned: gw280)

References

(Blocks 1 open bug)

Details

(Keywords: feature, Whiteboard: [gfx-noted])

Attachments

(1 file)

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).
See Also: → 1176011
Keywords: feature
Whiteboard: [gfx-noted]
Assignee: nobody → gwright
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.
Attachment #8787050 - Flags: review?(nical.bugzilla)
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 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.
Attachment #8787050 - Flags: review?(nical.bugzilla) → review+
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9beac1f92bd0
Move FixedSizeShmemAllocator to CompositorBridgeChild to allow for a more sane destruction sequence r=nical
https://hg.mozilla.org/mozilla-central/rev/9beac1f92bd0
Status: NEW → RESOLVED
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.