PCompositorBridgeParent::OnChannelClose() assertion without shutdown GC

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: Layers
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
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

9 months ago
The failure is happening in debug bc6, in the browser/base/content/test/popupNotifications/ directory.
(Assignee)

Comment 2

9 months 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

9 months ago
Clearing mAnimationStorage in CompositorBridgeParent::StopAndClearResources() seems to work. That happens a little earlier.

Comment 5

9 months 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

9 months 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

8 months 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

8 months ago
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
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.