761909 - Implement headless crash report plumbing for gonk

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

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

We need to modify the specs here based on product requirements.  But for now, let's hope we can get away without significant UI, because that's going to be a pain in the ass.

Comment 1

[(not currently active) Ted Mielczarek]

We talked about this in hand-wavy terms on a call a few weeks ago, but I don't think we nailed down the specifics. There are two potential ways we can make this work:
1) Write a tiny headless native app that gets spawned by the Gecko process from its exception handler.
2) Have the exception handler save some info before quitting, when the Gecko process gets restarted it can check this notice and fire an observer service notification or something, and Gaia can submit the report using our existing CrashSubmit.jsm code.

Which of those would you prefer?

Comment 2

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

Option (1) seems simpler on the surface, and would cover the case where the b2g "master" process itself crashes too much.  But we have to assume that that process is reliable because if it starts crash-looping, the entire phone is hosed.  (We know how to handle that case.)  And the bigger problem is that the b2g process configures wifi, 3g etc so the native app could quickly get complicated.

I favor (2).  Ideally we don't need gaia to directly control crash submission, we can control this through the DOM Settings API.  But that's negotiable based on product reqs.

(And TBH if the vast vast majority of crashes don't come from content processes, we lose again.  Content-process crashes are an easy problem.)

Comment 3

[(not currently active) Ted Mielczarek]

I'm going to take this bug as far as "save a note that there was a crash, fire an observer service notification on startup with the crash ID".

Comment 4

[Robert Kaiser]

(In reply to Ted Mielczarek [:ted] from comment #3)
> I'm going to take this bug as far as "save a note that there was a crash,
> fire an observer service notification on startup with the crash ID".

For the other steps, we should file additional bugs. I also think that having crash reporting working on B2G should block basecamp.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: crash reporter plumbing for gonk

This patch makes it so that on gonk we don't try to launch a crashreporter client, we simply write out the dump, but save the filename of the last dump we wrote to a known filename. Then on restart, we read that filename back out, move the {dmp,extra} to the pending directory and expose the dump ID on a new property on nsIXULRuntime. Chrome code can use this to submit the crash using CrashSubmit.jsm (which I didn't implement in this patch).

Comment 6

[Hubert Figuiere [:hub]]

Comment on attachment 642154 [details] [diff] [review]
crash reporter plumbing for gonk

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

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +1168,5 @@
>    delete notesField;
>    notesField = nsnull;
>  
> +  delete lastRunCrashID;
> +  lastRunCrashID = nsnull;

this needs to be surrounded by #ifdef NO_CRASHREPORTER_CLIENT

Comment 7

[Hubert Figuiere [:hub]]

Comment on attachment 642154 [details] [diff] [review]
crash reporter plumbing for gonk

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

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +2093,5 @@
>  #endif
>  
> +bool
> +CheckForLastRunCrash()
> +{

this needs to be surrounded by #ifdef NO_CRASHREPORTER_CLIENT

Comment 8

[Hubert Figuiere [:hub]]

I have a few other changes to make this compile on non Gonk platforms. Do you want the rebase and amended patch?

Comment 9

[(not currently active) Ted Mielczarek]

Sorry, I made these fixes locally and neglected to upload an updated patch.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: crash reporter plumbing for gonk

Updated patch. Still needs one more tweak before review.

Comment 11

[(not currently active) Ted Mielczarek]

Attachment: crash reporter plumbing for gonk

Updated slightly to allow runtime selection of whether a crashreporter client is present, so this code can more easily be reused for Metro work.

Comment 12

[Hubert Figuiere [:hub]]

Ted,

When I run with this patch on b2g, and I kill -SEGV b2g on the phone, I don't get any crash file created. 

If I debug it, in CheckForLastRunCrash(), the condition (NS_FAILED(lastCrashFile->Exists(&exists)) || !exists) is false, and therefor I don't get anything in lastRunCrashID

Am I missing something?

Comment 13

[(not currently active) Ted Mielczarek]

Oh! Yeah, I totally forgot. You need to have "export MOZILLA_OFFICIAL=1" in your mozconfig to get crash reporting enabled at runtime. I just edited gonk-misc/default-gecko-config in my B2G tree, there may be a less hacky way to do that.

Sorry!

Comment 14

[Hubert Figuiere [:hub]]

I have added "export MOZILLA_OFFICIAL=1" at the end of gonk-misc/default-gecko-config did clobber the tree and then build.sh and flash.sh, and still the same. SetExceptionHandler() is never called.

Comment 15

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 643498 [details] [diff] [review]
crash reporter plumbing for gonk

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

minor drive-by nit...

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +822,5 @@
>      return NS_ERROR_FAILURE;
>    nsCString tempPath(tempenv);
>  
> +#elif defined(MOZ_WIDGET_GONK)
> +  nsCString tempPath = NS_LITERAL_CSTRING("/data/local/tmp/");

The TMPDIR env var exists [1] in B2G builds.  Would be nice to use that instead of the hardcode here  

[1] https://github.com/mozilla-b2g/gonk-misc/blob/master/init.b2g.rc#L2

Comment 16

[Jim Mathies [:jimm]]

(In reply to Hub Figuiere [:hub] from comment #14)
> I have added "export MOZILLA_OFFICIAL=1" at the end of
> gonk-misc/default-gecko-config did clobber the tree and then build.sh and
> flash.sh, and still the same. SetExceptionHandler() is never called.

Does the generated application.ini have Enabled=1 and a crash server defined?

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to Michael Vines [:m1] from comment #15)
> The TMPDIR env var exists [1] in B2G builds.  Would be nice to use that
> instead of the hardcode here  
> 
> [1] https://github.com/mozilla-b2g/gonk-misc/blob/master/init.b2g.rc#L2

Thanks, I didn't know that!

Comment 18

[Jim Mathies [:jimm]]

In SetExceptionHandler don't you still need to initialize crashReporterPath? It's used in generating a path right after writing out lastCrashTimeFilename. Looks like that bracket in SetExceptionHandler needs to move up.

Comment 19

[(not currently active) Ted Mielczarek]

I can just move that block of code down below the early return for !doReport. We don't actually use the crashreporter client path in the headless client case. I had to fiddle a few things in this patch, I'll upload the new version prior to checkin shortly.

Comment 20

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #18)
> In SetExceptionHandler don't you still need to initialize crashReporterPath?
> It's used in generating a path right after writing out
> lastCrashTimeFilename. Looks like that bracket in SetExceptionHandler needs
> to move up.

Not sure if this is needed for unix. I've got updates for this for windows that I'll post to bug 741741.(In reply to Ted Mielczarek [:ted] from comment #19)
> I can just move that block of code down below the early return for
> !doReport. We don't actually use the crashreporter client path in the
> headless client case. I had to fiddle a few things in this patch, I'll
> upload the new version prior to checkin shortly.

On windows you do need crashReporterPath for the extra file data. If this works as is on unix don't worry about it, I've got an update on top of this for windows that I'll post in bug 741741.

Comment 21

[Hubert Figuiere [:hub]]

(In reply to Jim Mathies [:jimm] from comment #16)
> (In reply to Hub Figuiere [:hub] from comment #14)
> > I have added "export MOZILLA_OFFICIAL=1" at the end of
> > gonk-misc/default-gecko-config did clobber the tree and then build.sh and
> > flash.sh, and still the same. SetExceptionHandler() is never called.
> 
> Does the generated application.ini have Enabled=1 and a crash server defined?

in the [Crash Reporter]
I only have ServerURL defined.

Comment 22

[(not currently active) Ted Mielczarek]

Attachment: crash reporter plumbing for gonk.

Comment 23

[(not currently active) Ted Mielczarek]

This needed some tweaking to work around the changes from bug 771251, so I'd like a quick second review.

Comment 24

[Benjamin Smedberg]

Comment on attachment 644940 [details] [diff] [review]
crash reporter plumbing for gonk.

It's not clear to me why you needed to move the "cmdLine" block in the windows case, but this looks fine.

Comment 25

[(not currently active) Ted Mielczarek]

That was strictly for correctness, as jimm pointed out that that code would use the uninitialized crashReporterPath variable in the headlessClient case.

Comment 26

[(not currently active) Ted Mielczarek]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e1c36c26a6e7

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e1c36c26a6e7

Comment 28

[Robert Kaiser]

(In reply to Ted Mielczarek [:ted] from comment #13)
> Oh! Yeah, I totally forgot. You need to have "export MOZILLA_OFFICIAL=1" in
> your mozconfig to get crash reporting enabled at runtime.

We need to add that into the mozconfigs for the Nightly builds as well, I mentioned that in bug 785698 now, which already has other releng changes we need.