1816857 - Crash in [@ mozilla::ipc::UtilityProcessHost::RejectPromise]

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/fb4e53c4-a81c-41e9-ae12-eb2230230214

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0  ntdll.dll  RtlAcquireSRWLockExclusive  
1  xul.dll  mozilla::OffTheBooksMutex::Lock  xpcom/threads/Mutex.h:65
1  xul.dll  mozilla::detail::BaseAutoLock<mozilla::Mutex&>::BaseAutoLock  xpcom/threads/Mutex.h:236
1  xul.dll  mozilla::MozPromise<bool, nsresult, 0>::Private::Reject<const nsresult&>  xpcom/threads/MozPromise.h:1233
2  xul.dll  mozilla::ipc::UtilityProcessHost::RejectPromise  ipc/glue/UtilityProcessHost.cpp:316
2  xul.dll  mozilla::ipc::UtilityProcessHost::Shutdown  ipc/glue/UtilityProcessHost.cpp:232
2  xul.dll  mozilla::ipc::UtilityProcessManager::DestroyProcess  ipc/glue/UtilityProcessManager.cpp:439
3  xul.dll  mozilla::ipc::UtilityProcessManager::LaunchProcess  ipc/glue/UtilityProcessManager.cpp:160
4  xul.dll  mozilla::ipc::UtilityProcessManager::StartUtility  ipc/glue/UtilityProcessManager.cpp:229
4  xul.dll  mozilla::ipc::UtilityProcessManager::StartProcessForRemoteMediaDecoding  ipc/glue/UtilityProcessManager.cpp:292

Note: there are no crashes under this signatures because we first need to land bug 1816846 for them to appear, the stack above however represents the problem. It seems that the lock contained within mLaunchPromise is set to NULL which causes the crash.

Comment 1

[Jens Stutte [:jstutte]]

This seems not content process related, but sandboxing process. I think this can happen since bug 1753424 landed?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Nika, can you take a look at this while Alexandre is out?

Comment 3

[:gerard-majax]

(In reply to Gabriele Svelto [:gsvelto] from comment #0)

[...]

Note: there are no crashes under this signatures because we first need to land bug 1816846 for them to appear, the stack above however represents the problem. It seems that the lock contained within mLaunchPromise is set to NULL which causes the crash.

I'm curious how this can be. Since the stack mention Shutdown I'm wondering if we could not be just missing a mLaunchPromise = nullptr at some point?

Comment 4

[Jens Stutte [:jstutte]]

From this crash it seems that mLaunchPromise is actually nullptr where we do not expect it.

IIUC mLaunchPromise is set here but mLaunchPromiseSettled is always initialized false, so if we did never call UtilityProcessHost::LaunchPromise we might be in this state? I think it might be better to initialize mLaunchPromiseSettled to true and set it false only when we successfully created the promise?

It seems this can happen when UtilityProcessHost::Launch fails as it happens before we actually set mLaunchPromise, but DestroyProcess(aSandbox); apparently expects it being there and crashes.

Comment 5

[Jens Stutte [:jstutte]]

(note that according to searchfox coverage instrumentation this case is not hit during testing)

Comment 6

[:gerard-majax]

(I did comment before without having time to really have a look).

If we have Launch() failure, we will likely be able to catch more as soon as I fix to properly get us back error in bug 1819311.
I'm not sure however looking at https://crash-stats.mozilla.org/report/index/5b3fb790-3584-41b7-83a5-9864a0230304 that mLaunchPromise = nullptr, shouldn't we dont see mozilla::MozPromise<bool, nsresult, 0>::Private::Reject<const nsresult&>(nsresult const&, char const*) in the stack if it was the case?

Comment 7

[:gerard-majax]

Attachment: Bug 1816857 - Avoid empty launch promise on Utility launch failure r?nika!

Comment 8

[Jens Stutte [:jstutte]]

(In reply to Alexandre LISSY :gerard-majax from comment #6)

I'm not sure however looking at https://crash-stats.mozilla.org/report/index/5b3fb790-3584-41b7-83a5-9864a0230304 that mLaunchPromise = nullptr, shouldn't we dont see mozilla::MozPromise<bool, nsresult, 0>::Private::Reject<const nsresult&>(nsresult const&, char const*) in the stack if it was the case?

I think that there is nothing that would prevent us from calling Reject on a nullptr here. And yesterday I also looked at this stack's minidump in VS which also showed mLaunchPromise to be null when we get there (to be taken with a grain of salt, of course, as minidumps do not contain all the memory, but still).

Comment 9

[:gerard-majax]

Anyway, I've prepared a patch with your suggestion, but forgot to try it (now I did: https://treeherder.mozilla.org/jobs?repo=try&revision=c78c6a43b46fda7209b6a1964d702706a2e32d9b). I did that quite late, so I'd like to come back at it soon to make sure it's right.

Comment 10

[:gerard-majax]

Wondering if we should not just check for mLaunchPromise instead of changing the logic around.

Comment 11

[Jens Stutte [:jstutte]]

(In reply to Alexandre LISSY :gerard-majax from comment #10)

Wondering if we should not just check for mLaunchPromise instead of changing the logic around.

Yeah, this way we would be able to keep the information if we ever started the promise: nullptr and mLaunchPromiseSettled == false would indicate we never started it, while nullptr and mLaunchPromiseSettled == true would tell us that it ran and settled.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/148ae2898c4a
Avoid empty launch promise on Utility launch failure r=nika

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/148ae2898c4a

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:gerard-majax, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.