Closed Bug 1316931 Opened 3 years ago Closed 3 years ago

GLContextProviderWGL uses gfxUtils::ThreadSafeGetFeatureStatus but violates the assumption of having a worker thread

Categories

(Core :: Graphics, defect, P3)

52 Branch
All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

The code at [1] can return null and cause a crash. In particular I was seeing this in the Webrender branch (on Windows) because we try to initialize the GL library on the compositor thread and it calls this code from there [2].

On IRC dvander suggested that we should store the information as to whether or not the FEATURE_DX_INTEROP2 feature is available into gfxConfig during startup. (Side note: if we do this can also remove the IsFeatureBlacklisted function entirely as it is not used anywhere else).

[1] http://searchfox.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp#1472
[2] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/gfx/gl/GLContextProviderWGL.cpp#223
Priority: -- → P3
Whiteboard: [gfx-noted]
Looks like this code was added relatively recently, in bug 1287653. Also there is a mismatch between gfxPrefs [1] and all.js [2] for the gl.ignore-dx-interop2-blacklist pref name - in gfxPrefs the pref is missing the "gl." prefix. jgilbert, is this intentional?

[1] https://hg.mozilla.org/mozilla-central/rev/3167ef50f1ff#l2.12
[2] https://hg.mozilla.org/mozilla-central/rev/3167ef50f1ff#l5.12
Blocks: 1287653
Flags: needinfo?(jgilbert)
Summary: gfxUtils::ThreadSafeGetFeatureStatus assumes it has access to a worker thread → GLContextProviderWGL uses gfxUtils::ThreadSafeGetFeatureStatus but violates the assumption of having a worker thread
Also, just to be clear, the Webrender branch is creating a GLContextProviderWGL for the chrome window on the compositor thread, which AFAIK can't happen in production Firefox, so it's not like this bug can be hit normally. The pref naming might still be a real bug.
Blocks: 1314906
No longer blocks: webrender
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> Looks like this code was added relatively recently, in bug 1287653. Also
> there is a mismatch between gfxPrefs [1] and all.js [2] for the
> gl.ignore-dx-interop2-blacklist pref name - in gfxPrefs the pref is missing
> the "gl." prefix. jgilbert, is this intentional?
> 
> [1] https://hg.mozilla.org/mozilla-central/rev/3167ef50f1ff#l2.12
> [2] https://hg.mozilla.org/mozilla-central/rev/3167ef50f1ff#l5.12

I'm pretty sure those were supposed to match, sorry!
Flags: needinfo?(jgilbert)
I split that off to bug 1317068.
Comment on attachment 8810579 [details]
Bug 1316931 - Put DX interop2 status into gfxVars.

https://reviewboard.mozilla.org/r/92856/#review92868
Attachment #8810579 - Flags: review?(dvander) → review+
Comment on attachment 8810579 [details]
Bug 1316931 - Put DX interop2 status into gfxVars.

https://reviewboard.mozilla.org/r/92856/#review92928
Attachment #8810579 - Flags: review?(jgilbert) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6f4bb5cf321
Put DX interop2 status into gfxVars. r=dvander,jgilbert
https://hg.mozilla.org/mozilla-central/rev/e6f4bb5cf321
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Kats, is that something we want to let ride the train? Thanks
Flags: needinfo?(bugmail)
Yeah we can let it ride the trains.
Flags: needinfo?(bugmail)
You need to log in before you can comment on or make changes to this bug.