1449480 - Crash in logging::LogMessage::~LogMessage

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[[:philipp]]

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.

Comment 1

[Bob Owen (:bobowen)]

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.

Comment 2

[Bob Owen (:bobowen)]

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.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Don't crash in opt builds when scoped_handle.cc CloseHandleWrapper fails

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4f627e88ea
Don't crash in opt builds when scoped_handle.cc CloseHandleWrapper fails. r=jimm

Comment 5

[Bob Owen (:bobowen)]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/17b23e75aceb

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7a4f627e88ea