Refactor destruction code in TextureForwarder's subclasses

RESOLVED FIXED in Firefox 51

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gw280, Assigned: gw280)

Tracking

(Blocks: 1 bug, {feature})

unspecified
mozilla51
feature
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

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).
(Assignee)

Updated

2 years ago
See Also: → bug 1176011

Updated

2 years ago
Keywords: feature
Whiteboard: [gfx-noted]
(Assignee)

Updated

2 years ago
Blocks: 1297251
(Assignee)

Updated

2 years ago
Assignee: nobody → gwright
Created attachment 8787050 [details] [diff] [review]
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.
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+

Comment 4

2 years ago
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

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9beac1f92bd0
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.