Pass swallowed crashes inside MSAA/IA2 methods to breakpad

RESOLVED FIXED

Status

()

--
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: aaronlev, Assigned: aaronlev)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

11 years ago
Blocks: 368873
(Assignee)

Comment 1

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

Comment 3

11 years ago
That looks fixed. Do we mark this closed?
We probably need to expose that through our CrashReporter:: API.
(Assignee)

Comment 5

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

Updated

11 years ago
Severity: normal → major
(Assignee)

Updated

11 years ago
Blocks: 396346
(Assignee)

Updated

11 years ago
Summary: Add ability to pass a crash to breakpad → Pass swallowed crashes inside MSAA/IA2 methods to breakpad

Comment 7

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

Comment 8

11 years ago
Created attachment 299654 [details] [diff] [review]
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.
(Assignee)

Comment 9

11 years ago
And disabling that warning can be accomplished via #pragma warning( disable : 4509 )
(Assignee)

Comment 10

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

Comment 13

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

Comment 14

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

Comment 15

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

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

Comment 17

11 years ago
Created attachment 301068 [details] [diff] [review]
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.
Assignee: mark → aaronleventhal
Attachment #299654 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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+
(Assignee)

Comment 21

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

(Assignee)

Comment 22

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

Updated

11 years ago
Attachment #301068 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 23

11 years ago
Created attachment 301134 [details] [diff] [review]
Tweak the comments for #pragma and for new nsICrashReporter::WriteMinidumpForException()
Attachment #301134 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 24

11 years ago
(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
>===================================================================
>+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.
Attachment #301134 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 26

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

Updated

11 years ago
Status: ASSIGNED → NEW
Component: Breakpad Integration → Disability Access APIs
Product: Toolkit → Core
QA Contact: breakpad.integration → accessibility-apis
(Assignee)

Comment 27

11 years ago
Created attachment 301271 [details] [diff] [review]
Fix nits
Attachment #301271 - Flags: superreview?
(Assignee)

Updated

11 years ago
Attachment #301271 - Flags: superreview? → superreview?(benjamin)

Updated

11 years ago
Attachment #301271 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Updated

11 years ago
Attachment #301271 - Flags: approval1.9?
Comment on attachment 301271 [details] [diff] [review]
Fix nits

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


aaron, please file bugs to spread this on another msaa classes
Status: NEW → RESOLVED
Last Resolved: 11 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.