Closed Bug 1125273 Opened 9 years ago Closed 9 years ago

Disable CompositorVsyncObserver destructor assertion

Categories

(Core Graveyard :: Widget: Gonk, defect)

26 Branch
x86
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(1 file)

From try https://treeherder.mozilla.org/#/jobs?repo=try&revision=272c8de304cb.

Enabling the vsync compositor and touch resampling is failing these mochitests because of this assert - https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp?from=CompositorParent.cpp#229. 

With only the vsync compositor enabled, this kind of accurate since NS_INLINE_DECL_THREADSAFE_REFCOUNTING doesn't guarantee which thread an object is destroyed on. But the only reference to the CompositorVsyncObserver is the CompositorParent and was always released on the compositor thread.

With touch resampling enabled, another reference to the CompositorVsyncObserver is added in the GeckoTouchDispatcher. (https://dxr.mozilla.org/mozilla-central/source/widget/gonk/GeckoTouchDispatcher.cpp?from=GeckoTouchDispatcher.cpp&case=true#121). The GeckoTouchDispatcher is cleaned up on the main thread due to ClearOnShutdown, which holds the last reference. So the CompositorVsyncObserver can be destroyed both on the main thread and compositor thread, it's racy.

I tried using NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION, but the AddRef/Release are not overrideable, so the VsyncObserver threadsafe refcouting declaration is still in effect. I think it's safe to just disable this thread assertion.
Status: NEW → ASSIGNED
Attachment #8553888 - Flags: review?(bgirard)
Comment on attachment 8553888 [details] [diff] [review]
Delete Compositor Thread assertion in CompositorVsyncObserver

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

Seems ok, if we descontruct we're should have the only remaining reference and we only clear our own variable. I don't see what could go wrong (famous last words).
Attachment #8553888 - Flags: review?(bgirard) → review+
(In reply to Mason Chang [:mchang] from comment #0)
> ...
> CompositorVsyncObserver can be destroyed both on the main thread and
> compositor thread, it's racy.

(In reply to Benoit Girard (:BenWa) from comment #3)
> ...
> and we only clear our own variable. I don't see what could go wrong (famous
> last words).

Does it make sense to have the assert still there and assert that we're either on compositor or main thread?
(In reply to Milan Sreckovic [:milan] from comment #5)
> (In reply to Benoit Girard (:BenWa) from comment #3)
> > ...
> > and we only clear our own variable. I don't see what could go wrong (famous
> > last words).
> 
> Does it make sense to have the assert still there and assert that we're
> either on compositor or main thread?

We could do that too, I don't have a strong preference either way.
https://hg.mozilla.org/mozilla-central/rev/11522eeab0d4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8553888 [details] [diff] [review]
Delete Compositor Thread assertion in CompositorVsyncObserver

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Project Silk, 1062331
User impact if declined: We won't be able to enable touch resampling with silk.
Testing completed: Mochitests, manual testing
Risk to taking this patch (and alternatives if risky): Low, this just removes an invalid assertion with touch resampling enabled.
String or UUID changes made by this patch: None
Attachment #8553888 - Flags: approval-mozilla-b2g37?
Attachment #8553888 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: