1276931 - gfxPlatform::Init can be called from the vblankloop

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Josh Matthews [:jdm]]

https://crash-stats.mozilla.com/search/?product=Firefox&moz_crash_reason=%3DMOZ_RELEASE_ASSERT%28NS_IsMainThread%28%29%29&_facets=moz_crash_reason&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=moz_crash_reason#crash-reports

Example: https://crash-stats.mozilla.com/report/index/b299cc31-c881-42a9-8ce9-c59a72160524

This results in a release assert, since it's not being called from the main thread.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

We should be able to make sure we call Init() before starting the vsync thread.

Comment 3

[Mason Chang [Inactive] [:mchang]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> We should be able to make sure we call Init() before starting the vsync
> thread.

The only way we start the vsync thread is during gfxWindowsPlatform::Init. This seems to be happening during shutdown where we're calling the gfxWindowsPlatform during vsync shutdown.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #3)
> The only way we start the vsync thread is during gfxWindowsPlatform::Init.
> This seems to be happening during shutdown where we're calling the
> gfxWindowsPlatform during vsync shutdown.

Fun.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

We should probably set gPlatform to something like 0x1 so that we don't try to reinitialize and crash instead.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Disable vsync before clearing VsyncSource

I think what's happening here is that we shutdown gfxPlatform and don't wait to ensure that vsync is actually shutdown yet. Normally, the destructor for Vsync will stop the vsync thread during clean up. However, the destructor hasn't run yet and instead the vblank loop is going on the vsync thread at the same time the main thread shut down the gfxPlatform. This was fine until bug 1256547, which added calls to fetch gfxPlatform during each vblank loop. This patch disables vsync on the main thread during gfxPlatform::Shutdown.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Disable vsync before clearing VsyncSource

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8758423 [details] [diff] [review]
Disable vsync before clearing VsyncSource

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

::: gfx/thebes/gfxPlatform.cpp
@@ +822,5 @@
> +    gPlatform->mMemoryPressureObserver = nullptr;
> +    gPlatform->mSkiaGlue = nullptr;
> +
> +    if (XRE_IsParentProcess()) {
> +      gPlatform->mVsyncSource->GetGlobalDisplay().DisableVsync();

Shouldn't this wait until the vsync thread dies?

Comment 9

[Mason Chang [Inactive] [:mchang]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> Comment on attachment 8758423 [details] [diff] [review]
> Disable vsync before clearing VsyncSource
> 
> Review of attachment 8758423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxPlatform.cpp
> @@ +822,5 @@
> > +    gPlatform->mMemoryPressureObserver = nullptr;
> > +    gPlatform->mSkiaGlue = nullptr;
> > +
> > +    if (XRE_IsParentProcess()) {
> > +      gPlatform->mVsyncSource->GetGlobalDisplay().DisableVsync();
> 
> Shouldn't this wait until the vsync thread dies?

That only happens during destruction since we want to reuse the thread normally. Since we're in shutdown, nothing should be enabling vsync again, so that should stop this crash as the thread will just be idle.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #9)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > Comment on attachment 8758423 [details] [diff] [review]
> > Disable vsync before clearing VsyncSource
> > 
> > Review of attachment 8758423 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/thebes/gfxPlatform.cpp
> > @@ +822,5 @@
> > > +    gPlatform->mMemoryPressureObserver = nullptr;
> > > +    gPlatform->mSkiaGlue = nullptr;
> > > +
> > > +    if (XRE_IsParentProcess()) {
> > > +      gPlatform->mVsyncSource->GetGlobalDisplay().DisableVsync();
> > 
> > Shouldn't this wait until the vsync thread dies?
> 
> That only happens during destruction since we want to reuse the thread
> normally. Since we're in shutdown, nothing should be enabling vsync again,
> so that should stop this crash as the thread will just be idle.

I don't understand what prevents us from deleteing gPlatform before the last call to GetPlatform() at https://hg.mozilla.org/releases/mozilla-beta/annotate/0723a0212f5e/gfx/thebes/gfxWindowsPlatform.cpp#l2824
?

Comment 11

[Mason Chang [Inactive] [:mchang]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #10)
> (In reply to Mason Chang [:mchang] from comment #9)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> > > Comment on attachment 8758423 [details] [diff] [review]
> > > Disable vsync before clearing VsyncSource
> > > 
> > > Review of attachment 8758423 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: gfx/thebes/gfxPlatform.cpp
> > > @@ +822,5 @@
> > > > +    gPlatform->mMemoryPressureObserver = nullptr;
> > > > +    gPlatform->mSkiaGlue = nullptr;
> > > > +
> > > > +    if (XRE_IsParentProcess()) {
> > > > +      gPlatform->mVsyncSource->GetGlobalDisplay().DisableVsync();
> > > 
> > > Shouldn't this wait until the vsync thread dies?
> > 
> > That only happens during destruction since we want to reuse the thread
> > normally. Since we're in shutdown, nothing should be enabling vsync again,
> > so that should stop this crash as the thread will just be idle.
> 
> I don't understand what prevents us from deleteing gPlatform before the last
> call to GetPlatform() at
> https://hg.mozilla.org/releases/mozilla-beta/annotate/0723a0212f5e/gfx/
> thebes/gfxWindowsPlatform.cpp#l2824
> ?

If we call DisableVsync before, that loop should exit here - https://hg.mozilla.org/releases/mozilla-beta/annotate/0723a0212f5e/gfx/thebes/gfxWindowsPlatform.cpp#l2812

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #11)
> If we call DisableVsync before, that loop should exit here -
> https://hg.mozilla.org/releases/mozilla-beta/annotate/0723a0212f5e/gfx/
> thebes/gfxWindowsPlatform.cpp#l2812

It seems like this can still race. If we begin the vsync loop just before and then shutdown code runs.

Comment 13

[Mason Chang [Inactive] [:mchang]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
> (In reply to Mason Chang [:mchang] from comment #11)
> > If we call DisableVsync before, that loop should exit here -
> > https://hg.mozilla.org/releases/mozilla-beta/annotate/0723a0212f5e/gfx/
> > thebes/gfxWindowsPlatform.cpp#l2812
> 
> It seems like this can still race. If we begin the vsync loop just before
> and then shutdown code runs.

But gPlatform will still exist since we don't null it out until gfxPlatform::Shutdown ends - https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp?from=gfxPlatform.cpp#860

Or we can stop referencing gfxPlatform in the vblank loop?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8758423 [details] [diff] [review]
Disable vsync before clearing VsyncSource

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

Let's just shutdown and wait for the thread to finish.

Comment 15

[Mason Chang [Inactive] [:mchang]]

Attachment: Add VsyncSource::Shutdown

Adds a VsyncSource::Shutdown which shutsdown the global display. Each global display::shutdown is the destructor code.

Comment 16

[Mason Chang [Inactive] [:mchang]]

Had to fix a few try errors due to typos, but try looks good now - https://hg.mozilla.org/try/rev/b7ce4d3d8efb9bfc9c2a4502c0fc8b0acb424e9c

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c82675873549
Add VsyncSource::Shutdown which shuts down global display. r=jrmuizel

Comment 18

[Mason Chang [Inactive] [:mchang]]

Attachment: Add VsyncSource::Shutdown

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c82675873549

Comment 20

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8760513 [details] [diff] [review]
Add VsyncSource::Shutdown

Approval Request Comment
[Feature/regressing bug #]: Bug 1256547
[User impact if declined]: User can have a shutdown crash
[Describe test coverage new/current, TreeHerder]: Try
[Risks and why]: Low, this just cleans up the vsync work a bit earlier.
[String/UUID change made/needed]: None

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8760513 [details] [diff] [review]
Add VsyncSource::Shutdown

Fix a shutdown crash, taking it.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dca560586c399c3ae3a7ce96d5c3681ee2cbf5b4
https://hg.mozilla.org/releases/mozilla-beta/rev/852b4aba1b7a2d3079be7287570214914c143a47

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'gfxPlatform::Init':
 - nightly(version 50):2 crashes from 2016-06-06.
 - aurora (version 49):6 crashes from 2016-06-07.
 - beta   (version 48):286 crashes from 2016-06-06.
 - release(version 47):5969 crashes from 2016-05-31.
 - esr    (version 45):1 crash from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       0       0       0       0       0
 - aurora        0       0       0       0       0       0       4
 - beta          1       1       2       2      13      99     131
 - release     369     428     922     989    1064    1001     983
 - esr           0       0       0       0       0       0       0

Affected platforms: Windows, Linux