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

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: 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.

Comment 2

[(not currently active) Ted Mielczarek]

Works on Linux, just need a green tree to land on.

Comment 3

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/a8ba788cac18

Comment 4

[(not currently active) Ted Mielczarek]

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.

Comment 5

[Benjamin Smedberg]

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.

Comment 6

[(not currently active) Ted Mielczarek]

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

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: Rollup patch

Comment 9

[LegNeato]

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.

Comment 10

[(not currently active) Ted Mielczarek]

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)

Comment 11

[Dave Townsend [:mossop]]

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

Comment 12

[(not currently active) Ted Mielczarek]

I swear I have no idea how that happened. Sorry!

Comment 13

[Al Billings [:abillings - ex-MoCo]]

(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.