821710 - Send the tab URL for B2G browser content crashes

Status: RESOLVED FIXED

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

Description

[Robert Kaiser]

Bug 806515 is implementing the sending of the app origin with crash reports for processes that run an app, I'm splitting off to this bug the part about sending the currently active URL of crashing tab processes in the browser.

I hope that should complete us sending useful info in the URL field of all B2G content processes - or do we have others?

Comment 1

[Justin Lebar (not reading bugmail)]

This bug (or something like it) may be necessary in order for us to distinguish between crashes in browser process and crashes in the main process.  See bug 824658 comment 19.

Comment 2

[Justin Lebar (not reading bugmail)]

Please don't use the same field ("URL") for the browser's current URI and the app's manifest URI.  That would be confusing, because it would give us no way to filter between app process crashes and browser process crashes.

Note also that a browser process may have multiple current URIs.  At the moment we have one TabChid per browser process, but that may change.  You should iterate over all the crashed ContentParent's TabParents and find their URIs (which you may have to send up to the parent; I'm not sure they're there at the moment).

Comment 3

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

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #1)
> This bug (or something like it) may be necessary in order for us to
> distinguish between crashes in browser process and crashes in the main
> process.  See bug 824658 comment 19.

Main process crashes have the module "b2g" loaded, and browser processes have "plugin-container" loaded.

Comment 4

[Robert Kaiser]

We also have the process type field that tells us easily what's a content process and what is the main process crashing, we don't need those workarounds for that.

Comment 5

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

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #2)
> Note also that a browser process may have multiple current URIs.  At the
> moment we have one TabChid per browser process, but that may change.  You
> should iterate over all the crashed ContentParent's TabParents and find
> their URIs (which you may have to send up to the parent; I'm not sure
> they're there at the moment).

We can have multiple tabchilds because of popups.

But in desktop, we just send the most-recently-navigated URI.  I think that code will sort of accidentally Just Work even in remote browser processes with multiple tabs.

Comment 6

[Robert Kaiser]

(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #2)
> You
> should iterate over all the crashed ContentParent's TabParents and find
> their URIs (which you may have to send up to the parent; I'm not sure
> they're there at the moment).

We should always send only one URL, as that's all that our analysis supports. As comment #5 explains, we send the most recently navigated URI on desktop or mobile right now, if we have multiple URLs, we should do the same. In most cases, that's enough for analysis.

Comment 7

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

I think this should already be working.  Let me check right quick.

Comment 8

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

bp-9a5a5ec5-c453-46d2-9999-c9de62121227 --- nope.  *scratches head*

http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/nsExceptionHandler.cpp#292

294 static const char* kSubprocessBlacklist[] = {
295   "FramePoisonBase",
296   "FramePoisonSize",
297   "StartupTime",
298   "URL"
299 };

Whups!

Comment 9

[Dale Harvey (:daleharvey)]

cjones? does that just mean we need to remove the url field from the blacklist to get this fixed? 

And how do I force a crash so I can test this out?

Comment 10

[Justin Lebar (not reading bugmail)]

> And how do I force a crash so I can test this out?

$ adb shell
# b2g-ps
# kill -9 pid-of-browser-process

Comment 11

[Josh Matthews [:jdm]]

(In reply to Dale Harvey (:daleharvey) from comment #9)
> cjones? does that just mean we need to remove the url field from the
> blacklist to get this fixed? 

I don't think that's the best solution, since it could be prone to races based on whichever caller happened to set the annotation last. I suspect it would be better to have TabParent actors set the annotation correctly in ActorDestroy, and ensure that ContentParent doesn't overwrite it.

Comment 12

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

The original problem that motivated the blacklist is that plugin subprocesses crash and generate crash reports, but those reports would also pick up annotations from the parent process.  We want most of those parent-process annotations, except the ones in the blacklist.

For content processes however, all these annotations are supplied by the content process and are useful.  (Except probably StartupTime ...)  So we need to ensure that the annotations provided by the content processes "override" the ones from the parent process, and that we don't apply the blacklist when writing the .extra file.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The solution seems like a platform bug. Moving to General.

Comment 14

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

Two things seem to be happening here
 - the Annotate() function being used by CrashReporterParent seems to be bypassing the child-process blacklist.  Whups.
 - the code added by bug 806515 is unconditionally stomping the "URL" annotation with mAppManifestURL.  For "browser" processes, this will be the empty string.

So even though browser processes (should) already be annotating the URL correctly, and that annotation shouldn't be filtered by the blacklist, it will be stomped by the code in bug 806515.

Comment 15

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

Attachment: Send the URL along with "browser" process crashes

So it turns out the frontend is responsible for annotating URL in the common case.  This patch lets TabChild serve that role in a simple way.

This also prefers the manual app-manifest annotation over whichever URI happened to have been loaded last in an app process.

Comment 16

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

Kill browser process

URL=http://www.bing.com/

killing dialer process

URL=app://communications.gaiamobile.org/manifest.webapp

Comment 17

[Robert Kaiser]

The UI process of the browser app still sends it app manifest, though, right?

Comment 18

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

The browser UI runs in-process.

Comment 19

[Robert Kaiser]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #18)
> The browser UI runs in-process.

Ah, OK.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 698077 [details] [diff] [review]
Send the URL along with "browser" process crashes

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

::: dom/ipc/ContentParent.cpp
@@ +716,5 @@
> +            // loaded in the child process with our manifest URL.  We
> +            // would rather associate the crashes with apps than
> +            // random child windows loaded in them.
> +            //
> +            // XXX would be nice if we could get both ...

If you want both, file a followup to send the manifest URL in a separate annotation. We'd have to hook up some Socorro UI to display it (which is probably why URL was used here), but that would let you get the actual URL that's loaded out of webapps, which would be nice.

Comment 21

[Robert Kaiser]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20)
> If you want both, file a followup to send the manifest URL in a separate
> annotation. We'd have to hook up some Socorro UI to display it (which is
> probably why URL was used here), but that would let you get the actual URL
> that's loaded out of webapps, which would be nice.

It would probably take us a few months to get such an additional field displayed in the Socorro UI (judging by how much is on their plans and the current state of Socorro UI development), and we'd need to change the desktop and mobile WebRT implementations to also send the app origin/manifest in the new field, or else we'd be heavily inconsistent in how we send that info across products.
We can revisit this later, but for at least v1.x of FxOS, I heavily prefer the current state of having this in the UI field as that gives us at least the chance to have decent crash analysis via Socorro.

Comment 22

[Johnny Stenback (:jst)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/236eec8d205d

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

This changeset appears to have broken the Android build on Mac:

/Users/dmose/r/inbound/src/dom/ipc/TabChild.cpp:33:32: fatal error: nsExceptionHandler.h: No such file or directory

I have --disable-crashreporter in my mozconfig, because <https://wiki.mozilla.org/Mobile/Fennec/Android#Setup_Fennec_mozconfig> says this is required.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: bracket inclusion of nsExceptionHandler.h with MOZ_CRASHREPORTER ifdef

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Thanks for the quick review, Chris.  Landed on mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9de5e35606d2

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/236eec8d205d

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9de5e35606d2

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/12da69f9e88a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/245231194d06