1151650 - GfxInfoBase::GetFeatureStatus() sends an IPDL message off the main thread

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Andrew McCreight [:mccr8]]

This is similar to bug 1146416.  It looks like  	GfxInfoBase::GetFeatureStatus(int, int*) is being called off the main thread, which makes us send an IPDL message to the parent.  While this is happening, we have a script blocker which ends up capturing some main thread stuff, and running it off the main thread.

https://crash-stats.mozilla.com/report/index/3c3806c2-9cca-4914-9eac-ec8a82150402#allthreads

Comment 1

[Andrew McCreight [:mccr8]]

Another similar crash:
https://crash-stats.mozilla.com/report/index/a17ccf78-5daf-414a-a121-f84302150404

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Maybe you can help here Markus? GetFeatureStatus is called by InitLayersAccelerationPrefs, which asserts that we're on the main thread and also has this comment: "If this is called for the first time on a non-main thread, we're screwed."

Comment 3

[Markus Stange [:mstange]]

The ImageClientSingle::UpdateImage looks like off main thread video - Nical, can we make sure that we call InitLayersAccelerationPrefs before we initialize the image bridge thread?

Comment 4

[Andrew McCreight [:mccr8]]

Seems hard to control initialization of some graphics stuff, plus we're probably going to hit an assert here, so I'm going to set this to sec-moderate.

Comment 5

[Nicolas Silva [:nical]]

(In reply to Markus Stange [:mstange] from comment #3)
> The ImageClientSingle::UpdateImage looks like off main thread video - Nical,
> can we make sure that we call InitLayersAccelerationPrefs before we
> initialize the image bridge thread?

Yeah.

Comment 6

[Nicolas Silva [:nical]]

Attachment: init layers prefs in gfxPlatform to avoid doing it off the main thread

Comment 7

[Nicolas Silva [:nical]]

Attachment: init layers prefs in gfxPlatform to avoid doing it off the main thread

Sorry, the previous patch was incomplete.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8590340 [details] [diff] [review]
init layers prefs in gfxPlatform to avoid doing it off the main thread

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

::: gfx/thebes/gfxPlatform.cpp
@@ +466,5 @@
>      // Initialize the preferences by creating the singleton.
>      gfxPrefs::GetSingleton();
> +    // Initialize the these prefs now to avoid doing it off the main thread
> +    // by accident.
> +    InitLayersAccelerationPrefs();

Is this too early?  Inside of InitLayersAccelerationPrefs(), we check if different features are supported, does that not need gfxWindowsPlatform created, which only happens afterwards?

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

May be enough to just move the call to InitLayersAccelerationPrefs() down, just before InitLayersIPC()?

Comment 10

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #9)
> May be enough to just move the call to InitLayersAccelerationPrefs() down,
> just before InitLayersIPC()?

Good point, I'll update the patch.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Consider bug 1151713 when patching this, appears related.

Comment 12

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Is this too early?  Inside of InitLayersAccelerationPrefs(), we check if
> different features are supported, does that not need gfxWindowsPlatform
> created, which only happens afterwards?

Turns out this is fine. InitLayersAccelerationPrefs only needs gfxInfo to be usable. The latter is lazily initialized (and would be initialized at the latest a few lines below otherwise). It's still better to move InitLayersAccelerationPrefs down as you suggested so that the comment about when we initialize gfxInfo still holds true.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Nical, I took your patch and pushed to try under bug 1151713, as that appears to be the same problem, and it's a non-security bug.  Let's see how green it ends up.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc4f5622920f

Andrew, can you test with this try build, or let me know how to reproduce this?  If this fixes it, we should return it as a fix for bug 1151713?

Comment 15

[Andrew McCreight [:mccr8]]

(In reply to Milan Sreckovic [:milan] from comment #14)
> Andrew, can you test with this try build, or let me know how to reproduce
> this?  If this fixes it, we should return it as a fix for bug 1151713?

Sorry, I have no test case, I just saw a single crash on crash-stats.

Comment 16

[Nicolas Silva [:nical]]

Attachment: v2

Same patch with the initialization moved down a few lines, so that GfxInfo is still initialized at the same place as before the patch.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8593265 [details] [diff] [review]
v2

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

We can carry Jeff's review, and this already passed try on bug 1151713, so I'm just going to land it there.

Comment 18

[Nicolas Silva [:nical]]

Bug 1151713 landed

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is this something we should fix for Fx39 or any of the B2G/ESR releases?

Comment 20

[Nicolas Silva [:nical]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> Is this something we should fix for Fx39 or any of the B2G/ESR releases?

The fix is trivial enough that we can easily apply it to these branches and without risk. I don't know how much of a pain deploying something in B2G/ESR is, I would probably not deploy a new build just for this patch if the process is heavy.

I just checked and the patch in bug 1151713 applies cleanly to beta, I asked for approval there.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backporting isn't a huge deal. Sounds like it may be worth nominating for ESR38/b2g37 as well as a stability fix. Josh, do you want this for b2g34 as well?

Comment 22

[Josh Cheng [:josh]]

Hi Nicolas,
Please raise approval for b2g-37, thanks!

Comment 23

[Nicolas Silva [:nical]]

(In reply to Josh Cheng [:josh] from comment #22)
> Hi Nicolas,
> Please raise approval for b2g-37, thanks!

I asked for approval for both b2g-37 and esr in bug 1151713.