829477 - Manual crash submission leads to misleading UX

Status: RESOLVED FIXED

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

Description

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

I thought tchung had filed this, apologies if a dup.

STR
 (1) Opt out of automatic crash submission (settings)
 (2) Trigger a crash; |kill -11| some gecko process
 (3) Choose not to submit the crash
 (4) Use the phone normally for a while

After step (4), we're left with a pending crash report.  Every time a background app is killed because of memory pressure, gaia notifies "An app has crashed".  This is popped over whatever app happens to be foreground at that moment.

This is extremely annoying but also very misleading to the user.

Comment 1

[:Margaret Leibovic]

We're getting a crash notification from gecko when background apps are killed because of memory pressure? I thought that wasn't supposed to happen. It doesn't seem valid for the crash reporter to be generating crash IDs for memory pressure kills. Could this be a regression?

This banner was implemented in bug 801925 according to the spec, but the banner should only show up if there's actually a real crash.

Comment 2

[:Margaret Leibovic]

Urg, I think this might be a regression from bug 821498. Right now, in shell.js's reportCrash, we can still end up notifying gaia of a crash even if there isn't a valid crashID:
http://hg.mozilla.org/mozilla-central/annotate/8dd9b338324c/b2g/chrome/content/shell.js#l106

I think we need to add an "|| !crashID" in there.

Comment 3

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #2)

> I think we need to add an "|| !crashID" in there.

Actually, that wouldn't work, since that would regress the fix from bug 821498. It seems like we just need some logic to deal with the fact that we shouldn't notify gaia if we're not submitting a report for a crash that just happened.

I'd also like to know why we're going through this reportCrash path on memory pressure kills. I'm not sure that was always the case.

Comment 4

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #3)

> I'd also like to know why we're going through this reportCrash path on
> memory pressure kills. I'm not sure that was always the case.

Oh, we always call that on 'mozbrowserloadstart'. I should make sure to read the code before making comments :)

Hub, I have an untested patch, if you want to try it. I can't build the crash reporter for the device right now, so it's probably faster for you to test.

Comment 5

[:Margaret Leibovic]

Attachment: patch (untested)

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

cjones: I debated about submitting this as a bug, but I never did.  It's not a dup.

Comment 7

[Hubert Figuiere [:hub]]

It is on my plate. Margaret patches looks right, I'm rebuilding and testing, etc. I'll get this rollin' quick.

Comment 8

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

Let me artificially bump platform bugs count since this bug lives in $GECKO/b2g/chrome/content/shell.js and not in $GAIA. Moving to B2G -> General

Comment 9

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

(In reply to :Margaret Leibovic from comment #1)
> We're getting a crash notification from gecko when background apps are
> killed because of memory pressure? I thought that wasn't supposed to happen.
> It doesn't seem valid for the crash reporter to be generating crash IDs for
> memory pressure kills. Could this be a regression?

Gecko will observe that the process exited abnormally, but I'm not really sure what happens after that in terms of mozbrowser and shell.js notifications.

Comment 10

[Justin Lebar (not reading bugmail)]

> We're getting a crash notification from gecko when background apps are
> killed because of memory pressure? I thought that wasn't supposed to happen.

It is certainly intended to happen; mozbrowser should send mozbrowsererror type "fatal" on any crash of the child process.

Comment 11

[Hubert Figuiere [:hub]]

Attachment: Bug 829477 - Don't notify the UI if we didn't have crash ID.

Margaret patch tested, etc.

Comment 12

[:Margaret Leibovic]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> (In reply to :Margaret Leibovic from comment #1)
> > We're getting a crash notification from gecko when background apps are
> > killed because of memory pressure? I thought that wasn't supposed to happen.
> > It doesn't seem valid for the crash reporter to be generating crash IDs for
> > memory pressure kills. Could this be a regression?
> 
> Gecko will observe that the process exited abnormally, but I'm not really
> sure what happens after that in terms of mozbrowser and shell.js
> notifications.

I just forgot how the shell.js logic works. I was confused because we call a function named reportCrash() when we want to maybe report a crash, not necessarily always after a crash happens. I sorted it out in a conversation with myself above :)

I believe the crash reporter code takes care of not generating a crash to submit when it's a memory pressure kill, and that's why this was working correctly before.

Comment 13

[Hubert Figuiere [:hub]]

QA note: the easy way to test it is to run "membuster". It is part of the test applications.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 700986 [details] [diff] [review]
Bug 829477 - Don't notify the UI if we didn't have crash ID.

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

::: b2g/chrome/content/shell.js
@@ +122,5 @@
>      } catch (e) { }
>  
> +    // We can get here if we're just submitting old pending crashes.
> +    // Check that there's a valid crashID so that we only notify the
> +    // user if a crash just happened. Bug 829477

Nit : you could add that this also prevent showing UI for OOM crashes

Comment 15

[Justin Lebar (not reading bugmail)]

> Nit : you could add that this also prevent showing UI for OOM crashes

How do you guys determine whether it's an OOM?

Comment 16

[Hubert Figuiere [:hub]]

(In reply to Justin Lebar [:jlebar] from comment #15)
> > Nit : you could add that this also prevent showing UI for OOM crashes
> 
> How do you guys determine whether it's an OOM?

We don't determine it is an OOM, but we determine it is not a crash. That's all we need to know.

Comment 17

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa0bd5330c92
https://hg.mozilla.org/releases/mozilla-b2g18/rev/bfa8019c4d4e

Comment 18

[Hubert Figuiere [:hub]]

marking as fixed as per jst new rules.

Comment 19

[Justin Lebar (not reading bugmail)]

(In reply to Hub Figuiere [:hub] from comment #16)
> We don't determine it is an OOM, but we determine it is not a crash. That's
> all we need to know.

How do you decide what is and isn't a "crash"?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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