Closed Bug 761909 Opened 8 years ago Closed 7 years ago

Implement headless crash report plumbing for gonk

Categories

(Toolkit :: Crash Reporting, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: cjones, Assigned: ted)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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: nobody → ted.mielczarek
Assignee: ted.mielczarek → nobody
Blocks: 741741
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
(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: --- → ?
Attached patch crash reporter plumbing for gonk (obsolete) — Splinter Review
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).
blocking-basecamp: ? → ---
Summary: Implement minimal (headless) crash submitter for gonk → Implement headless crash report plumbing for gonk
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?
Sorry, I made these fixes locally and neglected to upload an updated patch.
Attached patch crash reporter plumbing for gonk (obsolete) — Splinter Review
Updated patch. Still needs one more tweak before review.
Attachment #642154 - Attachment is obsolete: true
Attached patch crash reporter plumbing for gonk (obsolete) — Splinter Review
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)
Attachment #643218 - Attachment is obsolete: true
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?
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
(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 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!
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.
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.
(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.
Attachment #643498 - Attachment is obsolete: true
This needed some tweaking to work around the changes from bug 771251, so I'd like a quick second review.
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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(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.