1769807 - Crash in [@ nsObserverService::AddObserver]

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

~700 crashes for Firefox 100.0 and 100.0.1 on various Window versions. Also seeing this signature for older version but with lower crash volume.

Crash report: https://crash-stats.mozilla.org/report/index/53d540bd-57da-407e-9239-f52940220517

MOZ_CRASH Reason: MOZ_CRASH(Using observer service off the main thread!)

Top 10 frames of crashing thread:

0 xul.dll nsObserverService::AddObserver xpcom/ds/nsObserverService.cpp:211
1 xul.dll mozilla::EnsureWin32kInitialized toolkit/xre/nsAppRunner.cpp:815
2 xul.dll mozilla::GetWin32kLockdownState toolkit/xre/nsAppRunner.cpp:855
3 xul.dll mozilla::GetContentWin32kLockdownState security/sandbox/common/SandboxSettings.cpp:110
4 xul.dll mozilla::SandboxBroker::SetSecurityLevelForContentProcess security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp:692
5 xul.dll mozilla::ipc::WindowsProcessLauncher::DoSetup ipc/glue/GeckoChildProcessHost.cpp:1375
6 xul.dll mozilla::ipc::BaseProcessLauncher::PerformAsyncLaunch ipc/glue/GeckoChildProcessHost.cpp:1000
7 xul.dll mozilla::detail::ProxyRunnable<mozilla::MozPromise<mozilla::ipc::LaunchResults, mozilla::ipc::LaunchError, 1>, RefPtr<mozilla::MozPromise<mozilla::ipc::LaunchResults, mozilla::ipc::LaunchError, 1> >  xpcom/threads/MozPromise.h:1538
8 xul.dll mozilla::TaskQueue::Runner::Run xpcom/threads/TaskQueue.cpp:196
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1181

Comment 1

[Bob Owen (:bobowen)]

Sounds like we might want to do what was suggested in bug 1764544 comment 19 for this.
So we always initialize on the main thread.

Comment 2

[Chris Martin [:cmartin]]

Attachment: Bug 1769807 - Ensure Win32k state initialized before content launch. r=nalexander!

Win32k Lockdown state must be initialized on the main thread, but currently
a process launcher may be the first thing to read it on the IPC Thread

Initializing Win32k Lockdown state also relies on the gfxPlatform being
initialized, but that also isn't explicit anywhere.

This patch ensures both things are true: Always ensure that Win32k State is
initialized before queuing a process launch to the IPC Thread, and always
ensure that gfxPlatform is initialized before attempting to read the
gfx state.

Comment 3

[Chris Martin [:cmartin]]

Based on this comment by Brian Grinstead, it looks like the patch above fixes this problem on Pine. I think it's reasonable to assume it will fix this as well, even though afaik we don't have a repro for this.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0afbb1e34fe3
Ensure Win32k state initialized before content launch r=bobowen

Comment 5

[Marian-Vasile Laza]

Backed out changeset 0afbb1e34fe3 (Bug 1769807) for causing bc failures on browser_xpcom_graph_wait.js.
Backout link
Push with failures <--> bc2
Failure Log

Comment 6

[Chris Martin [:cmartin]]

Issue seems to be that the test isn't happy that some of the components are now starting up earlier, but they don't appear on the whitelist of stuff that's allowed to do that.

I updated the patch, and just waiting for review from Dave Townsend and I'll try landing it again.

Comment 7

[Nick Alexander :nalexander [he/him]]

This all looks good from me.

Bob: can you land this? There's no reason to wait for Chris that I can see from the patch, but maybe you're aware of something.

Comment 8

[Bob Owen (:bobowen)]

(In reply to Nick Alexander :nalexander [he/him] from comment #7)

This all looks good from me.

Bob: can you land this? There's no reason to wait for Chris that I can see from the patch, but maybe you're aware of something.

Thanks, I've queued it up.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dc2f0c648e4
Ensure Win32k state initialized before content launch r=bobowen,nalexander

Comment 10

[Cristian Tuns]

Backed out for causing mochitest failures on browser_xpcom_graph_wait.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/backgroundtasks/tests/browser/browser_xpcom_graph_wait.js | AfterRunBackgroundTaskNamed: all services allowlist entries should have been used - 3 deepEqual 0 - JS frame :: chrome://mochitests/content/browser/toolkit/components/backgroundtasks/tests/browser/browser_xpcom_graph_wait.js :: test_xpcom_graph_wait :: line 410

Comment 11

[Bob Owen (:bobowen)]

Ah, looks like those extra allow list entries need to be Windows only.

Comment 12

[Bob Owen (:bobowen)]

Think this'll work:
https://treeherder.mozilla.org/jobs?repo=try&revision=63928fc71537afc8c65f0654896476e6934d4e82

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eabf99800cc9
Ensure Win32k state initialized before content launch. r=nalexander

Comment 14

[Natalia Csoregi [:nataliaCs]]

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

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 9277317 [details]
Bug 1769807 - Ensure Win32k state initialized before content launch. r=nalexander!

Beta/Release Uplift Approval Request

• User impact if declined: Some users will continue to experience start up crashes.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This is a very simple patch, only putting medium because it does change main process start up a little, which can be a bit fragile.
• String changes made/needed: None
• Is Android affected?: No

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9277317 [details]
Bug 1769807 - Ensure Win32k state initialized before content launch. r=nalexander!

Approved for 101.0rc1.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/62b172f7d5f0