Closed Bug 1449480 Opened 3 years ago Closed 3 years ago
Crash in logging::Log
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.
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
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)
Pushed by firstname.lastname@example.org: 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+
You need to log in before you can comment on or make changes to this bug.