Closed
Bug 1449480
Opened 3 years ago
Closed 3 years ago
Crash in logging::LogMessage::~LogMessage
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | + | fixed |
firefox61 | --- | fixed |
People
(Reporter: philipp, Assigned: bobowen)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(1 file)
1.47 KB,
patch
|
jimm
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Updated•3 years ago
|
Flags: needinfo?(bobowencode)
Assignee | ||
Comment 1•3 years ago
|
||
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)
Assignee | ||
Comment 2•3 years ago
|
||
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.
Assignee | ||
Comment 3•3 years ago
|
||
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)
![]() |
||
Updated•3 years ago
|
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
Assignee | ||
Comment 5•3 years ago
|
||
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 6•3 years ago
|
||
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+
Comment 7•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/17b23e75aceb
Comment 8•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7a4f627e88ea
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•