Closed
Bug 354980
Opened 18 years ago
Closed 18 years ago
integrate airbag exception handler library on windows
Categories
(Toolkit :: Crash Reporting, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ted, Assigned: ted)
References
()
Details
Attachments
(3 files, 6 obsolete files)
9.93 KB,
image/png
|
Details | |
6.19 KB,
image/png
|
Details | |
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)
Assignee | ||
Comment 1•18 years ago
|
||
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!
Assignee | ||
Comment 2•18 years ago
|
||
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.
Assignee | ||
Comment 3•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Attachment #240762 -
Attachment is obsolete: true
Assignee | ||
Comment 4•18 years ago
|
||
*** Bug 331357 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 5•18 years ago
|
||
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. :)
Assignee | ||
Updated•18 years ago
|
Assignee | ||
Comment 6•18 years ago
|
||
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.
Comment 7•18 years ago
|
||
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 8•18 years ago
|
||
Comment on attachment 241191 [details] [diff] [review] Updated patch I'd like to see the airbag makefile also.
Updated•18 years ago
|
Attachment #241191 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 9•18 years ago
|
||
The boring top-level Makefile: http://mavra.perilith.com/~luser/airbag_exception_handler/airbag/Makefile.in The crashreporter Makefile: http://mavra.perilith.com/~luser/airbag_exception_handler/airbag/client/Makefile.in The handler and sender Makefiles: http://mavra.perilith.com/~luser/airbag_exception_handler/airbag/airbag/src/client/windows/handler/Makefile.in http://mavra.perilith.com/~luser/airbag_exception_handler/airbag/airbag/src/client/windows/sender/Makefile.in
Assignee | ||
Comment 10•18 years ago
|
||
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.
Assignee | ||
Comment 11•18 years ago
|
||
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.
Assignee | ||
Comment 12•18 years ago
|
||
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
Assignee | ||
Updated•18 years ago
|
Attachment #241936 -
Flags: review?(benjamin)
*** Bug 216827 has been marked as a duplicate of this bug. ***
Blocks: 216827
Comment 14•18 years ago
|
||
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+
Updated•18 years ago
|
Attachment #241936 -
Flags: review?(mark)
Assignee | ||
Comment 15•18 years ago
|
||
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)
Assignee | ||
Comment 16•18 years ago
|
||
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.
Assignee | ||
Comment 17•18 years ago
|
||
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?
Comment 18•18 years ago
|
||
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.
Comment 20•18 years ago
|
||
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).
Comment 21•18 years ago
|
||
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 22•18 years ago
|
||
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+
Assignee | ||
Comment 23•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Component: Talkback Client → Airbag
Flags: review?(mark)
Flags: review-
Flags: review+
Product: Core → Toolkit
Assignee | ||
Comment 24•18 years ago
|
||
Comment on attachment 243214 [details] [diff] [review] Updated patch + bsmedberg's comment Well that was silly.
Attachment #243214 -
Flags: second-review?(mark)
Updated•18 years ago
|
QA Contact: chofmann → airbag
Comment 25•18 years ago
|
||
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+
Assignee | ||
Comment 26•18 years ago
|
||
(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.
Assignee | ||
Comment 27•18 years ago
|
||
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
Comment 28•18 years ago
|
||
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.
Assignee | ||
Comment 29•18 years ago
|
||
Same as before, but I forgot to include browser/installer/windows/packages-static in the diff.
Attachment #243693 -
Attachment is obsolete: true
Assignee | ||
Comment 30•18 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 31•18 years ago
|
||
I guess this technology will be available only in Firefox 3. No backporting expected to Firefox 1.5/2. Am I right?
Assignee | ||
Comment 32•18 years ago
|
||
Airbag won't build on anything less than VC8, which is only supported on trunk, so I'd say this is strictly trunk.
Comment 33•18 years ago
|
||
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?
Assignee | ||
Comment 34•18 years ago
|
||
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.
Comment 35•18 years ago
|
||
> 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
Comment 36•18 years ago
|
||
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.
Assignee | ||
Comment 37•18 years ago
|
||
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.
Description
•