1554438 - Crash should pref "gfx.direct3d11.use-double-buffering" be changed

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jean-Yves Avenard [:jya]]

Common case:
Start Firefox with the pref "gfx.direct3d11.use-double-buffering" set to false.
then set it to true. Shutdown.

It will assert on debug build, or hang

The reason for this is in:
https://searchfox.org/mozilla-central/source/gfx/ipc/GPUParent.cpp#272
if the pref is true we call:
widget::WinCompositorWindowThread::Start();

and when shutting down:
https://searchfox.org/mozilla-central/source/gfx/ipc/GPUParent.cpp#527
it calls
widget::WinCompositorWindowThread::ShutDown();

however, WinCompositorWindowThread::ShutDown()
assert that https://searchfox.org/mozilla-central/source/widget/windows/WinCompositorWindowThread.cpp#54
sWinCompositorWindowThread is set.

This is only true if widget::WinCompositorWindowThread::Start() has been called.

More troublesome case (this caused the backout of bug 1550422).
In all.js the pref gfx.direct3d11.use-double-buffering is set to have a default value of true.

But in gfxPrefs it is set to have a default value of false.

gfxPrefs was always initialised well after Preferences.
For StaticPrefs however it occurs earlier. We have an interval during which this StaticPrefs will be initialised to its default (false), and then all.js is read which would reset the value to true.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248345210&repo=try&lineNumber=1923
So we have a race where we see the GPUParent being created with the pref value set to false, and then at shutdown it's false.

And this cause the assertion.

I haven't determined on why we get a different values for the pref on this run. There's nothing in the code that could switch the pref.

So for now will just address this situation, and prevent the pref changing to cause a problem

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1554438 - P1. Don't rely on a live pref to determine order of actions. r?Bas,sotaro

The value of the pref may change between start and shutdown.

So we shouldn't rely on that pref to determine if we need to shutdown on thread started earlier if that pref was true.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1554438 - P2. Only shutdown thread if it was started. r?Bas,sotaro,mattwoodrow

It is theorically possible that the WinCompositorWindowThread failed to start. Should this happen, attempting to shut it down will cause shutdown to hang forever.

Depends on D32601

Comment 3

[Jean-Yves Avenard [:jya]]

We also have a similar case that cause bug 1438408.

If gfxVars::UseWebRender() change its value between the time GPUParent was started and then shutdown, we could attempt to call widget::WinCompositorWindowThread::ShutDown() when widget::WinCompositorWindowThread::Start() didn't.

Causing a crash or hang.

I believe the code should be changed such that we shouldn't assume that the start conditions are identical to the shutdown one. And we shutdown thread whenever they exists and not based on assuming they were started earlier (with invalid assumptions at that)

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1554438 - P3. Shutdown threads whenever they exist. r?Bas,sotaro,mattwoodrow

We only attempted to shutdow WinCompositorWindowThread if the WebRender thread hadn't not been started.

However, it is possible that the value of gfxVars::UseWebRender() changed since GPUParent::Init got called.

So don't assume anything, shutdown the thread if it still exists.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e40827b3a352
P1. Don't rely on a live pref to determine order of actions. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/5247954abf59
P2. Only shutdown thread if it was started. r=sotaro

Comment 6

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/e40827b3a352
https://hg.mozilla.org/mozilla-central/rev/5247954abf59

Comment 7

[Jean-Yves Avenard [:jya]]

I think I've figured out what was going on.
But that's an even weirder cause that I could have imagined.

So I've confirmed through try that the pref gfx.direct3d11.use-double-buffering doesn't change during the lifetime of the GPUParent.

However, during GPUParent::RecvInit() IsWin10OrLater() returns true.

But during GPUParent::ActorDestroy IsWin10OrLater() returns false !

Hence why this entire bug occurred in the first place as the code tested for gfxPrefs::Direct3D11UseDoubleBuffering() && IsWin10OrLater()

Will open a new bug about this.

wierd becomes wierder

Comment 8

[Jean-Yves Avenard [:jya]]

Will close when P3 gets r+ and lands.

Comment 9

[Jean-Yves Avenard [:jya]]

Actually, earlier comment was totally wrong.

Here is why it used to work, but following the move to StaticPrefs it no longer does.

gfxPrefs is initialised in GPUParent::Init()
https://searchfox.org/mozilla-central/rev/4606c7974a68cab416c038acaedcae49eed93822/gfx/ipc/GPUParent.cpp#113

where gfxPrefs::Singleton() is called.

As preferences weren't initialised in the GPU process, the default value for gfxPrefs of gfx.direct3d11.use-double-buffering was false.

The actual settings of the gfxPrefs values was done in GPUParent::RecvInit(): https://searchfox.org/mozilla-central/rev/4606c7974a68cab416c038acaedcae49eed93822/gfx/ipc/GPUParent.cpp#180

Now with the work done in bug 1550422, StaticPrefs is initialised very early and set at the same time as the GPU process is started.
So the default value of StaticPrefs::Direct3D11UseDoubleBuffering() is true.

In all those tests, GPUParent::RecvInit is never called, and as such neither wr::RenderThread::Start() is called, nor widget::WinCompositorWindowThread::Start().

When GPUParent::ActorShutdown() is called.
gfxPrefs::Direct3D11UseDoubleBuffering() would still return false, and as such didn't attempt to call widget::WinCompositorWindowThread::Shutdown() (https://searchfox.org/mozilla-central/rev/4606c7974a68cab416c038acaedcae49eed93822/gfx/ipc/GPUParent.cpp#522)

As wr::RenderThread::Get() would return nullptr, we fall on the test:
https://searchfox.org/mozilla-central/rev/4606c7974a68cab416c038acaedcae49eed93822/gfx/ipc/GPUParent.cpp#526
"else if (gfxPrefs::Direct3D11UseDoubleBuffering() && IsWin10OrLater()) {"

which with gfxPrefs's default would be false.
But StaticPrefs::Direct3D11UseDoubleBuffering() && IsWin10OrLater() instead returned true and would attempt to shutdown the thread that was never created.

And so: boom

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 9067583 [details]
Bug 1554438 - P1. Don't rely on a live pref to determine order of actions. r?Bas,sotaro

Beta/Release Uplift Approval Request

• User impact if declined: If the user change the value of the pref gfx.direct3d11.use-double-buffering while firefox is running.
  Close firefox would make it hang forever.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Disable webrender if it's enabled.
  Start firefox.
  Change the preference gfx.direct3d11.use-double-buffering to false.
  Close firefox.

It will hang or assert if it's a debug build

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We only attempt to shutdown code if it got started
• String changes made/needed:

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd4d1986773e
P3. Shutdown threads whenever they exist. r=mattwoodrow

Comment 12

[Julien Cristau [:jcristau]]

Is that pref ever changed by our code or UI? If not I don't think this warrants uplift.

Comment 13

[Oana Pop-Rus]

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

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to Julien Cristau [:jcristau] from comment #12)

Is that pref ever changed by our code or UI? If not I don't think this warrants uplift.

it's a visible pref in about:config

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9067583 [details]
Bug 1554438 - P1. Don't rely on a live pref to determine order of actions. r?Bas,sotaro

Doesn't seem worth worrying about then.

Comment 16

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using an older version of Nightly from 25-05-2019 on Windows 10 x64.
I retested everything using the latest Nightly 69.0a1 on Windows 10 x64, Ubuntu 18.04 x64 and macOS 10.13. The bug doesn't seem to be reproducing anymore.