Closed Bug 1039984 Opened 10 years ago Closed 10 years ago

[System] Trying to open a smartcollection after killing the Homescreen do nothing.

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S1 (1aug)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: salva, Assigned: salva)

Details

(Whiteboard: [systemsfe])

Attachments

(1 file)

Following the STR for bug 1034301, I discovered the following issue in v2.0 and master:

STR:
 1. Go to homescreen.
 2. Open a smart collection.
 2. Simulate the OOM killer terminates the Homescreen* while displaying the SmartCollection.
 3. Wait for Homescreen to be re-launched.
 4. Open a smart collection

Expected:
The smart collection opens.

Actual:
Nothing happens.

* Via adb shell, make kill -15 <homescreen_pid>
Whiteboard: [systemsfe]
The patch is somehow related with bug 1029335 and bug 1037041.
Attachment #8457891 - Flags: review?(alive)
Attachment #8457891 - Flags: feedback?(aus)
blocking-b2g: 2.0? → 2.0+
Target Milestone: --- → 2.0 S6 (18july)
Comment on attachment 8457891 [details] [review]
Allow Homescreen application window to be killed as other windows.

Could you elaborate more?
Comment on attachment 8457891 [details] [review]
Allow Homescreen application window to be killed as other windows.

The code looks fine, but please explain why this fixes the problem.
Is it because homescreenWindow.frontWindow is not removed after homescreen is killed by OOM?
Attachment #8457891 - Flags: review?(alive) → feedback+
Of course, sorry.

In general perspective, the Homescreen application acts like a regular application with the only difference it will never have next, previous and read applications as it is the base of its own stack. But it could have front applications result of activities initiated from the Homescreen (SmartCollections and wallpaper change). As additional point, the Homescreen should never be marked as killed because from the SystemApp perspective it is never killed (I mean, Gecko can kill the process but all the scaffolding to visually support Homescreen is never destroyed).

In particular, without this patch, killing the Homescreen resulted in not destroying references to the top activity so further requests to the same activity' origin did nothing [1]. Even more, as the top applications was never asked to kill itself, the element remain there but detached from DOM (and I think this is the cause of element being null in bug 1029335).

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/child_window_factory.js#L168
Comment on attachment 8457891 [details] [review]
Allow Homescreen application window to be killed as other windows.

Thanks for the comment. Makes sense to me.
Attachment #8457891 - Flags: feedback?(aus) → feedback+
(In reply to Salvador de la Puente González [:salva] from comment #4)
> Of course, sorry.
> 
> In general perspective, the Homescreen application acts like a regular
> application with the only difference it will never have next, previous and
> read applications as it is the base of its own stack. But it could have
> front applications result of activities initiated from the Homescreen
> (SmartCollections and wallpaper change). As additional point, the Homescreen
> should never be marked as killed because from the SystemApp perspective it
> is never killed (I mean, Gecko can kill the process but all the scaffolding
> to visually support Homescreen is never destroyed).
> 
> In particular, without this patch, killing the Homescreen resulted in not
> destroying references to the top activity so further requests to the same
> activity' origin did nothing [1]. Even more, as the top applications was
> never asked to kill itself, the element remain there but detached from DOM
> (and I think this is the cause of element being null in bug 1029335).
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> child_window_factory.js#L168

The explanation above and the change of removing special kill() for homescreen window makes sense.

But what I don't understand is, the kill() in appWindow should remove the frontWindow reference from the dead homescreenWindow. Could you point me out why it is not working?
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L414

Thanks for working on this.
(In reply to Alive Kuo [:alive][NEEDINFO!][Berlin 7/14-7/18] from comment #6)
> (In reply to Salvador de la Puente González [:salva] from comment #4)
> > Of course, sorry.
> > 
> > In general perspective, the Homescreen application acts like a regular
> > application with the only difference it will never have next, previous and
> > read applications as it is the base of its own stack. But it could have
> > front applications result of activities initiated from the Homescreen
> > (SmartCollections and wallpaper change). As additional point, the Homescreen
> > should never be marked as killed because from the SystemApp perspective it
> > is never killed (I mean, Gecko can kill the process but all the scaffolding
> > to visually support Homescreen is never destroyed).
> > 
> > In particular, without this patch, killing the Homescreen resulted in not
> > destroying references to the top activity so further requests to the same
> > activity' origin did nothing [1]. Even more, as the top applications was
> > never asked to kill itself, the element remain there but detached from DOM
> > (and I think this is the cause of element being null in bug 1029335).
> > 
> > [1]
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> > child_window_factory.js#L168
> 
> The explanation above and the change of removing special kill() for
> homescreen window makes sense.
> 
> But what I don't understand is, the kill() in appWindow should remove the
> frontWindow reference from the dead homescreenWindow. Could you point me out
> why it is not working?
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.
> js#L414
> 
> Thanks for working on this.

Let me clarify again: use AppWindow.prototype.kill to replace HomescreenWindow.prototype.kill makes sense to me. What I don't see is why you do not turn on _killed for homescreen? Any problem you see if you don't do it?
(In reply to Alive Kuo [:alive][NEEDINFO!][Berlin 7/14-7/18] from comment #7)
> Let me clarify again: use AppWindow.prototype.kill to replace
> HomescreenWindow.prototype.kill makes sense to me. What I don't see is why
> you do not turn on _killed for homescreen? Any problem you see if you don't
> do it?

Yes. The flag `_killed` its only used to avoid triggering the killing process for the same application twice. This is why you have a sentinel in [1]. In [2] you say when receiving a crash, kill the application. An application is not intended to automatically revive unless manually restarted. This works fine but for the Homescreen, the crash is handled in a different way. For Homescreen, the application is restarted instead [3].

Restarting process takes two paths depending on the visibility of the Homescreen [4]. If visible, it is killed, then re-rendered. If not, it is simply killed and the restart is delayed for when the Homescreen regains visibility.

Now notice the DOM elements supporting any application are detached from DOM in the destroy() method [5] but unlike normal applications, the memory object supporting the Homescreen is never garbage collected. From a System APP perspective, the model for the Homescreen window is never destroyed, although the view is not present.

If you don't prevent the object to be marked as `_killed`, then killing the Homescreen a second time (totally probable as the OOM killer can pass again) result in nothing due to the sentinel in [1], as the `_killed` lock is never removed. The solution is to distinguis the Homescreen case or add an unlock `delete this._killed` somewhere (maybe in render() [6]) but I preferred to distinguish the special case as a normal application can not revive.

One last solution is to split the `kill()` method into several responsibilites: marking as killed, detaching windows, destroying the window. Then we could override the method in Homescreen avoiding marking as killed but leaving detaching and destruction.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L391
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L811
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/homescreen_window.js#L126
[4] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/homescreen_window.js#L143
[5] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L479
[6] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L514
Comment on attachment 8457891 [details] [review]
Allow Homescreen application window to be killed as other windows.

Give r+ because after thinking again the _killed flag is the old behavior of homescreen window so it's my turn to think about this.
Thanks for fixing it.
Attachment #8457891 - Flags: review+
(In reply to Alive Kuo [:alive][NEEDINFO!][Berlin 7/14-7/18] from comment #9)
> Comment on attachment 8457891 [details] [review]
> Allow Homescreen application window to be killed as other windows.
> 
> Give r+ because after thinking again the _killed flag is the old behavior of
> homescreen window so it's my turn to think about this.
> Thanks for fixing it.

You're welcome.
I changed the commit comment and pushed to restart Travis waiting for it to be green.
Status: NEW → ASSIGNED
Master: 1e0ffb750bb4011a145fe0c8d05c8684e25ae48f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
The issue fixed on 2.0, 2.1 and 2.2

The smart collection opens ofter OOM crash

"Flame 2.0

Device: Flame 2.0 (319mb)(Kitkat Base)(Shallow Flash)
Build ID: 20141118000207
Gaia: 1ede2666f1e6c1b3fd3b282011caf0cbc59544b0
Gecko: 2bea026d4f86
Version: 32.0 (2.0)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0"

"Flame 2.1 

Device: Flame 2.1 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141118001204
Gaia: 1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko: 95fbd7635152
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0"

"Flame 2.2

Device: Flame 2.2 (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20141118040205
Gaia: 4aee256937afe9db2520752650685ba61ce6097d
Gecko: 7913c9392c5f
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0"
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: