Crash in [@ nsObserverService::AddObserver]
Categories
(Core :: Security: Process Sandboxing, defect, P1)
Tracking
()
People
(Reporter: aryx, Assigned: cmartin)
References
(Regression)
Details
(Keywords: crash, regression, topcrash-startup)
Crash Data
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
~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
Updated•2 years ago
|
Updated•2 years ago
|
Comment 1•2 years ago
|
||
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.
Assignee | ||
Comment 2•2 years ago
|
||
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.
Updated•2 years ago
|
Assignee | ||
Comment 3•2 years ago
•
|
||
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.
Updated•2 years ago
|
Comment 5•2 years ago
|
||
Backed out changeset 0afbb1e34fe3 (Bug 1769807) for causing bc failures on browser_xpcom_graph_wait.js.
Backout link
Push with failures <--> bc2
Failure Log
Assignee | ||
Comment 6•2 years ago
|
||
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.
Assignee | ||
Updated•2 years ago
|
Updated•2 years ago
|
Comment 7•2 years ago
|
||
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•2 years ago
|
||
(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 10•2 years ago
|
||
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•2 years ago
|
||
Ah, looks like those extra allow list entries need to be Windows only.
Comment 12•2 years ago
|
||
Updated•2 years ago
|
Comment 13•2 years ago
|
||
Comment 14•2 years ago
|
||
bugherder |
Comment 15•2 years ago
|
||
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•2 years ago
|
||
Comment on attachment 9277317 [details]
Bug 1769807 - Ensure Win32k state initialized before content launch. r=nalexander!
Approved for 101.0rc1.
Comment 17•2 years ago
|
||
bugherder uplift |
Description
•