Closed Bug 1394337 Opened 2 years ago Closed 2 years ago

Fix uninitailzed mPipelineId by WebRenderBridgeParent::CreateDestroyed()

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 2 obsolete files)

Current WebRenderBridgeParent::CreateDestroyed() causes to leak CompositorBridgeParent::LayerTreeState.
Assignee: nobody → sotaro.ikeda.g
Blocks: 1394338
(In reply to Sotaro Ikeda [:sotaro  PTO 1/Sep - 13/Sep] from comment #0)
> Current WebRenderBridgeParent::CreateDestroyed() causes to leak
> CompositorBridgeParent::LayerTreeState.

WebRenderBridgeParent holded uninitailzed mPipelineId when it was created by WebRenderBridgeParent::CreateDestroyed().
Attachment #8901720 - Flags: review?(bugmail)
(In reply to Sotaro Ikeda [:sotaro  PTO 1/Sep - 13/Sep] from comment #1)
> 
> WebRenderBridgeParent holded uninitailzed mPipelineId when it was created by
> WebRenderBridgeParent::CreateDestroyed().

mPipelineId is used as layers id like CrossProcessCompositorBridgeParent::DeallocPWebRenderBridgeParent()

   https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp#251
Comment on attachment 8901720 [details] [diff] [review]
patch - Fix WebRenderBridgeParent::CreateDestroyed()

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

Please update the commit message to describe in more detail what the problem is how the patch fixes it.

From looking at the code I don't see how this is a memory leak, because the CrossProcessCompositorBridgeParent should never be getting registered in the sIndirectLayerTrees structure at all. Instead what I would expect is that when CrossProcessCompositorBridgeParent::DeallocPWebRenderBridgeParent is called, it will call EraseLayerState with some garbage uninitialized value, and so it will erase some random layer state entry. Is that correct?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> 
> Please update the commit message to describe in more detail what the problem
> is how the patch fixes it.
> 
> From looking at the code I don't see how this is a memory leak, because the
> CrossProcessCompositorBridgeParent should never be getting registered in the
> sIndirectLayerTrees structure at all. Instead what I would expect is that
> when CrossProcessCompositorBridgeParent::DeallocPWebRenderBridgeParent is
> called, it will call EraseLayerState with some garbage uninitialized value,
> and so it will erase some random layer state entry. Is that correct?

Yes. I am going to update the commit message.
Updated the comment.
Attachment #8901720 - Attachment is obsolete: true
Attachment #8901720 - Flags: review?(bugmail)
Attachment #8902062 - Flags: review?(bugmail)
Summary: Fix WebRenderBridgeParent::CreateDestroyed() → Fix uninitailzed mPipelineId by WebRenderBridgeParent::CreateDestroyed()
Attachment #8902062 - Flags: review?(bugmail)
Attachment #8902073 - Flags: review?(bugmail)
Comment on attachment 8902073 [details] [diff] [review]
patch - Fix uninitailzed mPipelineId by WebRenderBridgeParent::CreateDestroyed()

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

s/uninitailzed/uninitialized/ in the commit message
Attachment #8902073 - Flags: review?(bugmail) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf44180cc29a
Fix uninitialized mPipelineId by WebRenderBridgeParent::CreateDestroyed() r=kats
https://hg.mozilla.org/mozilla-central/rev/cf44180cc29a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.