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)
Tracking
(blocking-b2g:2.0+, 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>
Updated•10 years ago
|
Whiteboard: [systemsfe]
Assignee | ||
Comment 1•10 years ago
|
||
The patch is somehow related with bug 1029335 and bug 1037041.
Attachment #8457891 -
Flags: review?(alive)
Attachment #8457891 -
Flags: feedback?(aus)
Updated•10 years ago
|
blocking-b2g: 2.0? → 2.0+
Updated•10 years ago
|
Target Milestone: --- → 2.0 S6 (18july)
Comment 2•10 years ago
|
||
Comment on attachment 8457891 [details] [review] Allow Homescreen application window to be killed as other windows. Could you elaborate more?
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
(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.
Comment 7•10 years ago
|
||
(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?
Assignee | ||
Comment 8•10 years ago
|
||
(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 9•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
(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
Assignee | ||
Comment 11•10 years ago
|
||
Master: 1e0ffb750bb4011a145fe0c8d05c8684e25ae48f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 12•10 years ago
|
||
v2.0: https://github.com/mozilla-b2g/gaia/commit/382dc87a9bbbf17f32ee60fdf4c6351a208b0833
status-b2g-v2.0:
--- → fixed
status-b2g-v2.1:
--- → fixed
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Comment 13•10 years ago
|
||
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?]
status-b2g-v2.2:
--- → verified
Flags: needinfo?(ktucker)
Updated•10 years ago
|
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.
Description
•