Closed Bug 354980 Opened 18 years ago Closed 18 years ago

integrate airbag exception handler library on windows

Categories

(Toolkit :: Crash Reporting, defect)

Product:
Toolkit
Component:
Crash Reporting
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: ted, Assigned: ted)

References

(
URL
)

Details

Attachments

(3 files, 6 obsolete files)

Enable Crash Reporting dialog
9.93 KB, image/png
Details
Sending Crash Report dialog
6.19 KB, image/png
Details
More complete updated patch
43.84 KB, patch
Details | Diff | Splinter Review 
This integrates just the exception handling side of things, so we get minidumps.  There's lots more to do after that.

Patch and zip of new code coming right up.

You can see an example minidump that this produced at: http://mavra.perilith.com/~luser/6e94626f-9423-4861-b5c7-2c6f93af32af.dmp
(it's from the testcase on bug 354771)
Attached patch Changes to existing code (obsolete) — — Splinter Review 
These are the changes to the existing code.  There are some build system changes, including a --disable-airbag config option.  It's on by default on supported platforms (win32 only) as requested by bsmedberg.  The rules.mk tweak is to handle the .cc C++ files used in the airbag source.

The new source was too big to attach, so here it is:
http://mavra.perilith.com/~luser/airbag_exception_handler.zip

Unzip it to toolkit/, it contains the airbag directory.  The entire airbag source tree is currently included.  This may not be what we really want to do, but it works for now.  Yes, I need to add license headers to almost everything, and yes this is pretty rough code, but it works.  Currently you will get minidumps to your temp directory (usually Documents and Settings\User\Local Settings\Temp).  They will have GUID names, with a .dmp extension.  On my system, they're associated with VC2k5 Express, so you can actually postmortem debug the crash by doubleclicking the file.  :)

Comments and suggestions welcome!
Ok, I've got a windows client uploading the minidumps to a really small CGI on my website.  The code needs some cleanup, I should have a new patch by tomorrow.
Attached patch Updated patch (obsolete) — — Splinter Review 
Updated patch.  Now sets the minidump directory to $PROFILE/minidumps, and launches the crashreporter on a crash.

Code in the zip file has been updated as well, same URL:
http://mavra.perilith.com/~luser/airbag_exception_handler.zip

It now build a crashreporter.exe that submits the minidump.  I have that CGI running to collect them, but it doesn't do anything particularly interesting right now.

Anyway, this is far enough along that bsmedberg can start reviewing.  Again, comments and suggestions are welcome.  I'm sure some of this code could use reorganizing.
Attachment #241191 - Flags: review?(benjamin)
Attachment #240762 - Attachment is obsolete: true
*** Bug 331357 has been marked as a duplicate of this bug. ***
I managed to get symbols working in the stacktraces:
http://mavra.perilith.com/~luser/airbag-collector/list.pl

It's a one-off hack for now, but at least it proves that the code works.  :)

http://mavra.perilith.com/~luser/firefox-3.0a1.en-US.win32.airbag.zip

Opt build + airbag, reports to the CGI in my previous comment.  The VC8 CRT is not included, since vcsk5 express doesn't include the redist...  You can copy it from a trunk nightly, just copy msvc*.dll and the .manifest file.

Comments on the crash reporter UI are welcome.

This is close. We should really link the airbag library in toolkit/xre/Makefile.in, instead of in browser/app. Use SHARED_LIBRARY_LIBS.
Comment on attachment 241191 [details] [diff] [review]
Updated patch

I'd like to see the airbag makefile also.
Attachment #241191 - Flags: review?(benjamin) → review-
GUI for enabling crash reporting, displayed on first run of crashreporter or when running the program with no arguments.  Still needs proper wrapping in the text box, and real verbiage there.
GUI for sending crash report.  A progressmeter probably doesn't make sense here, since the crash reports should be fairly small (~60k) and airbag doesn't actually give us progress updates currently anyway.  We might just want something like a throbber on this dialog.
Attached patch Patch including new files (obsolete) — — Splinter Review 
Ok, this patch includes the new files in toolkit/airbag, with the exception of the airbag library source and the 3 Makefiles I wrote to build those static libs.
Attachment #241191 - Attachment is obsolete: true
Attachment #241936 - Flags: review?(benjamin)
*** Bug 216827 has been marked as a duplicate of this bug. ***
Comment on attachment 241936 [details] [diff] [review]
Patch including new files

Previous review comment about linking airbag into toolkit/xre still applies.

In SetAirbagExceptionHandler, we should at least check an environment variable.

We're going to have to think about how to set the airbag collector URL... it should really be configured in application.ini (nsXREAppData), and not set in crashreporter.ini, which should be for localization only. I suppose we don't need to do that for first-cut landing.
Attachment #241936 - Flags: review?(benjamin) → review+
Attachment #241936 - Flags: review?(mark)
Addresses bsmedberg's review comments:
1) Used SHARED_LIBRARY_LIBS in the xre Makefile.in
2) Added a check for an environment variable--MOZ_AIRBAG--must be set to 1 to enable the exception handler, otherwise off by default.
3) A few other minor tweaks.
Attachment #241936 - Attachment is obsolete: true
Attachment #242590 - Flags: review?(benjamin)
Attachment #241936 - Flags: review?(mark)
Ok, so the full patch including the airbag source is 2.5Mb, which is too big for bugzilla.  It's just the previous patch + the airbag source tree though.

It looks like airbag won't compile out of the box under VC6.  Is this going to be a problem with this enabled by default?

No.
(In reply to comment #17)
> It looks like airbag won't compile out of the box under VC6.  Is this going to
> be a problem with this enabled by default?

Nope.  Incidentally, we were wondering about this for the trunk alpha -- do we want to ship that alpha with airbag?  This would be end of November, so it may be too soon.
There is no reason not to ship the alpha with this compiled in. It will be off-by-default until we have the server resources in place (which will probably be after november).
shipping both talkback and airbag in an alpha or beta might be a good idea to help in the verfication or evaluation.   we could think about a turning both on or a random 50/50 split, then compare reports out of the backends to evaluate.
Comment on attachment 242590 [details] [diff] [review]
Updated patch (not including airbag source)

>Index: toolkit/airbag/client/Makefile.in

>+CPPSRCS = \
>+	$(NULL)

Just omit this.

>+#XXX: should move to toolkit/locale

Make sure a followup is filed, please?
Attachment #242590 - Flags: review?(benjamin) → review+
Fixed that review comment plus I updated to airbag tip to get some sender fixes, but bryner did some reorg, so there's a Makefile.in for airbag/src/common/windows/ now.  Nothing functionally changed.

I have a few followups I'm going to want to file, I was going to wait till this landed, but maybe I'll just file them now.
Attachment #242590 - Attachment is obsolete: true
Attachment #243214 - Flags: review?(mark)
Component: Talkback Client → Airbag
Flags: review?(mark)
Flags: review-
Flags: review+
Product: Core → Toolkit
Comment on attachment 243214 [details] [diff] [review]
Updated patch + bsmedberg's comment

Well that was silly.
Attachment #243214 - Flags: second-review?(mark)
Blocks: 358079
Blocks: 358082
QA Contact: chofmann → airbag
Comment on attachment 243214 [details] [diff] [review]
Updated patch + bsmedberg's comment

Looks good for the most part.

>Index: toolkit/airbag/Makefile.in

>+ACDEFINES +=    -UWIN32_LEAN_AND_MEAN

This seems like something we should fix in Airbag.

>Index: toolkit/airbag/nsAirbagExceptionHandler.cpp

>+TCHAR crashReporterExe[MAX_PATH] = L"\0";
>+TCHAR minidumpPath[MAX_PATH] = L"\0";

These can be static.

>+  ZeroMemory( &si, sizeof(si) );

Extra spacing?

>+  if(CreateProcess(NULL, cmdLine, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi)) {

Mozilla style is |if (CreateProcess(NULL, ...|, space between if and the opening parenthesis.

>+  gAirbagExceptionHandler = new google_airbag::ExceptionHandler(tempStr,
>+                                                                AirbagMinidumpCallback,

This line exceeds 80 characters.

>+  // check environment var to see if we're enabled.
>+  // we're off by default, so it must exist and be
>+  // set to a non-zero value.

Add a note that off-by-default is temporary.

>+  if(minidumpPath[l-1] != '\\' && l < MAX_PATH) {
>+    minidumpPath[l] = '\\';
>+    minidumpPath[l+1] = '\0';
>+  }

Do we (Airbag) do bad things if the path doesn't end with a backslash?  If so, we should fix that.

The logic here is wrong, you should make sure that (l < MAX_PATH - 1).

>Index: toolkit/airbag/client/crashreporter_win.cpp

>+#undef WIN32_LEAN_AND_MEAN

Why an #undef this time instead of -U?

>+using namespace google_airbag;

"using namespace" can get cumbersome, how about "using google_airbag::CrashReportSender"?  Or, since there's only one use in the file, you can just qualify the name with the namespace when you use it.

>+  //XXX: send some extra params?
>+  map<wstring,wstring> params;

Space between the two wstrings?

Not necessarily for now, but for the future, "name" and "version" at the very least are probably good ideas.  We should probably have guidelines here, so that various different airbag clients and servers can be closer to interoperable.
Attachment #243214 - Flags: second-review?(mark) → second-review+
(In reply to comment #25)
> >+ACDEFINES +=    -UWIN32_LEAN_AND_MEAN
> 
> This seems like something we should fix in Airbag.

Filed on your end:
http://code.google.com/p/airbag/issues/detail?id=60&can=2&q=
but for now I'll probably just #undef it in my source files.
  
> Do we (Airbag) do bad things if the path doesn't end with a backslash?  If so,
> we should fix that.

No, but since my code needs to build the minidump path in the callback, I need to ensure that we have that.  That's why I filed
http://code.google.com/p/airbag/issues/detail?id=40&can=2&q=

> >Index: toolkit/airbag/client/crashreporter_win.cpp
> 
> >+#undef WIN32_LEAN_AND_MEAN
> 
> Why an #undef this time instead of -U?

Apparently putting it in ACDEFINES breaks the resource compiler.  I guess I should just be consistent and #undef this everywhere.
 
> Not necessarily for now, but for the future, "name" and "version" at the very
> least are probably good ideas.  We should probably have guidelines here, so
> that various different airbag clients and servers can be closer to
> interoperable.

I'll file a followup on this.
Blocks: 358197
Attached patch Updated patch (obsolete) — — Splinter Review 
Ok, this addresses mento's comments, plus I updated to airbag tip (again) so I got his change to remove the dependency on rpcrt4.dll, so I removed that.  I will probably check this in tonight.
Attachment #243214 - Attachment is obsolete: true
You should be able to get rid of your #undef WIN32_LEAN_AND_MEAN directives now too.  Just be sure to explicitly #include any header files that you need.
Same as before, but I forgot to include browser/installer/windows/packages-static in the diff.
Attachment #243693 - Attachment is obsolete: true
Checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
I guess this technology will be available only in Firefox 3. No backporting expected to Firefox 1.5/2. Am I right?
Airbag won't build on anything less than VC8, which is only supported on trunk, so I'd say this is strictly trunk.

We need to come up with a unit-testing plan for this. We should at least have a known-crasher testcase to make sure airbag comes up, as well as tests for the crash-sender components.
Flags: in-testsuite?
I'm not up to speed on how our unit tests work.  Do we just wrap a small test harness around the C++ code and go from there, or should we write a test that invokes Firefox?  As for the crashreporter parts, does that JS http server support POST?  If so, we could potentially work something with that.
Depends on: js-post
> As for the crashreporter parts, does that JS http server support POST?

It currently doesn't, but it's definitely on the todo list.  I'll find some time in the next few weeks to work on POST support (filed as bug 366371), while I'm between semesters.

Question: do the POST submissions use the "chunked" transfer coding?  I wouldn't be at all surprised if it did, and if so, that's another MUST feature which will have to be supported to make this work (no bug filed, yet).
No longer depends on: js-post
Depends on: js-post
We know that airbag itself is pretty well unit tested. What we really want to test are our integration points. So a simple XR application with a known-crash component to check that a crash report was created and processed.
Do we have a way to run XulRunner apps as a test currently?  It should be trivial to write an XPCOM component that crashes to test this.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: