[User Story] Sheets Manager: Crashed window

VERIFIED FIXED in 2.1 S3 (29aug)

Status

defect
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: pdol, Assigned: alive)

Tracking

({uiwanted})

unspecified
2.1 S3 (29aug)
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.1, tracking-b2g:backlog, b2g-master verified)

Details

(Whiteboard: [ucid:System135, ft:systemsfe], system-browser)

Attachments

(1 attachment)

Reporter

Description

6 years ago
User Story:
As a user I want to be informed when a browser window has crashed due to an exception or low memory, so that I am not left wondering why a particular window disappeared.

Acceptance Criteria:
1. If a window crashes, I am informed of the crash.

Assumptions:
1. UX spec to define if crash prompt shows for non-foreground windows.
Component: Gaia::System → Gaia::System::Window Mgmt
Reporter

Updated

6 years ago
Component: Gaia::System::Window Mgmt → Gaia::System::Browser
No longer blocks: browser-chrome-mvp
Reporter

Updated

6 years ago
Part of task manager.

UX spec needed.
Component: Gaia::System::Browser Chrome → Gaia::System::Window Mgmt
Flags: needinfo?(fdjabri)
Keywords: uiwanted
Flags: in-moztrap?(nhirata.bugzilla)
Flags: in-moztrap?(nhirata.bugzilla) → in-moztrap?(jsmith)
No longer blocks: 1.4-systems-fe
Flags: in-moztrap?(jsmith)
Reporter

Comment 2

5 years ago
Francis, do we need this given that zombie apps should remain in the sheet stack?
blocking-b2g: --- → backlog
feature-b2g: --- → 2.1
(In reply to Peter Dolanjski [:pdol] from comment #2)
> Francis, do we need this given that zombie apps should remain in the sheet
> stack?

Note, we won't keep zombie sheets in case of crashes, only for out of memory kills.
Reporter

Updated

5 years ago
Whiteboard: [ucid:System135, 1.4:P2, ft:systems-fe], system-browser → [ucid:System135, ft:systemsfe], system-browser
Covered as part of the Task manager spec at https://mozilla.box.com/s/4pfp6y8pjhwojtt9yhl8
Flags: needinfo?(fdjabri)
Target Milestone: --- → 2.1 S3 (29aug)
Sam, do you know what if anything is left to do here, and what is the best home for this bug?
Flags: needinfo?(sfoster)
The task manager consumes StackManager.snapshot() - which is just a clone of its array of AppWindow instances. This is the same information consumed by the edge gestures. We can represent a OOM killed instance in that array, and revive it if the user selects it - this capability is already there in AppWindow, actually triggering it falls to the task manager. I'm aware of the requirement but I dont think its tracked in its own bug. As to if these are actually maintained in the StackManager, I'm not sure TBH and that's probably a question for Alive - I've not actually seen this case, but I'm not developing on a low memory device currently so it could be there already.
Flags: needinfo?(sfoster)
Not part of browser chrome
Blocks: 967420
No longer blocks: browser-chrome-mvp
Just assign an owner on user story bug. Candice, feel free to dispatch the bug to engineering managers or leads if it's much better for then to own the user story bugs. Thank you.
Assignee: nobody → cserran
Reporter

Comment 9

5 years ago
Alive, do we have a prompt that tells a user if a window crashes?
Francis, I'm not sure if this is covered in the spec.  The spec calls for showing killed tasks in the task manager, but Etienne indicates above that that only applies for out of memory cases, not crashes.  Also, what happens if the crash happens while the window is in the foreground?  
Maybe we cover this already, but I can't test it.
Flags: needinfo?(fdjabri)
Flags: needinfo?(alive)
(In reply to Peter Dolanjski [:pdol] from comment #9)
> Alive, do we have a prompt that tells a user if a window crashes?

Yes.

> Francis, I'm not sure if this is covered in the spec.  The spec calls for
> showing killed tasks in the task manager, but Etienne indicates above that
> that only applies for out of memory cases, not crashes.  Also, what happens
> if the crash happens while the window is in the foreground?  

We just close/remove the app and back to homescreen.

> Maybe we cover this already, but I can't test it.

We should not restart it for the user IMO.
Crash is critical and maybe some problem is there in the app, but killed by OOM doesn't mean the app is having problem.
Flags: needinfo?(alive)
Reporter

Comment 11

5 years ago
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> (In reply to Peter Dolanjski [:pdol] from comment #9)
> > Alive, do we have a prompt that tells a user if a window crashes?
> 
> Yes.

Thanks for confirming Alive.
Do you know what the UX looks like for this?  I've never seen it.
Flags: needinfo?(alive)
Assignee: cserran → alive
(In reply to Peter Dolanjski [:pdol] from comment #9)
> Alive, do we have a prompt that tells a user if a window crashes?
> Francis, I'm not sure if this is covered in the spec.  The spec calls for
> showing killed tasks in the task manager, but Etienne indicates above that
> that only applies for out of memory cases, not crashes.  

I spoke with Rob and we'd prefer to still show crashed apps in the Task manager, but are ok with not showing them.
Flags: needinfo?(fdjabri)
(In reply to Peter Dolanjski [:pdol] from comment #11)
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #10)
> > (In reply to Peter Dolanjski [:pdol] from comment #9)
> > > Alive, do we have a prompt that tells a user if a window crashes?
> > 
> > Yes.
> 
> Thanks for confirming Alive.
> Do you know what the UX looks like for this?  I've never seen it.

I don't have a screenshot and the ux spec link is broken.
The first you see crash report it will be a full screen dialog to tell you to send report or not.
After that you will see a bottom banner: 'Gallery is crashed' shows at the bottom of the screen if the foreground app is crashed.
Flags: needinfo?(alive)
(In reply to Francis Djabri [:djabber] from comment #12)
> (In reply to Peter Dolanjski [:pdol] from comment #9)
> > Alive, do we have a prompt that tells a user if a window crashes?
> > Francis, I'm not sure if this is covered in the spec.  The spec calls for
> > showing killed tasks in the task manager, but Etienne indicates above that
> > that only applies for out of memory cases, not crashes.  
> 
> I spoke with Rob and we'd prefer to still show crashed apps in the Task
> manager, but are ok with not showing them.

Since we have difficulty to tell crash from oom I think we will show all of kill-by-gecko app in task manager if we want.

Anyway, we are still diverged here. It's not task manager to manage anything.

Do we come to a conclusion that all background-killed app should be revived when launching from task manager or "being edge swiped"? If so, I am going to turn on the app.suspending settings forever and we could close this bug.
Flags: needinfo?(firefoxos-ux-bugzilla)
(In reply to Etienne Segonzac (:etienne) from comment #3)
> (In reply to Peter Dolanjski [:pdol] from comment #2)
> > Francis, do we need this given that zombie apps should remain in the sheet
> > stack?
> 
> Note, we won't keep zombie sheets in case of crashes, only for out of memory
> kills.

Well, I think we don't tell which is crash or oom. All of them is coming from mozbrowsererror...isn't it?
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> (In reply to Francis Djabri [:djabber] from comment #12)
> > (In reply to Peter Dolanjski [:pdol] from comment #9)
> > > Alive, do we have a prompt that tells a user if a window crashes?
> > > Francis, I'm not sure if this is covered in the spec.  The spec calls for
> > > showing killed tasks in the task manager, but Etienne indicates above that
> > > that only applies for out of memory cases, not crashes.  
> > 
> > I spoke with Rob and we'd prefer to still show crashed apps in the Task
> > manager, but are ok with not showing them.
> 
> Since we have difficulty to tell crash from oom I think we will show all of
> kill-by-gecko app in task manager if we want.
> 
> Anyway, we are still diverged here. It's not task manager to manage anything.
> 
> Do we come to a conclusion that all background-killed app should be revived
> when launching from task manager or "being edge swiped"? If so, I am going
> to turn on the app.suspending settings forever and we could close this bug.

Rephrase:

We already have the ability to keep any killed-by-gecko-at-background apps in the stack instead of removing.
But we are turning it off right now. *Note: nothing to do with task manager.*

Do you want me to turn it on from now on?

I forgot the bug but we had a discussion in another bug for the same thing.
Reporter

Comment 17

5 years ago
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #16)
> Rephrase:
> 
> We already have the ability to keep any killed-by-gecko-at-background apps
> in the stack instead of removing.
> But we are turning it off right now. *Note: nothing to do with task manager.*
> 
> Do you want me to turn it on from now on?
> 
> I forgot the bug but we had a discussion in another bug for the same thing.

Yes, I think we should.  However, is there a risk of instability/regressions from doing this?  We are very close to FL, so I don't want it to cause serious issues.
Flags: needinfo?(alive)
QA, how do you want to handle this? As this setting has defaulted off, and given the nature of it there is inevitably some risk of instability and regression from turning it on. But, without the ability to keep killed apps in the stack, navigating sheets via either the task manager or edge gesture is close to useless on a low-memory device. Perhaps instead of landing and compounding the churn we'll see this week it would be worth turning it on on test device and putting it through its paces before determining if its safe to land for 2.1?
Flags: needinfo?(mozillamarcia.knous)
This is the 'app-suspending.enabled' setting, exposed on the Developer settings menu as 'App suspending'.
We're getting very late in the release cycle, can we turn this on today?
Patch coming.
Flags: needinfo?(alive)
A Long-time-no-see small patch
Attachment #8480373 - Flags: review?(etienne)
I think we need to kill all front windows if it's suspending...added unit test for it.
Comment on attachment 8480373 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/23401

Let the zombie invasion begin :)
Attachment #8480373 - Flags: review?(etienne) → review+
https://github.com/mozilla-b2g/gaia/commit/507d99b3739e626fb5c0de76a884905b828b5d60
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Ni outlived its usefulness, Marcia - just consider it a heads-up
Flags: needinfo?(mozillamarcia.knous)

Comment 27

5 years ago
I'm filing a bug with the Bugzilla people. All of our ni? flags are NOT showing up in our queries or in my badge number. Wonderful.

Comment 28

5 years ago
Flagging Francis for review.
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(fdjabri)
Thanks, this works according to spec.
Flags: needinfo?(fdjabri)
Verifying fix on flame 3.0 devices
Results: Apps that die from OoM or a crash will remain in the task manager, and may be revived/relaunched from their previous position.

--------------------------------------------------
Environmental Variables:
Device: Flame 3.0
BuildID: 20150210010523
Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko: 2cb22c058add
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
--------------------------------------------------
Repro: 7/7
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
blocking-b2g: backlog → ---
(In reply to Oliver Nelson [:oliverthor] from comment #30)
> Verifying fix on flame 3.0 devices
> Results: Apps that die from OoM or a crash will remain in the task manager,
> and may be revived/relaunched from their previous position.
> 
> --------------------------------------------------
> Environmental Variables:
> Device: Flame 3.0
> BuildID: 20150210010523
> Gaia: 0cf517083f7eb5fc269e1236edba50534f65e3cd
> Gecko: 2cb22c058add
> Gonk: e7c90613521145db090dd24147afd5ceb5703190
> Version: 38.0a1 (3.0) 
> Firmware Version: v18D-1
> User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
> --------------------------------------------------
> Repro: 7/7

It's not working in current builds anymore, I filed bug 1176596 for it.
You need to log in before you can comment on or make changes to this bug.