Closed Bug 1769807 Opened 1 month ago Closed 1 month ago

Crash in [@ nsObserverService::AddObserver]

Categories

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

Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
102 Branch
Tracking Status
firefox-esr91 --- unaffected
firefox100 + wontfix
firefox101 + fixed
firefox102 + fixed

People

(Reporter: aryx, Assigned: cmartin)

References

(Regression)

Details

(Keywords: crash, regression, topcrash-startup)

Crash Data

Attachments

(1 file)

~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
Flags: needinfo?(bobowencode)
Has Regression Range: --- → yes
See Also: → 1764544

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.

Flags: needinfo?(bobowencode)

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.

Assignee: nobody → cmartin
Status: NEW → ASSIGNED

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.

Priority: -- → P1
Pushed by cmartin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0afbb1e34fe3
Ensure Win32k state initialized before content launch r=bobowen

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

Flags: needinfo?(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.

Flags: needinfo?(cmartin) → needinfo?(dtownsend)
Flags: needinfo?(dtownsend) → needinfo?(nalexander)

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.

Flags: needinfo?(nalexander) → needinfo?(bobowencode)

(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.

Flags: needinfo?(bobowencode)
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9dc2f0c648e4
Ensure Win32k state initialized before content launch r=bobowen,nalexander

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
Flags: needinfo?(cmartin)

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

Flags: needinfo?(cmartin)
Attachment #9277317 - Attachment description: Bug 1769807 - Ensure Win32k state initialized before content launch → Bug 1769807 - Ensure Win32k state initialized before content launch. r=nalexander!
Pushed by bobowencode@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eabf99800cc9
Ensure Win32k state initialized before content launch. r=nalexander
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 102 Branch

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
Attachment #9277317 - Flags: approval-mozilla-beta?

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

Approved for 101.0rc1.

Attachment #9277317 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1764544
You need to log in before you can comment on or make changes to this bug.