Crash in logging::LogMessage::~LogMessage

RESOLVED FIXED in Firefox 60

Status

()

defect
--
critical
RESOLVED FIXED
Last year
12 days ago

People

(Reporter: philipp, Assigned: bobowen)

Tracking

({crash, regression})

60 Branch
mozilla61
x86
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60+ fixed, firefox61 fixed)

Details

(crash signature)

Attachments

(1 attachment)

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/scoped_handle.cc:177
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/target_process.cc:82
4 firefox.exe sandbox::PolicyBase::OnJobEmpty security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.cc:598
5 firefox.exe `anonymous namespace'::JobTracker::FreeResources security/sandbox/chromium/sandbox/win/src/broker_services.cc:82
6 firefox.exe sandbox::BrokerServicesBase::TargetEventsThread security/sandbox/chromium/sandbox/win/src/broker_services.cc:222
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
Status: NEW → ASSIGNED
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 bobowencode@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4f627e88ea
Don't crash in opt builds when scoped_handle.cc CloseHandleWrapper fails. r=jimm
Comment on attachment 8963099 [details] [diff] [review]
Don't crash in opt builds when scoped_handle.cc 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?]:
No STR

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

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

[Is the change risky?]:
No

[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]:
None
Attachment #8963099 - Flags: approval-mozilla-beta?
Comment on attachment 8963099 [details] [diff] [review]
Don't crash in opt builds when scoped_handle.cc 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+
https://hg.mozilla.org/mozilla-central/rev/7a4f627e88ea
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
See Also: → 1564899
You need to log in before you can comment on or make changes to this bug.