Closed
Bug 1361825
Opened 7 years ago
Closed 7 years ago
PCompositorBridgeParent::OnChannelClose() assertion without shutdown GC
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(1 file)
Bug 833143 removes a shutdown GC that we should not need. On Windows 8, this causes us to hit an assertion: Assertion failure: CompositorThreadHolder::IsInCompositorThread(), at c:/builds/moz2_slave/m-in-w64-d-0000000000000000000/build/src/gfx/layers/AnimationHelper.cpp:28 #01: mozilla::layers::CompositorAnimationStorage::Release() [obj-firefox/dist/include/mozilla/layers/AnimationHelper.h:83] #02: mozilla::layers::CompositorBridgeParent::ActorDestroy(mozilla::ipc::IProtocol::ActorDestroyReason) [gfx/layers/ipc/CompositorBridgeParent.cpp:644] #03: mozilla::layers::PCompositorBridgeParent::OnChannelClose() [obj-firefox/ipc/ipdl/PCompositorBridgeParent.cpp:1808]
Assignee | ||
Comment 1•7 years ago
|
||
The failure is happening in debug bc6, in the browser/base/content/test/popupNotifications/ directory.
Assignee | ||
Comment 2•7 years ago
|
||
Peter, do you have some idea what might be going on here? I noticed the assertion you added in the other bug, but it was hitting it really early because some CSS code was calling it in startup, so that didn't seem like the same thing. Thanks.
Flags: needinfo?(howareyou322)
Assignee | ||
Comment 3•7 years ago
|
||
Clearing mAnimationStorage in CompositorBridgeParent::StopAndClearResources() seems to work. That happens a little earlier.
Assignee | ||
Comment 4•7 years ago
|
||
Here's the try run for that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d20533a73e9018d659b4c6f6111f082df51b5bdhttps://treeherder.mozilla.org/#/jobs?repo=try&revision=0d20533a73e9018d659b4c6f6111f082df51b5bd
Comment 5•7 years ago
|
||
With bug 833143, the shutdown time was changed. During the shutdown, I guess main thread was calling shutdown to clean up sCompositorThreadHolder in [1] which made CompositorThreadHolder::IsInCompositorThread() return false. In Compositor thread, ActorDestory was calling and then hit the assertion. Therefore, there was a potential race condition between main and compositor thread. I will try to reproduce this in my local first. [1]http://searchfox.org/mozilla-central/source/gfx/layers/ipc/CompositorThread.cpp#131 (In reply to Andrew McCreight [:mccr8] from comment #3) > Clearing mAnimationStorage in > CompositorBridgeParent::StopAndClearResources() seems to work. That happens > a little earlier. If it helps for now, I'm fine with this change.
Flags: needinfo?(howareyou322)
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8865936 [details] Bug 1361825 - Clear mAnimationStorage in CompositorBridgeParent::StopAndClearResources(). https://reviewboard.mozilla.org/r/137524/#review140894 r+ with comment addressed. ::: gfx/layers/ipc/CompositorBridgeParent.cpp:652 (Diff revision 1) > StopAndClearResources(); > > RemoveCompositor(mCompositorID); > > mCompositionManager = nullptr; > mAnimationStorage = nullptr; Since we moved the cleaup of AnimationStorage inside stopAndClearResources(), I think we don't need to clean up here again.
Attachment #8865936 -
Flags: review?(howareyou322) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Peter Chang[:pchang] from comment #7) > Since we moved the cleaup of AnimationStorage inside > stopAndClearResources(), I think we don't need to clean up here again. That's a good point. I've updated the patch to fix that.
Comment 10•7 years ago
|
||
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f045129f8b81 Clear mAnimationStorage in CompositorBridgeParent::StopAndClearResources(). r=pchang
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f045129f8b81
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•