Closed Bug 829477 Opened 12 years ago Closed 12 years ago

Manual crash submission leads to misleading UX

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: cjones, Assigned: hub)

Details

(Whiteboard: UX-P?)

Attachments

(1 file, 1 obsolete file)

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.
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.
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.
(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.
Assignee: nobody → hub
(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.
Attached patch patch (untested) (obsolete) — Splinter Review
Attachment #700948 - Attachment is patch: true
blocking-basecamp: ? → +
Priority: -- → P3
Target Milestone: --- → B2G C4 (2jan on)
cjones: I debated about submitting this as a bug, but I never did.  It's not a dup.
It is on my plate. Margaret patches looks right, I'm rebuilding and testing, etc. I'll get this rollin' quick.
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
Component: Gaia::System → General
(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.
> 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.
Margaret patch tested, etc.
Attachment #700948 - Attachment is obsolete: true
Attachment #700986 - Flags: review?(fabrice)
(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.
QA note: the easy way to test it is to run "membuster". It is part of the test applications.
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
Attachment #700986 - Flags: review?(fabrice) → review+
> Nit : you could add that this also prevent showing UI for OOM crashes

How do you guys determine whether it's an OOM?
(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.
marking as fixed as per jst new rules.
(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"?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: