Last Comment Bug 806515 - Send an app identifier for B2G content crashes
: Send an app identifier for B2G content crashes
Status: VERIFIED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: B2G C3 (12dec-1jan)
Assigned To: Hubert Figuiere [:hub]
:
Mentors:
Depends on: 759502 777187
Blocks: b2g-crash-reporting 821710
  Show dependency treegraph
 
Reported: 2012-10-29 13:03 PDT by Robert Kaiser
Modified: 2012-12-30 18:15 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
fixed
fixed


Attachments
Bug 806515 - Send app identifier for B2G content. (947 bytes, patch)
2012-12-25 14:50 PST, Hubert Figuiere [:hub]
bzbarsky: review+
Details | Diff | Splinter Review
Bug 806515 - Part 2: use the URL field for content crash. (886 bytes, patch)
2012-12-27 08:56 PST, Hubert Figuiere [:hub]
kairo: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2012-10-29 13:03:06 PDT
In bug 777187, we implemented some crash annotations that tell us about what device we are crashing on - things that match what we're sending on Android as well.
In bug 759502, we made the desktop web app runtime send the origin of a web app in the URL field of crash reports (which is only available to a small selected set of people on the server side, due to privacy reasons).

It would be extremely valuable for stability assessment (think trying to reproduce the crash to be able to debug) if we could send an identifier of the crashing app on B2G as well (probably also the origin, to match the desktop runtime) and potentially even the URL of crashed tabs in the browser (but that's something we can potentially delay to post-v1).
Comment 1 Hubert Figuiere [:hub] 2012-10-29 13:13:59 PDT
I can take that one if you want.
Comment 2 Robert Kaiser 2012-11-01 09:57:42 PDT
Hub, thanks, though I'd first like some input from privacy there, that's why I CCed Tom.

Tom, what can/should we do there to get helpful information and at the same time be OK from the privacy POV?
Comment 3 Tom Lowenthal [:StrangeCharm] 2012-11-02 17:26:14 PDT
I think that this is consistent with the message that we currently show on crashes, and our general explanations of crash reporting elsewhere. I'd *like* to double check that what we end up collecting is clearly in-line with the notice (&c) after it'd done, but I don't think that's anywhere near blocking.
Comment 4 Robert Kaiser 2012-11-03 17:22:25 PDT
Tom, does your OK to this include browser tab URLs, or should we split those off into their own bug (and maybe even push off to v2 so we might do some useful UI for it if needed)?

Hub, I think comment #3 means we can move forward with adding an annotation for app crashes to list the app origin in the URL field, like it's being done for the desktop web app runtime.
Comment 5 Tom Lowenthal [:StrangeCharm] 2012-11-06 11:32:41 PST
Based on my knowledge of the notice we're planning to give, I think that users should expect everything about their session (including URLs, half-filled forms, cookies...) to be sent.  I'm still on the hook for the text of the page which describes everything that goes in the report. 

Would it work for you to document everything you'd like to have in the report, so I can double-check my notes for what to put on that page?
Comment 6 David Bolter [:davidb] 2012-11-14 11:01:15 PST
This is on hub's plate unless someone beats him to it.
Comment 7 Robert Kaiser 2012-11-26 10:01:28 PST
(In reply to Tom Lowenthal [:StrangeCharm] from comment #5)
> Would it work for you to document everything you'd like to have in the
> report, so I can double-check my notes for what to put on that page?

The problem with that (and why I haven't replied for so long) is that it would require to document everything that is in a normal crash report now, and I'm not sure I even know everything there. I know we have the minidump that contains some memory fragments, including the stack (but not 100% sure what all is in there), then we have a number of fields that describe what build, system and device this crash happened on, possibly some circumstances like when the build was launched and when it crashed - and we'd like to add to that which app crashed (i.e. the app origin/URL) or which browser tab crashed (URL of the website).
Comment 8 Tom Lowenthal [:StrangeCharm] 2012-12-07 12:42:25 PST
I'd like to know everything in a normal crash report, so that we can document it and notify users appropriately. For more privacy-conscious users, knowing what's in a crash report may be an important part of deciding whether to send one. I'd really appreciate it if we could make a SUMO article documenting what's in a crash report.
Comment 9 Robert Kaiser 2012-12-13 06:54:56 PST
I don't know if I even could tell you "everything in a normal crash report" as I don't know what exactly is or can be included in the minidump. Ted or Benjamin should know about that.

Outside of the minidump, the annotations a B2G crash report currently contains are those:
"Version", "id", "Vendor", "version", "ReleaseChannel", "buildid", "BuildID", "ProductName", "ProductID" (Information about what build this is),
"InstallTime" (Information about the specific installation),
"Notes", "StartupTime", "FramePoisonSize", "FramePoisonBase" (Information about the current run of the build),
"CrashTime", "submitted_timestamp", "timestamp" (Information about the specific crash),
"Android_Hardware", "Android_Device", "Android_CPU_ABI2", "Android_CPU_ABI", "Android_Model", "Android_Brand", "Android_Manufacturer", "Android_Version", "Android_Board" (Device/OS information).

What we request to be added here is information on which app or tab was causing the crash.


That said, we are getting late enough into the B2G development process that it's getting questionable if we have any chance to still get this in at all for v1.
Comment 10 Alex Keybl [:akeybl] 2012-12-13 09:44:19 PST
Given comment 8, let's move forward with adding the app origin into the URL field. Comment 9 (and subsequent documentation) will do the trick. Without this info, our understanding is that finding STR for crashes will be very difficult.
Comment 11 Benjamin Smedberg [:bsmedberg] 2012-12-13 09:56:09 PST
http://hg.mozilla.org/mozilla-central/annotate/9db79b97abbb/dom/ipc/ContentParent.cpp#l638 is where you would normally insert the annotation code on a crash. This requires that the ContentParent know its app origin/current URL, of course.
Comment 12 Robert Kaiser 2012-12-14 07:22:09 PST
I split off the part about browser tab URLs to bug 821710 and the documentation to bug 821711.

From all I can gather, Tom says in comment #3 that we're good to send the app origin in the URL field (which is protected for privacy on the server side anyhow) for app process crashes and match the desktop and mobile web app runtimes in that regard.

Hub says in comment #1 that he can take this and Alex in comment #10 that we'd like to actually have it soon. Hub, can you move forward with implementing this?
Comment 13 Andrew Overholt [:overholt] (back Aug 31) 2012-12-21 07:53:51 PST
How are things going here, Hub?
Comment 14 Hubert Figuiere [:hub] 2012-12-25 14:50:05 PST
Created attachment 695675 [details] [diff] [review]
Bug 806515 - Send app identifier for B2G content.
Comment 15 Hubert Figuiere [:hub] 2012-12-25 14:52:58 PST
Comment on attachment 695675 [details] [diff] [review]
Bug 806515 - Send app identifier for B2G content.

I only one review from either of you. It seems that jlebar is on vacation until 2013.

Thanks.
Comment 16 Boris Zbarsky [:bz] 2012-12-26 13:51:49 PST
Comment on attachment 695675 [details] [diff] [review]
Bug 806515 - Send app identifier for B2G content.

Maybe call it AppManifestURL so it doesn't collide with other URLs people might want?

r=me with that
Comment 17 Hubert Figuiere [:hub] 2012-12-26 17:52:19 PST
Will do. Thanks !
Comment 19 Justin Lebar (not reading bugmail) 2012-12-27 07:43:32 PST
I tried to submit a comment here a few days ago, but flaky wifi is flaky:

Right now browser processes will have an empty URL field, while the main process will have no URL field.  Are you able to distinguish these two cases in crash-stats?

Because of the potential for confusion here, I was going to suggest that we do this right, rather than push it off into a follow-up.  But I don't get a say if my comments don't go through.  :)
Comment 20 Hubert Figuiere [:hub] 2012-12-27 08:06:59 PST
(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #19)
> I tried to submit a comment here a few days ago, but flaky wifi is flaky:
> 
> Right now browser processes will have an empty URL field, while the main
> process will have no URL field.  Are you able to distinguish these two cases
> in crash-stats?

I guess we need to special case this. How do I detect I'm in the browser process?

I'll do a follow up patch for that.

> Because of the potential for confusion here, I was going to suggest that we
> do this right, rather than push it off into a follow-up.  But I don't get a
> say if my comments don't go through.  :)

No worries. I don't mind doing a follow-up patch.
Comment 21 Robert Kaiser 2012-12-27 08:18:24 PST
(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #19)
> Right now browser processes will have an empty URL field, while the main
> process will have no URL field.  Are you able to distinguish these two cases
> in crash-stats?

Yes, as browser tabs don't send URLs anyhow yet.

> Because of the potential for confusion here, I was going to suggest that we
> do this right, rather than push it off into a follow-up.  But I don't get a
> say if my comments don't go through.  :)

We send the app origin in the URL field for the desktop and Android web app runtimes as well, so if you believe we should do it differently here, then I call on you to submit the changes for those and for Socorro to handle the new fields correctly as well.
Comment 22 Hubert Figuiere [:hub] 2012-12-27 08:20:46 PST
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #21)
> We send the app origin in the URL field for the desktop and Android web app
> runtimes as well, so if you believe we should do it differently here, then I
> call on you to submit the changes for those and for Socorro to handle the
> new fields correctly as well.

So I shouldn't have done what comment 16 suggested ? :-/
Comment 23 Robert Kaiser 2012-12-27 08:23:58 PST
(In reply to Hub Figuiere [:hub] from comment #20)
> I'll do a follow up patch for that.

As bug 821710 won't be done for v1 anyhow, let's not hold up this bug here for any considerations connected to this. We will not get browser tab URLs, so any URL field we get from B2G will be an app origin anyhow.
If you feel like addressing Justin's concern, please file a new bug, as said in comment #21, we need a number of other changes along with it, possibly.

(In reply to Hub Figuiere [:hub] from comment #22)
> So I shouldn't have done what comment 16 suggested ? :-/

Exactly, doing that creates a ton of followup problems that need to be solved and destroys any existing analysis we could apply to this.
Comment 24 Justin Lebar (not reading bugmail) 2012-12-27 08:54:46 PST
> How do I detect I'm in the browser process?

ContentParent::mIsForBrowser.

> Yes, as browser tabs don't send URLs anyhow yet.

Perhaps I'm missing something, but I don't think that speaks to the question.

browser tabs send an empty URL, right?

The B2G main process does not send a URL at all, empty or otherwise, as I understand.  The B2G main process does not have a corresponding ContentParent (because it has no parent process), so the code added here will not run if the B2G main process crashes.

I was asking if you can tell the difference between these two cases, so you can tell the difference between a crash which occurred in a browser process and a crash which occurred in the main process.
Comment 25 Hubert Figuiere [:hub] 2012-12-27 08:56:54 PST
Created attachment 696055 [details] [diff] [review]
Bug 806515 - Part 2: use the URL field for content crash.

Very trivial change
Comment 26 Robert Kaiser 2012-12-27 09:19:48 PST
(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #24)
> The B2G main process does not send a URL at all, empty or otherwise, as I
> understand.

That's no problem, we have very solid differentiation between main (gecko) and content processes throughout the crash reporting and analysis/stats stack. We already report the process type (empty for the main process, we call it "browser" in stats as that's what it is on desktop and mobile, "plugin" for plugin processes on desktop, "content" for app/tab processes on B2G or tabs on XUL Fennec).
We don't need further instrumentation to differentiate between main and content processes.

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #24)
> browser tabs send an empty URL, right?

Right.

AND, FYI, the main process may send no URL field but that's interpreted as empty on the server side, but as the process type is a separate field, this is nothing we need to care or worry about.

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #24)
> I was asking if you can tell the difference between these two cases, so you
> can tell the difference between a crash which occurred in a browser process
> and a crash which occurred in the main process.

Yes, we can as described above. Compare e.g. bp-19d3c4d6-3f11-46c3-a93e-5b7102121226 (gecko, no "Process Type" field listed) and bp-3b899d94-21a5-4162-85e5-e3ce72121226 (content, see "Process Type" field in position 4 in the list).
Of course, without the patch from this bug, I can't tell if the content report is from a browser tab or an app process (and in the latter case, which one).
Comment 27 Robert Kaiser 2012-12-27 09:21:29 PST
Comment on attachment 696055 [details] [diff] [review]
Bug 806515 - Part 2: use the URL field for content crash.

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

Thanks for this.
Comment 29 Michelle Luna 2012-12-27 11:26:20 PST
Hi,

Here is what we say about crash information on SUMO today:
https://support.mozilla.org/en-US/kb/send-plugin-crash-reports-help-improve-firefox#w_what-information-is-sent-in-a-crash-report

"What information is sent in a crash report?

Crash reports only include technical information to help Firefox developers determine what went wrong, and how to fix it. These reports do not include personal information. The information sent in a report includes:

    what webpage you were on
    version of Firefox you were using
    your operating system
    installed plugins
    installed extensions
    and more technical info. 

This information is subject to the Firefox Privacy Policy. "

-Michelle
Comment 30 Robert Kaiser 2012-12-27 12:22:55 PST
(In reply to Michelle Luna from comment #29)
> Here is what we say about crash information on SUMO today

Thanks, I have split off the documentation part to bug 821711, though, so I guess it's best to put this there.
Comment 31 Michelle Luna 2012-12-27 13:13:06 PST
Ok, thanks, I've copied this info over to 821711 and added the whiteboard there instead. Can we remove the whiteboard from this bug?
Comment 32 Robert Kaiser 2012-12-27 13:40:26 PST
(In reply to Michelle Luna from comment #31)
> Ok, thanks, I've copied this info over to 821711 and added the whiteboard
> there instead. Can we remove the whiteboard from this bug?

Thanks, and yes, I just didn't realize I should do that. Done now.
Comment 33 Daniel Holbert [:dholbert] 2012-12-27 14:01:25 PST
https://hg.mozilla.org/mozilla-central/rev/a13cadb983fb
Comment 35 Hubert Figuiere [:hub] 2012-12-28 09:14:50 PST
(In reply to Daniel Holbert [:dholbert] from comment #33)
> https://hg.mozilla.org/mozilla-central/rev/a13cadb983fb

We are still missing the second checkin into m-c
Comment 36 Graeme McCutcheon [:graememcc] 2012-12-29 04:35:25 PST
https://hg.mozilla.org/mozilla-central/rev/1ebc8df48c4c
Comment 37 Robert Kaiser 2012-12-30 18:15:53 PST
Verified. Reports like bp-ebedae09-9ad4-4ff6-be66-bdb2a2121229 now have a URL field with the app manifest URL, in this case it's app://camera.gaiamobile.org/manifest.webapp

Note You need to log in before you can comment on or make changes to this bug.