FPU Exception filter crashes when we hit a pure virtual function call

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Toolkit
Crash Reporting
--
critical
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

({verified1.9.2})

Trunk
mozilla1.9.3a5
x86
Windows 7
verified1.9.2
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .4+, status1.9.2 .4-fixed)

Details

(Whiteboard: [3.6.4 ridealong], URL)

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
I hit a local pure virtual call today which invoked Breakpad, which invoked the floating point exception filter. The app then crashed and produced a Microsoft crash dialog, because the EXCEPTION_POINTERS structure was NULL, and the filter unconditionally dereferences it.

Should be trivially patchable, and I think I can even write a test for it.
(Assignee)

Comment 1

8 years ago
Created attachment 443379 [details] [diff] [review]
Null-check exinfo

Two-line fix, a bunch more lines of test changes + test harness changes.

I need to test this on a non-Windows platform before landing, too.
Attachment #443379 - Flags: review?(benjamin)

Updated

8 years ago
Attachment #443379 - Flags: review?(benjamin) → review+
(Assignee)

Comment 2

8 years ago
Works on Linux, just need a green tree to land on.
(Assignee)

Comment 3

8 years ago
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/a8ba788cac18
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Comment 4

8 years ago
Comment on attachment 443379 [details] [diff] [review]
Null-check exinfo

This patch fixes a regression from the FPU exception filter in which we can crash again while handling certain types of crashes on Windows. The user will get the Microsoft crash dialog in those cases, and not our crash reporter, meaning we lose visibility into those crashes on Socorro. This patch contains a test for the change, as well.
Attachment #443379 - Flags: approval1.9.2.5?
(Assignee)

Updated

8 years ago
Flags: in-testsuite+

Updated

8 years ago
Attachment #443379 - Flags: approval1.9.2.4?

Comment 5

8 years ago
Comment on attachment 443379 [details] [diff] [review]
Null-check exinfo

I'd take this with a 3.6.4 respin also, although there's currently not one planned.

Updated

8 years ago
Whiteboard: [rc-ridealong]
(Assignee)

Comment 6

8 years ago
Bustage fix, had to disable the test on OS X (apparently we don't catch purecalls in Breakpad there!)
http://hg.mozilla.org/mozilla-central/rev/6c5782d2377e
Can you put a roll-up patch up for approval with the bustage fix included? I agree that if we do a 3.6.4 build4 we'd want this, otherwise we'll take for 3.6.5.
blocking1.9.2: ? → .5+
status1.9.2: --- → wanted
Whiteboard: [rc-ridealong] → [3.6.4 ridealong]
(Assignee)

Comment 8

8 years ago
Created attachment 443707 [details] [diff] [review]
Rollup patch
Attachment #443707 - Flags: approval1.9.2.5?
Attachment #443707 - Flags: approval1.9.2.4?
(Assignee)

Updated

8 years ago
Attachment #443379 - Flags: approval1.9.2.5?
Attachment #443379 - Flags: approval1.9.2.4?

Comment 9

8 years ago
Looks like we will be doing a build4 for bug 563847, so we'll want this ridealong as well.

a=LegNeato for 1.9.2.4. Please land on mozilla-1.9.2 default and GECKO1924_20100413_RELBRANCH asap.

Updated

8 years ago
Attachment #443707 - Flags: approval1.9.2.5?
Attachment #443707 - Flags: approval1.9.2.4?
Attachment #443707 - Flags: approval1.9.2.4+
(Assignee)

Comment 10

8 years ago
Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e15bd0e4e549

and to the relbranch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/489e05095126

The relbranch landing doesn't include the tests because there was another patch that needed to be backported to branch for the tests to work, and I didn't want to backport it to the relbranch as well. (got a=LegNeato for that on IRC) I did land the tests on 1.9.2 proper.

For verifying this bug (note it is Windows-only):
Install "Crash me now Advanced": http://ted.mielczarek.org/mozilla/crashme-advanced.xpi
Tools -> Crash Me! -> Pure virtual call!

Expected results: Mozilla Crash Reporter
(without this patch you would instead get the Windows error dialog)
(Assignee)

Updated

8 years ago
status1.9.2: wanted → .4-fixed
I'm guessing Ted didn't really mean to backout a set of extension manager tests with this so I restored them with http://hg.mozilla.org/mozilla-central/rev/c69f1d2e14c5
(Assignee)

Comment 12

8 years ago
I swear I have no idea how that happened. Sorry!
(In reply to comment #10)

> For verifying this bug (note it is Windows-only):
> Install "Crash me now Advanced":
> http://ted.mielczarek.org/mozilla/crashme-advanced.xpi
> Tools -> Crash Me! -> Pure virtual call!
> 
> Expected results: Mozilla Crash Reporter
> (without this patch you would instead get the Windows error dialog)

Verified for 1.9.1.4 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4) Gecko/20100513 Firefox/3.6.4 (.NET CLR 3.5.30729) and compared against 1.9.1.3 with the STR above.
Keywords: verified1.9.2
blocking1.9.2: .5+ → .4+
You need to log in before you can comment on or make changes to this bug.