Closed Bug 958348 Opened 6 years ago Closed 6 years ago

Double exceptions lead to deadlock in Breakpad

Categories

(Toolkit :: Crash Reporting, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: dmajor, Assigned: dmajor)

References

Details

Attachments

(1 file)

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!
See Also: → 958108
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?
Yes. I'd much rather the app crash than deadlock.
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.
Or we could just call stub_SetUnhandledExceptionFilter directly and bypass our hook.
Doh, how did I miss that :)

No concerns about coupling from having breakpad code know xul internals?
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.
(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?
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
Assignee: nobody → dmajor
Attachment #8367076 - Flags: review?(ted)
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.
Attachment #8367076 - Flags: review?(ted) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5b1a561bd89e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.