602891 - Backend support for Jetpack crash reporting

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benjamin Smedberg]

Because we're not going to get builtin Firefox frontend support for jetpack-process crashes in Firefox 4 (bug 578685), I'm just going to implement all the necessary backend support. That way the jetpack SDK can have logic to submit crashes (as well as normal cleanup work) and restart the addon.

Comment 1

[Benjamin Smedberg]

Attachment: Part A - Refactor NoteIntentionalCrash, rev. 1

Comment 2

[Benjamin Smedberg]

Attachment: Part B - main Jetpack crashreporting, rev. 0.1, not quite passing tests

Comment 3

[Benjamin Smedberg]

Attachment: Part C - clear the listener when the IPC channel shuts down, rev. 1

cjones, we talked about this on IRC and this was the patch that ended up in my queue. I can't remember whether this was the solution you wanted or whether there was something different.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 487973 [details] [diff] [review]
Part A - Refactor NoteIntentionalCrash, rev. 1

IntentionalCrash.h probably needs a license header.

Why is IntentionalCrash.h in mozilla/ but the stuff inside isn't inside the mozilla:: namespace?

Comment 5

[Benjamin Smedberg]

Well, because I wanted it to be in an anonymous namespace, but I'm not sure why. I'll put it in mozilla::

Comment 6

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

Comment on attachment 487976 [details] [diff] [review]
Part C - clear the listener when the IPC channel shuts down, rev. 1

Apologies for the review lag.

The solution we discussed was jetpack posting a new task to (delete the *Process instance?).  Trying to interleave this *Channel code with the *Process dying is going to be pretty fragile.  It's more robust to keep a strong ordering on these events at shutdown.  (Yeah, this should be documented better.)

Comment 7

[Benjamin Smedberg]

Attachment: Part B - main Jetpack crashreporting, rev. 1

Apparently part C isn't needed, this at least passes the tests.

Comment 8

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

Comment on attachment 491187 [details] [diff] [review]
Part B - main Jetpack crashreporting, rev. 1

> NS_IMETHODIMP
> JetpackParent::Destroy()
> {
>-  if (!mSubprocess)
>-    return NS_ERROR_NOT_INITIALIZED;
>+  if (mSubprocess)
>+    Close();
> 
>-  Close();
>-  XRE_GetIOMessageLoop()
>-    ->PostTask(FROM_HERE, new DeleteTask<JetpackProcessParent>(mSubprocess));
>-  mSubprocess = NULL;
>-
>+  NS_ASSERTION(!mSubprocess, "ActorDestroy should have been called.");
>   return NS_OK;
> }

I don't know what Destroy() is for jetpacks or how it's used, so I can't review this.  The rest looks good.

Comment 9

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/dc021bc9929f
http://hg.mozilla.org/mozilla-central/rev/238d99ca6886
http://hg.mozilla.org/mozilla-central/rev/94e93142bfb6

Comment 10

[Benjamin Smedberg]

Updated docs at https://developer.mozilla.org/en/Jetpack_Processes to mention core:process-error.