MemoryProtectionExceptionHandler shouldn't call __debugbreak().

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: ehoogeveen, Assigned: ehoogeveen)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
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
(Assignee)

Comment 1

11 months ago
Created attachment 8823391 [details] [diff] [review]
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.
Attachment #8823391 - Flags: review?(jdemooij)

Comment 2

11 months ago
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

11 months ago
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!
(Assignee)

Comment 4

11 months ago
(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.

Updated

11 months ago
Attachment #8823391 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 5

11 months ago
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.
Keywords: checkin-needed

Comment 6

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc66456df92f
Don't unintentionally crash from the exception handler on Windows. r=jandem
Keywords: checkin-needed

Comment 7

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc66456df92f
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.