Closed Bug 1151650 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox-esr38 --- fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: mccr8, Assigned: nical)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [fixed by bug 1151713][adv-main39+][adv-esr38.1+])

Attachments

(1 file, 2 obsolete files)

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
Flags: needinfo?(wmccloskey)
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."
Flags: needinfo?(wmccloskey) → needinfo?(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?
Flags: needinfo?(mstange) → needinfo?(nical.bugzilla)
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.
Keywords: sec-moderate
(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.
Assignee: nobody → nical.bugzilla
Flags: needinfo?(nical.bugzilla)
Attachment #8590339 - Flags: review?(jmuizelaar) → review+
Sorry, the previous patch was incomplete.
Attachment #8590339 - Attachment is obsolete: true
Attachment #8590340 - Flags: review?(jmuizelaar)
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?
Attachment #8590340 - Flags: feedback-
May be enough to just move the call to InitLayersAccelerationPrefs() down, just before InitLayersIPC()?
(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.
Consider bug 1151713 when patching this, appears related.
(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.
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.
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?
Flags: needinfo?(continuation)
(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.
Flags: needinfo?(continuation)
Attached patch v2Splinter Review
Same patch with the initialization moved down a few lines, so that GfxInfo is still initialized at the same place as before the patch.
Attachment #8590340 - Attachment is obsolete: true
Attachment #8590340 - Flags: review?(jmuizelaar)
Attachment #8593265 - Flags: review?(jmuizelaar)
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.
Attachment #8593265 - Flags: review?(jmuizelaar) → review+
Bug 1151713 landed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Is this something we should fix for Fx39 or any of the B2G/ESR releases?
Flags: needinfo?(nical.bugzilla)
Target Milestone: --- → mozilla40
(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.
Flags: needinfo?(nical.bugzilla)
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?
Hi Nicolas,
Please raise approval for b2g-37, thanks!
Flags: needinfo?(jocheng) → needinfo?(nical.bugzilla)
(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.
Flags: needinfo?(nical.bugzilla)
Whiteboard: [fixed by bug 1151713]
Whiteboard: [fixed by bug 1151713] → [fixed by bug 1151713][adv-main39+][adv-esr38.1+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.