Closed Bug 1449480 Opened 3 years ago Closed 3 years ago

Crash in logging::LogMessage::~LogMessage


(Core :: Security: Process Sandboxing, defect)

60 Branch
Not set



Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + fixed
firefox61 --- fixed


(Reporter: philipp, Assigned: bobowen)



(Keywords: crash, regression)

Crash Data


(1 file)

This bug was filed from the Socorro interface and is
report bp-5f7c1f31-d192-4084-b4a3-bc1180180328.

Top 10 frames of crashing thread:

0 firefox.exe logging::LogMessage::~LogMessage security/sandbox/chromium-shim/base/logging.cpp:120
1 firefox.exe `anonymous namespace'::ActiveVerifier::CloseHandle security/sandbox/chromium/base/win/
2 firefox.exe base::win::GenericScopedHandle<base::win::HandleTraits, base::win::VerifierTraits>::Close security/sandbox/chromium/base/win/scoped_handle.h:103
3 firefox.exe sandbox::TargetProcess::~TargetProcess security/sandbox/chromium/sandbox/win/src/
4 firefox.exe sandbox::PolicyBase::OnJobEmpty security/sandbox/chromium/sandbox/win/src/
5 firefox.exe `anonymous namespace'::JobTracker::FreeResources security/sandbox/chromium/sandbox/win/src/
6 firefox.exe sandbox::BrokerServicesBase::TargetEventsThread security/sandbox/chromium/sandbox/win/src/
7 kernel32.dll BaseThreadInitThunk 
8 mozglue.dll patched_BaseThreadInitThunk mozglue/build/WindowsDllBlocklist.cpp:838
9 ntdll.dll __RtlUserThreadStart 


this crash appears with a high frequency on 60.0b7 - it's 10% of browser crashes in very early stability data for that beta build and seems to come from the artificially introduced crash in bug 1445167.

the crash only seems to hit 32bit users of windows 7/8.1 so far.
Flags: needinfo?(bobowencode)
I was expecting these (as changes of signature from some other crashes), although the number of crashes is surprising.

It isn't crashing exactly where I thought it might be, but it's in the right area.
Hopefully I can get to the root cause of this now.
Assignee: nobody → bobowencode
Flags: needinfo?(bobowencode)
It looks like we must be closing handles twice somewhere, possibly not even in the chromium sandbox code.
This is possibly what is causing the other issues around handle monitoring in this code.

As this is so severe, I'm going to make this a DCHECK, so it will only crash on debug builds.
I'm not adding a patch to security/sandbox/chromium-shim/patches for this,
because we need to get this fixed ASAP, certainly before we take another update.
Attachment #8963099 - Flags: review?(jmathies)
Attachment #8963099 - Flags: review?(jmathies) → review+
Pushed by
Don't crash in opt builds when CloseHandleWrapper fails. r=jimm
Comment on attachment 8963099 [details] [diff] [review]
Don't crash in opt builds when CloseHandleWrapper fails

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1445167 introduced the actual crash, but the underlying problem (probably multiple closing of the same Windows handle) is unknown as yet.

[User impact if declined]:
This is the top crash on 60b7

[Is this code covered by automated tests?]:
Not directly, although this code is hit many times in most mochitests.

[Has the fix been verified in Nightly?]:

[Needs manual test from QE? If yes, steps to reproduce]: 

[List of other uplifts needed for the feature/fix]:

[Is the change risky?]:

[Why is the change risky/not risky?]:
This is a very simple change that changes the crashing CHECK to a DCHECK, so it will only crash in debug builds.
(Equivalent to making a MOZ_RELEASE_ASSERT into a MOZ_ASSERT.)

[String changes made/needed]:
Attachment #8963099 - Flags: approval-mozilla-beta?
Comment on attachment 8963099 [details] [diff] [review]
Don't crash in opt builds when CloseHandleWrapper fails

Make the check debug-only to avoid frequent crashes on release builds. Approved for 60.0b8.
Attachment #8963099 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
See Also: → 1564899
You need to log in before you can comment on or make changes to this bug.