381049 - Pass swallowed crashes inside MSAA/IA2 methods to breakpad

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Because COM's RPC mechanism wraps calls in __try/__except, it is hiding crashes in our accessibility code on Windows. This leads to mysterious and difficult-to-fix bugs.

If we could wrap our own COM calls in __try/__except and then pass the crashes to breakpad, that would help us increase quality.

Comment 1

[Aaron Leventhal]

Here is the issue in breakpad's issue tracker: http://code.google.com/p/google-breakpad/issues/detail?id=172

Comment 2

[Aaron Leventhal]

No it's now http://code.google.com/p/google-breakpad/issues/detail?id=192

Comment 3

[Aaron Leventhal]

That looks fixed. Do we mark this closed?

Comment 4

[(not currently active) Ted Mielczarek]

We probably need to expose that through our CrashReporter:: API.

Comment 5

[Aaron Leventhal]

Notes from Mark Mentovai:

You'll need access to the global exception handler object created  
near startup time.  This object is created by toolkit/airbag/ 
nsAirbagExceptionHandler.cpp as gExceptonHandler, but it's a static  
global, so you'll need to work some magic to be able to access it  
from the code you need to.  Once you've done that, you can just pass  
the exception from an SEH filter clause directly into Breakpad.   
Assuming that all you do is de-static-ize that variable and provide  
an extern declaration in the code you need to use it from, AND that  
you're working from within the same module:

__try {
   // guarded code
}
__except (gExceptionHandler->WriteMinidumpForException 
(GetExceptionInformation()) ?
               EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) {
}

That's as simple as it gets, but your contortions may need to be more  
involved if the exception handler's in a different module from the  
code you need to specially guard, or if you'll be going through an  
accessor instead of a direct "extern" declaration for some other reason.

Mark

Comment 6

[(not currently active) Ted Mielczarek]

We could add a function to nsAirbagExceptionHandler.h, in the CrashReporter namespace (#ifdef XP_WIN32 obviously) that would accept an EXCEPTION_POINTERS directly and pass it to gExceptionHandler.

Comment 7

[Marco Zehe (:MarcoZ)]

One such hard to reproduce bugs has crept into Thunderbird on November 28, 2007, documented in bug 405951. This is a result of fix for bug 404380. Problem is if I try to grab a call stack using a Debug build and Visual Studio, it doesn't produce any useful output.
So if the crash could be caught and passed to BreakPad so we'd get a more accurate report, this would be extremely valuable.

Comment 8

[Aaron Leventhal]

Attachment: WIP -- just for get_accState

I get these warnings:
c:/moz/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp(536) : warning C4509: nonstandard extension used: 'nsAccessibleWrap::get_accState' uses SEH and 'xpAccessible' has destructor
        c:/moz/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp(528) : see declaration of 'xpAccessible'

SEH doesn't like the destructor on an auto variable, if the exception handler is called the destructor won't be. However at that point we've crashed and I don't care so AFAICT we should just disable this warning here.

Comment 9

[Aaron Leventhal]

And disabling that warning can be accomplished via #pragma warning( disable : 4509 )

Comment 10

[Aaron Leventhal]

Ted, our dependencies are somehow broken.

A build error occurs if you try to use nsICrashReporter.h even if you add crashreporter to requires:
e:/builds/sendchange-slave/sendchange-win32/mozilla/accessible/src/msaa/nsAccessibleWrap.cpp(67) : fatal error C1083: Cannot open include file: 'nsICrashReporter.h': No such file or directory
Here's the log when the try server tried it:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1201491140.1201495424.26772.gz#err0

On my local machine if I build the toolkit/crashreporter directory first everything works fine.

What's going on? Why isn't nsICrashReporter.h getting generated by xpidl at the same time all the other nsI*.h files are being generated?

Comment 11

[(not currently active) Ted Mielczarek]

aaron: you don't have --disable-crashreporter in your mozconfig, right?  The makefile just uses XPIDLSRCS, which gets generated in export, so I don't know why it wouldn't work properly.

Comment 12

[(not currently active) Ted Mielczarek]

Oh, a11y is in tier_gecko, and crashreporter is in tier_toolkit, so your code gets built before crashreporter.

bsmedberg: this kind of sucks.  Is there a way around this?

Comment 13

[Aaron Leventhal]

I would have thought that all IDL is generated before any C++ is compiled. That's not the case?

Comment 14

[Benjamin Smedberg]

That is not the case. We do the build in tiers specifically to prevent code from tier gecko depending on APIs in tier toolkit, etc.

We could move accessibility into tier-toolkit, I suppose.

Comment 15

[Aaron Leventhal]

Right, but layout and widget need to use accessibility interfaces, so that can't work.

Why not move crash reporter into tier gecko? It's not just MSAA that swallows crashes, anything that uses COM does that. They may need to pass the crashes directly to crashreporter as well.

Comment 16

[Benjamin Smedberg]

The crashreporter impl has to stay in tier-toolkit for linkage reasons. The standard solution to this would be to move the API (nsICrashReporter) into a lower tier (perhaps in xpcom/system) so that you can depend on the API.

Comment 17

[Aaron Leventhal]

Attachment: Wrap IAccessible methods. Will do other interface impls in follow-up patches

I tested this and it does send crash reports in!

The reports have a stack but don't have actual source code lines in them but I believe that's because it's my own build and that once this is in an official nightly it will look like any other crash report.

Comment 18

[alexander :surkov (:asurkov)]

Is it -w patch or try/catch has own indentation?

Comment 19

[alexander :surkov (:asurkov)]

Actually I'm ok with the patch, at least with ally part.

Your new method should have notion it's implemented on win platform only. Why is SEH property of win32, i mean what's up with win64? Your method return void, not boolean.

Also I would say either to cvs copy file or if you remove/add it then clean up it in the sense of styling but it's up to  you and reviewer of that code sure.

Could you add comment for pragma warning 4509?

I assume crashreporter is not an option, otherwise it makes sense to use macros for try/catch

Comment 20

[alexander :surkov (:asurkov)]

Is GetExceptionInformation win api function, it would be nice to have :: before it

Comment 21

[Aaron Leventhal]

(In reply to comment #18)
> Is it -w patch or try/catch has own indentation?
It's not -w but I really don't want to break CVS history on every method. I think it's worth bending the rules on indentation this time.


(In reply to comment #19)
> Also I would say either to cvs copy file or if you remove/add it then clean up
> it in the sense of styling but it's up to  you and reviewer of that code sure.
We can try that. There have not been significant changes to that file, however. It's at version 1.2 and the second round of changes was just whitespace.

> Could you add comment for pragma warning 4509?
Yes.

> I assume crashreporter is not an option, otherwise it makes sense to use macros for try/catch
Crashreporter is an option but when it's turned off everything will behave as it does now, because it will fall back to the exception handler that's part of COM anyway. It's just that the crash won't be reported.

Comment 22

[Aaron Leventhal]

Fixed the comment for the new method:
  /**
    * Write a minidump immediately, with the user-supplied exception
    * information. This is implemented on Windows only, because
    * SEH (structured exception handling) exists on Windows only.
    * @param aExceptionInfo  EXCEPTION_INFO* provided by Window's SEH
    */
  [noscript] void WriteMinidumpForException(in voidPtr aExceptionInfo);

1) SEH is not for Win32 only.
2) This doesn't return a boolean for success -- it uses XPCOM's nsresult type instead as most interface methods do.
3) Explains why implemented on Windows only.

Comment 23

[Aaron Leventhal]

Attachment: Tweak the comments for #pragma and for new nsICrashReporter::WriteMinidumpForException()

Comment 24

[Aaron Leventhal]

(In reply to comment #20)
> Is GetExceptionInformation win api function, it would be nice to have :: before
> it
> 

Okay, just added that now. Missed this before.

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 301134 [details] [diff] [review]
Tweak the comments for #pragma and for new nsICrashReporter::WriteMinidumpForException()

>Index: toolkit/xre/nsAppRunner.cpp
>===================================================================
>+NS_IMETHODIMP
>+nsXULAppInfo::WriteMinidumpForException(void *aExceptionInfo)

nit: prevailing style here is to have the * next to the type name, |void* aExceptionInfo|.

>Index: toolkit/crashreporter/nsExceptionHandler.cpp
>===================================================================
>+nsresult WriteMinidumpForException(EXCEPTION_POINTERS *aExceptionInfo)

Here as well.

>Index: accessible/src/msaa/Makefile.in
>===================================================================
>+ifdef MOZ_CRASHREPORTER
>+REQUIRES += crashreporter
>+endif
>+

You probably don't need this since you moved the idl, do you?

>Index: xpcom/system/Makefile.in
>===================================================================

I'm not sure that I'm really excited about having the nsICrashReporter interface live here, but I don't have a better suggestion.

Looks good to me aside from those nits.

Comment 26

[Aaron Leventhal]

(In reply to comment #20)
> Is GetExceptionInformation win api function, it would be nice to have :: before
> it

The compiler is allowing me to put the first :: in, but not the second here:
FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }

I get:
nsAccessibleWrap.cpp(150) : error C2589: '(' : illegal token on right side of '::'
nsAccessibleWrap.cpp(150) : error C2059: syntax error : '::'
nsAccessibleWrap.cpp(150) : error C2143: syntax error : missing ';' before '{'

So I can put it on GetExceptionCode() but not GetExceptionInformation()

Comment 27

[Aaron Leventhal]

Attachment: Fix nits

Comment 28

[Damon Sicore (:damons)]

Comment on attachment 301271 [details] [diff] [review]
Fix nits

a1.9+=damons

Comment 29

[alexander :surkov (:asurkov)]

checked in


aaron, please file bugs to spread this on another msaa classes