1328355 - MemoryProtectionExceptionHandler shouldn't call __debugbreak().

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks to dmajor for spotting this. ReportCrashIfDebug() currently calls __debugbreak() on Windows because I mistakenly assumed it was a no-op on opt builds. In reality, it's what actually triggers the crash in MOZ_CRASH().

That's not so bad in and of itself, but we call ReportCrashIfDebug() *before* setting the crash annotation [2], so crashes from PageProtectingVector on Windows won't have had crash reasons in Socorro. There probably weren't any crashes anyway, since the used page protection didn't catch anything on OSX either, but it's still pretty annoying.

[1] https://dxr.mozilla.org/mozilla-central/rev/c91249f41e3766274131a84f9157a4d9d9949520/js/src/ds/MemoryProtectionExceptionHandler.cpp#143
[2] https://dxr.mozilla.org/mozilla-central/rev/c91249f41e3766274131a84f9157a4d9d9949520/js/src/ds/MemoryProtectionExceptionHandler.cpp#185

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Don't unintentionally crash from the exception handler on Windows.

Just removes the offending call. Thinking back more, I'm pretty sure I did see crash reasons on Socorro in local tests. Still, this isn't the behavior I intended.

Comment 2

[(Away)]

For what it's worth, all of this is behind #ifdef DEBUG anyway, so it shouldn't have any impact on crash-stats in the wild. But it's still good to fix this for the sake of having the code match the intent. :)

Comment 3

[(Away)]

Also note that in bug 1324093 patch 2, I plan to stop setting crash annotations on debug builds. I assumed nobody was paying attention to them. Let me know if that's wrong!

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to David Major [:dmajor] from comment #2)
> For what it's worth, all of this is behind #ifdef DEBUG anyway, so it
> shouldn't have any impact on crash-stats in the wild.

Oh you're right, that's why it didn't make a difference in local testing.

> Also note that in bug 1324093 patch 2, I plan to stop setting crash annotations on debug builds. I assumed nobody was paying attention to them. Let me know if that's wrong!

That's fine, as far as I know the crash reporter annotation is purely for crash stats. Reporting the crash reason to stderr in debug builds seems worthwhile even for the special annotations from MemoryProtectionExceptionHandler, but as far as I can tell you aren't changing that.

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks! A try run wouldn't do much good here as we don't expect to hit these crashes, and anyway this just removes a single function call.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc66456df92f
Don't unintentionally crash from the exception handler on Windows. r=jandem

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dc66456df92f