559228 - Implement multi-process crash reporting on Mac

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of Crash Plugin UI

Seen while running  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100413 Minefield/3.7a5pre. I could not find a dupe of this bug. 

File: Flash Player.plugin
Version: 10.1.53.7
Shockwave Flash 10.1 r53

STR:
1. Load URL.
2. Crash in OOPP with attached screenshot

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsICrashReporter.submitReports]"  nsresult: "0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame :: chrome://browser/content/browser.js :: anonymous :: line 8449"  data: no]

Comment 1

[Marcia Knous [:marcia]]

Asa filed Bug 554451, but I guess I was wondering why there was no crash report available and less about the nature of the message.

Comment 2

[Marcia Knous [:marcia]]

The same video does not crash using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.3pre) Gecko/20100405 Firefox/3.6.3plugin1.

Comment 3

[Benjamin Smedberg]

I can't find the bug, but plugin-process crash reporting is not implemented on mac yet. cjones, is there a bug for that, or shall we use this one?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There's no bug AFAIK.

Comment 5

[Justin Dolske [:Dolske]]

The exception, specifically, is from reading the nsICrashReporter.submitReports property, which is unimplemented on OS X (bug 542379). But the effect of the exception is harmless at the moment (it's stale code from the 1st pass of UI where we submitted reports automatically, but will hopefully become unstale in the future if we add an optional autosubmit ability).

Comment 6

[Marcia Knous [:marcia]]

Nominating so this gets on the radar. Not sure if we are targeting getting data for the beta.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Marcia, don't think we need this for the first beta, but I do think we want to get it for beta2!

Comment 8

[Carsten Book [:Tomcat]]

(In reply to comment #7)
> Marcia, don't think we need this for the first beta, but I do think we want to
> get it for beta2!

well i think from a QA Services Point i think we would like the Data for Mac as soon as possible to look into possible issues.

Comment 9

[chris hofmann]

what is the value of turning on OOPP for Mac if we don't have any data to analyze and understand if OOPP is improving stability?   we should consider turning on Mac OOPP at the same time that we can get data out of crash-stats.

Comment 10

[Marcia Knous [:marcia]]

Since this bug is currently unowned and targeted for B2, shouldn't we get it assigned to someone?

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Probably because Benjamin isn't cc'd: Benjamin/Ted/Josh, can we get this resourced and assigned?

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Hey Ted, how's this looking for b3?

Comment 13

[(not currently active) Ted Mielczarek]

This is not going to make b3, it only got assigned to me a few days ago and I've only just barely started.

Comment 14

[Shawn Wilsher :sdwilsh]

This punting forward game is NOT sustainable.  This needs to get fixed for beta5, preferably early in the beta cycle.

Comment 15

[(not currently active) Ted Mielczarek]

This is almost done, I'll probably be able to land it in the next couple of days.

Comment 16

[(not currently active) Ted Mielczarek]

First portion is up for upstream review:
http://breakpad.appspot.com/146001/show

Comment 17

[(not currently active) Ted Mielczarek]

Second portion up:
http://breakpad.appspot.com/148001/show

Just need to hack up the Gecko glue to make it work.

Comment 18

[(not currently active) Ted Mielczarek]

These both landed upstream:
http://code.google.com/p/google-breakpad/source/detail?r=646
http://code.google.com/p/google-breakpad/source/detail?r=647

Comment 19

[(not currently active) Ted Mielczarek]

Ok, this is still going to require a little more work, and unfortunately I'm not available this weekend to finish it up. It's probably only another couple of hours of work, but I probably won't be able to finish it by Monday night.

Comment 20

[(not currently active) Ted Mielczarek]

Current work is available in my mq repository:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/mq/

The first three patches are:
1) Update Breakpad
2) Makefile changes to match Breakpad update
3) Gecko changes to use OOP crashreporting

It's close, but still needs a little work.

Comment 21

[(not currently active) Ted Mielczarek]

Okay, I've got the child process exception handling working. I don't have the hang detector dumping hooked up yet, but after talking with bsmedberg I think we'll just leave that disabled on mac for the time being so we can get child process crash reporting working in for beta 4. I'll attach patches in a second. The "sync to Breakpad" patch syncs us up to r647 + two patches that haven't landed yet:
http://breakpad.appspot.com/151001/show (has review, just didn't land it yet)
http://breakpad.appspot.com/156001/show (not yet reviewed, but a very simple patch)

Comment 22

[(not currently active) Ted Mielczarek]

Attachment: Sync to Breakpad r647, Moz build system changes

Comment 23

[(not currently active) Ted Mielczarek]

Attachment: Hook up OOP crash reporting for plugin processes on OS X

Comment 24

[(not currently active) Ted Mielczarek]

I ran the modules/plugin chrome mochitests with these patches and --setpref=dom.ipc.plugins.enabled=true and all the tests passed, so that's comforting. I've also pushed to try as a sanity check.

Comment 25

[(not currently active) Ted Mielczarek]

(In reply to comment #21)
> The "sync to Breakpad" patch syncs us up to r647 + two patches that haven't
> landed yet:
> http://breakpad.appspot.com/151001/show (has review, just didn't land it yet)
> http://breakpad.appspot.com/156001/show (not yet reviewed, but a very simple
> patch)

I just landed these upstream:
http://code.google.com/p/google-breakpad/source/detail?r=650
http://code.google.com/p/google-breakpad/source/detail?r=651

Comment 26

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/22568ff8773c
http://hg.mozilla.org/mozilla-central/rev/e93e3a3d3a6c
http://hg.mozilla.org/mozilla-central/rev/fc2a78c0ac8d

Comment 27

[(not currently active) Ted Mielczarek]

Just a note for QA:
This is going to be kind of a pain to verify. Right now we only run Flash 10.1 and the new Java plugin OOP on OS X:
http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#915

This means that you can't test with the "Test Plugin" without flipping the dom.ipc.plugins.enabled pref to true. In addition, you can't test by using "kill" to kill the plugin-container process like on Linux, since that doesn't trigger Breakpad on Mac.

Comment 28

[Marcia Knous [:marcia]]

(In reply to comment #27)
> Just a note for QA:
Thanks for the work, Ted. I will work on trying to verify this bug.

> This is going to be kind of a pain to verify. Right now we only run Flash 10.1
> and the new Java plugin OOP on OS X:
> http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#915
> 
> This means that you can't test with the "Test Plugin" without flipping the
> dom.ipc.plugins.enabled pref to true. In addition, you can't test by using
> "kill" to kill the plugin-container process like on Linux, since that doesn't
> trigger Breakpad on Mac.

Comment 29

[Marcia Knous [:marcia]]

I can verify that this is working using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b4) Gecko/20100817 Firefox/4.0b4 as I have already been able to crash in OOPP and received this report: http://crash-stats.mozilla.com/report/index/bp-30f0aed3-b49a-45c2-bb9a-4a2492100817.

If I understand Comment 21 correctly for now I will only get one report and not the hang report as well?

Comment 30

[(not currently active) Ted Mielczarek]

Correct, right now hang reporting is not hooked up at all on Mac. I've separated that out to bug 587747. Thanks for verifying!

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 466308 [details] [diff] [review]
Sync to Breakpad r647, Moz build system changes

Review of attachment 466308 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/crashreporter/google-breakpad/src/common/mac/Makefile.in
@@ +77,5 @@
>  FORCE_STATIC_LIB = 1
>  
>  include $(topsrcdir)/config/rules.mk
> +
> +COMPILE_CMFLAGS += -std=c99

You wouldn't happen to remember why you needed that C standard flag when building an ObjC file, would you?

Comment 32

[(not currently active) Ted Mielczarek]

Not 6 years later, no, but looking at the diff that patch also added MachIPC.mm, so I presume it was something in that file:
http://hg.mozilla.org/mozilla-central/rev/e93e3a3d3a6c#l3.12

Comment 33

[(not currently active) Ted Mielczarek]

Although that's a .mm file, and we don't use CMFLAGS for that (we use CMMFLAGS), so I'm not entirely sure.

Comment 34

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #33)
> Although that's a .mm file, and we don't use CMFLAGS for that (we use
> CMMFLAGS), so I'm not entirely sure.

Heh, I didn't even spot that, that's a good one. What made me raise an eyebrow is the -std flag for C99 for an ObjC source...