Closed Bug 602891 Opened 9 years ago Closed 9 years ago

Backend support for Jetpack crash reporting

Categories

(Core :: IPC, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

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.
Attachment #487973 - Flags: review?(ted.mielczarek)
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.
Attachment #487976 - Flags: review?(jones.chris.g)
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?
Attachment #487973 - Flags: review?(ted.mielczarek) → review+
Well, because I wanted it to be in an anonymous namespace, but I'm not sure why. I'll put it in mozilla::
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.)
Attachment #487976 - Flags: review?(jones.chris.g) → review-
Apparently part C isn't needed, this at least passes the tests.
Attachment #487975 - Attachment is obsolete: true
Attachment #487976 - Attachment is obsolete: true
Attachment #491187 - Flags: review?(jones.chris.g)
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.
Attachment #491187 - Flags: review?(jones.chris.g) → review+
Depends on: 613148
Updated docs at https://developer.mozilla.org/en/Jetpack_Processes to mention core:process-error.
Depends on: 613623
No longer depends on: 613623
You need to log in before you can comment on or make changes to this bug.