Closed Bug 1361825 Opened 7 years ago Closed 7 years ago

PCompositorBridgeParent::OnChannelClose() assertion without shutdown GC

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

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]
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.
Flags: needinfo?(howareyou322)
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 [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 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 amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f045129f8b81
Clear mAnimationStorage in CompositorBridgeParent::StopAndClearResources(). r=pchang
https://hg.mozilla.org/mozilla-central/rev/f045129f8b81
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: