1772089 - Crash in [@ shutdownhang | NtQueryDirectoryFileEx]

Status: RESOLVED FIXED

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

Description

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

Crash signature only observed for Windows 10 and 11. It's not new but they frequency changed from ~100 crash reports for Firefox 99 to ~400 for Firefox 100.

Crash report: https://crash-stats.mozilla.org/report/index/daffbce8-81d4-4ca7-b5c3-3d2f50220601

MOZ_CRASH Reason: Shutdown hanging at step AppShutdown. Something is blocking the main-thread.

Top 10 frames of crashing thread:

0 ntdll.dll NtQueryDirectoryFileEx 
1 KERNELBASE.dll FindNextFileW 
2 xul.dll nsDirEnumerator::HasMoreElements xpcom/io/nsLocalFileWin.cpp:790
3 xul.dll nsLocalFile::Remove xpcom/io/nsLocalFileWin.cpp:2339
4 xul.dll mozilla::net::CacheFileIOManager::SyncRemoveDir netwerk/cache2/CacheFileIOManager.cpp:3996
5 xul.dll mozilla::net::CacheFileIOManager::SyncRemoveAllCacheFiles netwerk/cache2/CacheFileIOManager.cpp:4012
6 xul.dll static mozilla::net::CacheFileIOManager::Shutdown netwerk/cache2/CacheFileIOManager.cpp:1181
7 xul.dll mozilla::net::CacheObserver::Observe netwerk/cache2/CacheObserver.cpp:222
8 xul.dll nsObserverList::NotifyObservers xpcom/ds/nsObserverList.cpp:70
9 xul.dll nsObserverService::NotifyObservers xpcom/ds/nsObserverService.cpp:291

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Should be fixed by bug 1786256.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

The last few crashes with this signature are caused by DeleteDirIfExists
This was added in bug 1375863.

Crash report: https://crash-stats.mozilla.org/report/index/a9e55446-c293-4fff-934c-b242d0230120

3	xul.dll	OpenDir(nsTString<char16_t> const&, nsDir**)	xpcom/io/nsLocalFileWin.cpp:683	inlined
3	xul.dll	nsDirEnumerator::Init(nsIFile*)	xpcom/io/nsLocalFileWin.cpp:782	cfi
4	xul.dll	nsLocalFile::Remove(bool)	xpcom/io/nsLocalFileWin.cpp:2338	cfi
5	xul.dll	DeleteDirIfExists(nsIFile*)	toolkit/xre/nsXREDirProvider.cpp:774	inlined
5	xul.dll	nsXREDirProvider::DoShutdown()	toolkit/xre/nsXREDirProvider.cpp:1013	cfi
6	xul.dll	ScopedXPCOMStartup::~ScopedXPCOMStartup()	toolkit/xre/nsAppRunner.cpp:2061	cfi

Moving to Security: Process sandboxing to investigate.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

That call originated in bug 1270018, if I'm reading the history correctly. (There's been some noise since then with changes to configure flags, NPAPI removal, etc.)

Edited to add: “that” call to DeleteDirIfExists; there was earlier code to do something similar, so maybe bug 1270018 is also too recent.

Comment 4

[Bob Owen (:bobowen)]

Hi :Jamie - I'm looking to remove the creation for the content temp dir on Windows, but I noticed this use of it for the InterceptorLog.

It seems that this is only enabled with the env var MOZ_MSCOM_LOG_BASENAME.
If I change this code to use the normal temp dir like the parent process then it will still work in DEBUG builds, because we give access to that dir for other reasons.
It won't work in the content process for opt builds unless you also disable the content process sandbox, for example by setting MOZ_DISABLE_CONTENT_SANDBOX.
Would that be OK, I don't know if this logging is used often?

Comment 5

[James Teh [:Jamie]]

It isn't used that often now and it won't be used at all once we ship Cache the World within the next few months. I think you can go ahead.

For reference, while we were still developing our old architecture, it was previously sometimes useful with the sandbox enabled because the sandbox can change COM behaviour in ways that are otherwise hard to diagnose; e.g. whether we can access a particular COM proxy.

Comment 6

[Bob Owen (:bobowen)]

Attachment: Bug 1772089 p1: Remove SetUpSandboxEnvironment from mozilla::dom::ContentProcess. r=handyman!

This removes includes that were no longer needed on macOS as well.

Comment 7

[Bob Owen (:bobowen)]

Attachment: Bug 1772089 p2: Switch to OS temp in InterceptorLog. r=Jamie!

This means that the content process sandbox would need to be disabled on an opt
build for this logging to work.

Depends on D168591

Comment 8

[Bob Owen (:bobowen)]

Attachment: Bug 1772089 p3: Remove content temp dir from PathUtils. r=barret!

This changes tempDir to be the OS temp to prevent churn, because it has many
more uses than osTempDir.

Depends on D168593

Comment 9

[Bob Owen (:bobowen)]

Attachment: Bug 1772089 p4: Remove content temp dir from Windows sandbox policy rules. r=handyman!

This also removes sRoamingAppDataDir as it is no longer used.

Depends on D168594

Comment 10

[Bob Owen (:bobowen)]

Attachment: Bug 1772089 p5: Remove content temp dir from Windows and masOS. r=jld!,haik!,glandium!

This defines MOZ_CONTENT_TEMP_DIR to make it easier to track this in the code.
It also uses this to guard some Linux specific uses.

Depends on D168595

Comment 11

[Bob Owen (:bobowen)]

Attachment: Bug 1772089 p6: Fix content sandbox file system test. r=haik!

The content temp dir is no longer defined for macOS and Windows.

Depends on D168596

Comment 12

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/jobs?repo=try&revision=36818084465644e0956ca721c48c203e573308cc

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf38c171d012
p1: Remove SetUpSandboxEnvironment from mozilla::dom::ContentProcess. r=handyman
https://hg.mozilla.org/integration/autoland/rev/259ebb3a8f82
p2: Switch to OS temp in InterceptorLog. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/5851b2b1d419
p3: Remove content temp dir from PathUtils. r=barret
https://hg.mozilla.org/integration/autoland/rev/c12b40d149b2
p4: Remove content temp dir from Windows sandbox policy rules. r=handyman
https://hg.mozilla.org/integration/autoland/rev/0c812cd61db6
p5: Remove content temp dir from Windows and masOS. r=jld,haik,glandium
https://hg.mozilla.org/integration/autoland/rev/9696340d342d
p6: Fix content sandbox file system test. r=haik

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/cf38c171d012
https://hg.mozilla.org/mozilla-central/rev/259ebb3a8f82
https://hg.mozilla.org/mozilla-central/rev/5851b2b1d419
https://hg.mozilla.org/mozilla-central/rev/c12b40d149b2
https://hg.mozilla.org/mozilla-central/rev/0c812cd61db6
https://hg.mozilla.org/mozilla-central/rev/9696340d342d