|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
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]
The failure is happening in debug bc6, in the browser/base/content/test/popupNotifications/ directory.
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.
Clearing mAnimationStorage in CompositorBridgeParent::StopAndClearResources() seems to work. That happens a little earlier.
With bug 833143, the shutdown time was changed. During the shutdown, I guess main thread was calling shutdown to clean up sCompositorThreadHolder in  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. 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.
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+
(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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/f045129f8b81 Clear mAnimationStorage in CompositorBridgeParent::StopAndClearResources(). r=pchang
Status: NEW → RESOLVED
Last Resolved: 8 months 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.