Closed
Bug 1151650
Opened 10 years ago
Closed 10 years ago
GfxInfoBase::GetFeatureStatus() sends an IPDL message off the main thread
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: mccr8, Assigned: nical)
References
Details
(Keywords: sec-moderate, Whiteboard: [fixed by bug 1151713][adv-main39+][adv-esr38.1+])
Attachments
(1 file, 2 obsolete files)
|
1.59 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
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
| Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(wmccloskey)
| Reporter | ||
Comment 1•10 years ago
|
||
Another similar crash:
https://crash-stats.mozilla.com/report/index/a17ccf78-5daf-414a-a121-f84302150404
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)
Comment 3•10 years ago
|
||
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)
| Reporter | ||
Comment 4•10 years ago
|
||
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
| Assignee | ||
Comment 5•10 years ago
|
||
(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)
| Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8590339 -
Flags: review?(jmuizelaar)
Updated•10 years ago
|
Attachment #8590339 -
Flags: review?(jmuizelaar) → review+
| Assignee | ||
Comment 7•10 years ago
|
||
Sorry, the previous patch was incomplete.
Attachment #8590339 -
Attachment is obsolete: true
Attachment #8590340 -
Flags: review?(jmuizelaar)
Comment 8•10 years ago
|
||
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-
Comment 9•10 years ago
|
||
May be enough to just move the call to InitLayersAccelerationPrefs() down, just before InitLayersIPC()?
| Assignee | ||
Comment 10•10 years ago
|
||
(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•10 years ago
|
||
Consider bug 1151713 when patching this, appears related.
| Assignee | ||
Comment 12•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
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)
| Reporter | ||
Comment 15•10 years ago
|
||
(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)
| Assignee | ||
Comment 16•10 years ago
|
||
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 17•10 years ago
|
||
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+
| Assignee | ||
Comment 18•10 years ago
|
||
Bug 1151713 landed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 19•10 years ago
|
||
Is this something we should fix for Fx39 or any of the B2G/ESR releases?
| Assignee | ||
Comment 20•10 years ago
|
||
(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)
Comment 21•10 years ago
|
||
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?
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1:
--- → ?
status-b2g-v2.2:
--- → affected
status-firefox38:
--- → wontfix
status-firefox38.0.5:
--- → wontfix
status-firefox39:
--- → affected
status-firefox-esr38:
--- → affected
Flags: needinfo?(jocheng)
Comment 22•10 years ago
|
||
Hi Nicolas,
Please raise approval for b2g-37, thanks!
Flags: needinfo?(jocheng) → needinfo?(nical.bugzilla)
| Assignee | ||
Comment 23•10 years ago
|
||
(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)
| Reporter | ||
Updated•10 years ago
|
Whiteboard: [fixed by bug 1151713]
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Whiteboard: [fixed by bug 1151713] → [fixed by bug 1151713][adv-main39+][adv-esr38.1+]
Updated•10 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•