935750 - (suspending-app) [Window Management] Keep crashed app slept and revive it once being opened again.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

I noted that if an app is killed after we open the cardview, the card is still there and click on the card doesn't do anything.
We could: dynamically removing the card(with animation?) when the app is terminated at background OR
Keep the configuration of the appWindow and "relaunch" the app when user click the "card".

Comment 1

[Etienne Segonzac (:etienne)]

We will get this for free with bug 939809 since the StackManager knows when an app is killed.

Maybe we should rename here to represent the work on "relaunch".

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Etienne Segonzac (:etienne) from comment #1)
> We will get this for free with bug 939809 since the StackManager knows when
> an app is killed.

I think we could just use AppWindow.prototype.isDead implemented by you and implement AppWindow.prototype.relaunch
I don't see the hard block from bug 939809 now.

> 
> Maybe we should rename here to represent the work on "relaunch".

Renamed

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

A more simple fix:
Card should knows the app config, so it just needs to tell AppWindowFactory/AppWindowManager to launch the same app with the same config when the user is clicking on the dead card.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

I had the patch but the problem is how should I re-instantiate the Wrapper :/
Because the flow of wrapper launch is a little messy now.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/14828

WIP, needs tests.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

I decide to drop this idea because the call path is somewhat weird. Deassign first.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Have a patch working now. Take back.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/16061

Let's see if etienne like this.

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8371910 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16061

Frankly Alive you are my hero :)

I'm having issues with the screenshot caching (some previously zombied app never get screenshoted again)... but the main feature works wonderfully!

I think we should land the main feature ASAP without the screenshot/caching part and open a follow up where we get UX input for the transition and figure out the details.

What do you think?

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

It's fine for me but let me struggle for a while :)
I am trying to do: render the background with (1) icon splash again or (2) the cached screenshot URL.
So we don't need to change AppWindow.prototype._showScreenshotOverlay and _hideScreenshotOverlay to avoid the issue.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8371910 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16061

Second struggle ;)
* Render the container background with screenshot cache if any instead of using showScreenshotOverlay.

Comment 12

[Etienne Segonzac (:etienne)]

Comment on attachment 8371910 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16061

Never thought I would say that, but:

r- because of a risk of zombie apocalypse.

:)

This is a pretty awesome feature for devices with only 256MB of RAM, I'm trying it on a buri with great success, to make it even better:

* We need to remove the screenshot in element.backgroundImage when we revive the app.

* We shouldn't keep more than 10 zombie apps (UX agreed on this number), otherwise things could go crazy especially with the current card view :/

Note sure what's the best way to implement the second item, but we can do something simple at first and fine tune it later.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Etienne Segonzac (:etienne) from comment #12)
> r- because of a risk of zombie apocalypse.

BRAINNNNNNNNNNNNNNN

> * We shouldn't keep more than 10 zombie apps (UX agreed on this number),
> otherwise things could go crazy especially with the current card view :/
> 
> Note sure what's the best way to implement the second item, but we can do
> something simple at first and fine tune it later.

The first thing come to my mind is an App-Window-Priority-Manager to actively kill background appWindow by launching time :) Maybe reference to stackManager.

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8371910 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16061

Hope it's last run!
* Replace all zombie with suspending to avoid confusion. Love 'zombie' but you know that. BRAAAAINNNNNN
* Add SuspendingAppPriorityManager to active kill eldest suspending app.
* Force removing backgroundImage when app is loaded.
* More unit tests.

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 8371910 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16061

r=me with the few nits addressed and the small follow ups filed.

glad the zombie apocalypse was avoided, very glad to see fast app switching being useful on a buri :)

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

https://github.com/mozilla-b2g/gaia/commit/5c33197f62c17adb9c51f9e6539d51a4a69e928e

Thanks Etienne for purging zombies!!

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This feature was being discussed for possible uplift to 1.3T to provide better experience.

The master/1.4 patch is quite complex so it clear that we cannot do simple uplift for it. Alive, would you be able to provide a risk assessment and LOE for re-do this on 1.3T?

Comment 21

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #20)
> This feature was being discussed for possible uplift to 1.3T to provide
> better experience.
> 
> The master/1.4 patch is quite complex so it clear that we cannot do simple
> uplift for it. Alive, would you be able to provide a risk assessment and LOE
> for re-do this on 1.3T?

LOE is about one week, and since we tend not to take any screenshot in tarako,
some cardview logic needs to be changed to reflect, e.g. put app icon as screenshot layer.
Some life cycle event handler needs to be moved into the window.js from window_manager.js

SuspendingAppPriorityManager is still needed because it's easy to have too many killed apps.

I'm not sure if this feature will conflict with reviving music from system work Dominic had already implemented for tarako.

Comment 22

[Joe Cheng [:jcheng] (please needinfo)]

triage: This feature is too risky to uplift to 1.3T+. minus

Comment 23

[Ivan Tsay (:ITsay)]

Hi Alive,

Does v1.4 contain this fix?

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Ivan Tsay (:ITsay) from comment #23)
> Hi Alive,
> 
> Does v1.4 contain this fix?

No.

Comment 25

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.

Comment 26

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

Not sure if this is suitable for 1.4.  Do you know if we should have this for 1.4, Alive?
ni? james as a fyi.

Comment 27

[James Zhang (Spreadtrum)]

I think bug 1005787 patch is better.
Alive, what do you think about?

Comment 28

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

Ah ok.  Thanks James.  I'll leave it up to you and Alive to figure out.  :)

Comment 29

[(inactive after 6/18) Alive Kuo [:alive]]

Bug 1005787 is totally different from this; this one is a feature and bug 1005787 is a bug.
The feature is talking about: keep the crashed app even it's killed by OOM at background.
The bug is talking about: relaunch the app once the app is killed "only" during cardsview is opened.

This feature is still pref-off in master now so I would say no for v1.4

Comment 30

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

Thanks Alive.

Comment 31

[Oliver Nelson [:oliverthor]]

Verifying fix on flame 3.0 devices.
Results: When apps OoM, they will remain in the task manager as zombie apps, where attempting to access them will effectively relaunch the app. These apps maintain their previous screenshot while in card, but on open relaunch and thus do not retain their previous screenshot state.

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