958348 - Double exceptions lead to deadlock in Breakpad

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(Away)]

From bug 958108:

Finally, breakpad tries to handle the second exception. This deadlocks at this line: http://hg.mozilla.org/mozilla-central/annotate/124d80d7556d/toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc#l448 because we lock ExceptionHandler::handler_stack_critical_section_ which is already locked from the first exception on the main thread. This is *supposed* to be prevented by http://hg.mozilla.org/mozilla-central/annotate/124d80d7556d/toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc#l421 but that appears to be broken in some way. Because this is a release-mode build I don't have great locals to figure out what the issue is, but it also ought to be a separate bug!

Comment 1

[(Away)]

So it turns out that SetUnhandledExceptionFilter is "broken" because we patched it out!

https://hg.mozilla.org/mozilla-central/file/30f3710477c2/toolkit/crashreporter/nsExceptionHandler.cpp#l313

If I remove that intercept, then the AV from the uninitialized critical section bubbles up to Windows and crashes the app, which isn't that much better. So really we need to fix the other bugs that got us here in the first place. Assuming those are fixed, is there any point to looking at this bug?

Comment 2

[Benjamin Smedberg]

Yes. I'd much rather the app crash than deadlock.

Comment 3

[(Away)]

Ideas -

* Remember the last two handlers when we start blocking, and allow switching back and forth between those two.

* Free pass for any handler within the same module as patched_SetUnhandledExceptionFilter.

Comment 4

[Benjamin Smedberg]

Or we could just call stub_SetUnhandledExceptionFilter directly and bypass our hook.

Comment 5

[(Away)]

Doh, how did I miss that :)

No concerns about coupling from having breakpad code know xul internals?

Comment 6

[Benjamin Smedberg]

This is all pretty tightly coupled, so I'm not so worried about it in general, but we shouldn't fork google-breakpad: maybe we need to have a callback of some sort. I'd talk to Ted about exactly how to do it in a way we can get it upstream.

Comment 7

[(Away)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> This is all pretty tightly coupled, so I'm not so worried about it in
> general, but we shouldn't fork google-breakpad: maybe we need to have a
> callback of some sort. I'd talk to Ted about exactly how to do it in a way
> we can get it upstream.

Ted, what do you think, replace direct calls to SetUnhandledExceptionFilter with a callback?

Comment 8

[(not currently active) Ted Mielczarek]

We could add a callback, but that seems very specialized. Your "remember the last two handlers" idea sounds less invasive, I don't know how much work it is. If that doesn't pan out we could add a callback to Breakpad.

Comment 9

[(Away)]

Attachment: Allow swapping the current and previous exception filters

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8367076 [details] [diff] [review]
Allow swapping the current and previous exception filters

Review of attachment 8367076 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this seems straightforward.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b1a561bd89e

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b1a561bd89e