806515 - Send an app identifier for B2G content crashes

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Robert Kaiser]

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]]

I can take that one if you want.

Comment 2

[Robert Kaiser]

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]

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]

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]

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] (NeedInfo me for attention)]

This is on hub's plate unless someone beats him to it.

Comment 7

[Robert Kaiser]

(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]

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]

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]]

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]

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]

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]]

How are things going here, Hub?

Comment 14

[Hubert Figuiere [:hub]]

Attachment: Bug 806515 - Send app identifier for B2G content.

Comment 15

[Hubert Figuiere [:hub]]

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 [:bzbarsky]]

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]]

Will do. Thanks !

Comment 18

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a13cadb983fb

Comment 19

[Justin Lebar (not reading bugmail)]

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]]

(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]

(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]]

(In reply to Robert Kaiser (:[e-mail]) 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]

(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)]

> 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]]

Attachment: Bug 806515 - Part 2: use the URL field for content crash.

Very trivial change

Comment 26

[Robert Kaiser]

(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]

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 28

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ebc8df48c4c

Comment 29

[Michelle Luna]

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]

(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]

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]

(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]]

https://hg.mozilla.org/mozilla-central/rev/a13cadb983fb

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29df7a6bc630
https://hg.mozilla.org/releases/mozilla-aurora/rev/3111a0069387

https://hg.mozilla.org/releases/mozilla-b2g18/rev/35c0f583a45c
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a2fa5558e944

Comment 35

[Hubert Figuiere [:hub]]

(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]]

https://hg.mozilla.org/mozilla-central/rev/1ebc8df48c4c

Comment 37

[Robert Kaiser]

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