Closed Bug 381049 Opened 17 years ago Closed 16 years ago

Pass swallowed crashes inside MSAA/IA2 methods to breakpad


(Core :: Disability Access APIs, defect)

Not set





(Reporter: aaronlev, Assigned: aaronlev)


(Blocks 1 open bug)


(Keywords: access)


(3 files, 1 obsolete file)

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.
Blocks: ia2
Here is the issue in breakpad's issue tracker:
That looks fixed. Do we mark this closed?
We probably need to expose that through our CrashReporter:: API.
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()) ?

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.

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.
Severity: normal → major
Blocks: fox3access
Summary: Add ability to pass a crash to breakpad → Pass swallowed crashes inside MSAA/IA2 methods to breakpad
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.
Attached patch WIP -- just for get_accState (obsolete) — Splinter Review
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.
And disabling that warning can be accomplished via #pragma warning( disable : 4509 )
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:

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?
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.
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?
I would have thought that all IDL is generated before any C++ is compiled. That's not the case?
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.
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.
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.
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.
Assignee: mark → aaronleventhal
Attachment #299654 - Attachment is obsolete: true
Attachment #301068 - Flags: review?(ted.mielczarek)
Attachment #301068 - Flags: review?(surkov.alexander)
Is it -w patch or try/catch has own indentation?
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
Is GetExceptionInformation win api function, it would be nice to have :: before it
Attachment #301068 - Flags: review?(surkov.alexander) → review+
(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?

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

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.
Attachment #301068 - Flags: review?(ted.mielczarek)
(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 on attachment 301134 [details] [diff] [review]
Tweak the comments for #pragma and for new nsICrashReporter::WriteMinidumpForException()

>Index: toolkit/xre/nsAppRunner.cpp
>+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/
>+REQUIRES += crashreporter

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

>Index: xpcom/system/

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.
Attachment #301134 - Flags: review?(ted.mielczarek) → review+
(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()
Component: Breakpad Integration → Disability Access APIs
Product: Toolkit → Core
QA Contact: breakpad.integration → accessibility-apis
Attached patch Fix nitsSplinter Review
Attachment #301271 - Flags: superreview?
Attachment #301271 - Flags: superreview? → superreview?(benjamin)
Attachment #301271 - Flags: superreview?(benjamin) → superreview+
Attachment #301271 - Flags: approval1.9?
Comment on attachment 301271 [details] [diff] [review]
Fix nits

Attachment #301271 - Flags: approval1.9? → approval1.9+
checked in

aaron, please file bugs to spread this on another msaa classes
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 417436
Depends on: 418032
You need to log in before you can comment on or make changes to this bug.