Implement headless crash report plumbing for gonk

RESOLVED FIXED in mozilla17

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cjones, Assigned: ted)

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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

Comment 1

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

Updated

6 years ago
Assignee: nobody → ted.mielczarek
(Assignee)

Updated

6 years ago
Assignee: ted.mielczarek → nobody

Updated

6 years ago
Blocks: 741741
(Assignee)

Comment 3

6 years ago
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".
Assignee: nobody → ted.mielczarek

Comment 4

6 years ago
(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.
blocking-basecamp: --- → ?
(Assignee)

Comment 5

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

Updated

6 years ago
blocking-basecamp: ? → ---
Summary: Implement minimal (headless) crash submitter for gonk → Implement headless crash report plumbing for gonk
(Assignee)

Updated

6 years ago
Blocks: 773892
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 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
I have a few other changes to make this compile on non Gonk platforms. Do you want the rebase and amended patch?
(Assignee)

Comment 9

6 years ago
Sorry, I made these fixes locally and neglected to upload an updated patch.
(Assignee)

Comment 10

6 years ago
Created attachment 643218 [details] [diff] [review]
crash reporter plumbing for gonk

Updated patch. Still needs one more tweak before review.
(Assignee)

Updated

6 years ago
Attachment #642154 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Created attachment 643498 [details] [diff] [review]
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.
Attachment #643498 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Attachment #643218 - Attachment is obsolete: true

Updated

6 years ago
Attachment #643498 - Flags: review?(benjamin) → review+
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?
(Assignee)

Comment 13

6 years ago
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!
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 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

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

Comment 17

6 years ago
(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

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

Comment 19

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

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

Comment 22

6 years ago
Created attachment 644940 [details] [diff] [review]
crash reporter plumbing for gonk.
Attachment #644940 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Attachment #643498 - Attachment is obsolete: true
(Assignee)

Comment 23

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

Comment 24

6 years ago
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.
Attachment #644940 - Flags: review?(benjamin) → review+
(Assignee)

Comment 25

6 years ago
That was strictly for correctness, as jimm pointed out that that code would use the uninitialized crashReporterPath variable in the headlessClient case.
https://hg.mozilla.org/mozilla-central/rev/e1c36c26a6e7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Comment 28

6 years ago
(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.
You need to log in before you can comment on or make changes to this bug.