Closed
Bug 1112987
Opened 10 years ago
Closed 10 years ago
[FFOS7715 v2.1][Gaia_System_WindowMgmt] after monkey test phone stay at browser or other apps, couldn't back to homescreen
Categories
(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ben.song, Unassigned)
References
Details
Attachments
(3 files, 3 obsolete files)
Reproduce step: 1.phone start with 12 hours monkey test. Ideal result: Phone stay at browser or other apps, couldn't back to homescreen. Actual result: Phone could back to homescreen. Reproduce rate: 2/9
Dear Vance,Shawn,Luke, Base on my analysis, I found there are some problem during SheetsTransition.You could see this in the attachment.
Flags: needinfo?(vchen)
Flags: needinfo?(sku)
Flags: needinfo?(luke)
Dear Vance,Shawn,Luke, I think we should add some code at isActive() of app_window.js like this. AppWindow.prototype.isActive = function aw_isActive() { if (!this.element) { return false; } if (this.element.classList.contains('will-become-active')) { return true; } if (this.element.classList.contains('will-become-inactive')) { return false; } if (this.transitionController) { return (this.transitionController._transitionState == 'opened' || this.transitionController._transitionState == 'opening'); } else { // Before the transition controller is inited return false; } };
Ni the correct luke here Hi Ben Song, could you elaborate more on your modification? Thanks Vance
Flags: needinfo?(vchen)
Flags: needinfo?(sku)
Flags: needinfo?(luke)
Flags: needinfo?(lchang)
Flags: needinfo?(ben.song)
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #3) > Ni the correct luke here > > Hi Ben Song, could you elaborate more on your modification? > > Thanks > > Vance Dear Vance, Sorry, I found this modify has added at recently version which you supply. Moreover, I think in this bug when the phone is doing with sheetTransition, the appIn be killed and AppWindowManager._activeApp would be homescreen, but at this the appIn would still revivebrowser and publish('opened'), so it would be the problem of two active app. I would upload a patch, please help me to review it. Thanks.
Flags: needinfo?(ben.song) → needinfo?(vchen)
Dear Vance,Luke, In this patch, I add 'will-crashed' into classList of an app's element while this app is being killed by lowmemorykiller. So when swipein of sheetTransition, we would check this.element and this classList,if this.element is null or this.element.classList contains 'will-crashed', we would return directly.
Attachment #8538330 -
Flags: review?(vchen)
Attachment #8538330 -
Flags: review?(lchang)
Update the previous patch here,thanks.
Attachment #8538940 -
Flags: review?(vchen)
Attachment #8538940 -
Flags: review?(lchang)
Summary: [v2.1][Gaia_System_WindowMgmt] after monkey test phone stay at browser or other apps, couldn't back to homescreen → [FFOS7715 v2.1][Gaia_System_WindowMgmt] after monkey test phone stay at browser or other apps, couldn't back to homescreen
Updated•10 years ago
|
Summary: [FFOS7715 v2.1][Gaia_System_WindowMgmt] after monkey test phone stay at browser or other apps, couldn't back to homescreen → [v2.1][Gaia_System_WindowMgmt] after monkey test phone stay at browser or other apps, couldn't back to homescreen
Summary: [v2.1][Gaia_System_WindowMgmt] after monkey test phone stay at browser or other apps, couldn't back to homescreen → [FFOS7715 v2.1][Gaia_System_WindowMgmt] after monkey test phone stay at browser or other apps, couldn't back to homescreen
Comment 7•10 years ago
|
||
Comment on attachment 8538330 [details] [diff] [review] two_active_problem.patch Clear flags on this old patch since you have an updated one.
Attachment #8538330 -
Flags: review?(vchen)
Attachment #8538330 -
Flags: review?(lchang)
Comment 8•10 years ago
|
||
Comment on attachment 8538940 [details] [diff] [review] new_patch.patch Hi Alive, Could you please give us some feedback on this? Thanks.
Flags: needinfo?(lchang)
Attachment #8538940 -
Flags: review?(vchen)
Attachment #8538940 -
Flags: review?(lchang)
Attachment #8538940 -
Flags: review?(alive)
Comment 9•10 years ago
|
||
Comment on attachment 8538940 [details] [diff] [review] new_patch.patch Review of attachment 8538940 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I don't see why this works. Please elaborate more. https://bugzilla.mozilla.org/show_bug.cgi?id=1112987#c4 is wrong to me - the appIn will be revived if it was killed before sheet transition starts, but the active class of appOut will be removed once it gets swipeout.
Attachment #8538940 -
Flags: review?(alive)
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9) > Comment on attachment 8538940 [details] [diff] [review] > new_patch.patch > > Review of attachment 8538940 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry I don't see why this works. Please elaborate more. > https://bugzilla.mozilla.org/show_bug.cgi?id=1112987#c4 is wrong to me - > the appIn will be revived if it was killed before sheet transition starts, > but the active class of appOut will be removed once it gets swipeout. Dear Alive, While appIn and appOut do action of sheetsTransition, appIn be killed of lowmemorykiller. At this moment appIn's isActive is true so it should begin to switch to background. Moreover,because of _broadcastTimeout in stack_manager.js, last broadcast action of sheetsTransition would wait for the switch of appIn and homescreen until appIn publish terminate. This moment,homescreen be foreground app and appIn be background app, but now broadcast action start, and appIn would publish swipe in to publish opened. After this, In foreground there are two apps: homescreen and appIn. Thanks.
Flags: needinfo?(alive)
Reporter | ||
Comment 11•10 years ago
|
||
Dear Alive, So we would add a flag when app be crashed by lowmemorykiller, when app would publish swipein,we would check this flag to get this app is or not being killing. If it be true, we would not publish swipein, but to invoke clearTransitionClasses to clear will-become-active in classList of appIn. Thanks.
Comment 12•10 years ago
|
||
Comment on attachment 8538940 [details] [diff] [review] new_patch.patch Review of attachment 8538940 [details] [diff] [review]: ----------------------------------------------------------------- Redirect to etienne since he knows this better than me.
Attachment #8538940 -
Flags: review?(etienne)
Comment 13•10 years ago
|
||
(In reply to ben.song from comment #11) > Dear Alive, > > So we would add a flag when app be crashed by lowmemorykiller, when app > would publish swipein,we would check this flag to get this app is or not > being killing. > > If it be true, we would not publish swipein, but to invoke > clearTransitionClasses to clear will-become-active in classList of appIn. > > Thanks. I feel that swipein should absorb that for stackManager, but I will let etienne judge.
Flags: needinfo?(alive)
Reporter | ||
Comment 14•10 years ago
|
||
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #13) > (In reply to ben.song from comment #11) > > Dear Alive, > > > > So we would add a flag when app be crashed by lowmemorykiller, when app > > would publish swipein,we would check this flag to get this app is or not > > being killing. > > > > If it be true, we would not publish swipein, but to invoke > > clearTransitionClasses to clear will-become-active in classList of appIn. > > > > Thanks. > > I feel that swipein should absorb that for stackManager, but I will let > etienne judge. Dear Alive, OK.thanks.
Flags: needinfo?(vchen)
Comment 15•10 years ago
|
||
Comment on attachment 8538940 [details] [diff] [review] new_patch.patch Review of attachment 8538940 [details] [diff] [review]: ----------------------------------------------------------------- Hello Ben, I think the fix should only touch app_window.js and we should make the `isCrashed` check part of AppWindo#_handle__swipein. Other than that, 2 notes about the patch: * we don't need the comment referencing the bug * we'll need to add unit tests in app_window_test.js to cover this case Let me know if you need help with the unit tests, but I'm sure the tests in app_window_test.js will be a great source of inspiration :) Thanks!
Attachment #8538940 -
Flags: review?(etienne)
Reporter | ||
Comment 16•10 years ago
|
||
Dear Etienne, Could you help me with the unit tests ? I have update the patch as you said. Thanks.
Flags: needinfo?(etienne)
Attachment #8543563 -
Flags: review?(etienne)
Comment 17•10 years ago
|
||
Comment on attachment 8543563 [details] [diff] [review] update.patch Review of attachment 8543563 [details] [diff] [review]: ----------------------------------------------------------------- The code looks good! For the tests you'll want to add something similar to https://github.com/mozilla-b2g/gaia/blob/3e56f24a0eaf2e95f3f438748290a9b2c877db7f/apps/system/test/unit/app_window_test.js#L1932 where you dispatch a mozbrowsererror on the app first (so that isCrashed is true) and you assert that transitionController.clearTransitionClasses() was properly called. You'll also want to add an assertion to https://github.com/mozilla-b2g/gaia/blob/3e56f24a0eaf2e95f3f438748290a9b2c877db7f/apps/system/test/unit/app_window_test.js#L2022 checking that the isCrashed flag is properly reset. Cheers!
Attachment #8543563 -
Flags: review?(etienne)
Updated•10 years ago
|
Flags: needinfo?(etienne)
Reporter | ||
Comment 18•10 years ago
|
||
Dear Etienne, I have updated this patch with unit test, please help me to review it. In this update I assert app1.isCrashed be true after mozbrowsererror happens, and this value be false after reviveBrowser invoked. I add a test case to where isCrashed is true but not dispatch a mozbrowsererror on the app and assert that transitionController.clearTransitionClasses() was properly called.For I have asserted isCrashed be true while mozbrowsererror happens. Thanks.
Flags: needinfo?(etienne)
Attachment #8544335 -
Flags: review?(etienne)
Updated•10 years ago
|
Attachment #8543563 -
Attachment is obsolete: true
Flags: needinfo?(etienne)
Updated•10 years ago
|
Attachment #8538940 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8538330 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
Comment on attachment 8544335 [details] [diff] [review] swipein.patch Review of attachment 8544335 [details] [diff] [review]: ----------------------------------------------------------------- All good, thank you for this contribution! Can you open a pull request to gaia so we can land this patch?
Attachment #8544335 -
Flags: review?(etienne) → review+
Reporter | ||
Comment 20•10 years ago
|
||
(In reply to Etienne Segonzac (:etienne) from comment #19) > Comment on attachment 8544335 [details] [diff] [review] > swipein.patch > > Review of attachment 8544335 [details] [diff] [review]: > ----------------------------------------------------------------- > > All good, thank you for this contribution! > Can you open a pull request to gaia so we can land this patch? Dear Etienne, I have opened a pull request to gaia, please help me to land it. https://github.com/mozilla-b2g/gaia/pull/27178/files Thanks.
Flags: needinfo?(etienne)
Comment 21•10 years ago
|
||
(In reply to ben.song from comment #20) > (In reply to Etienne Segonzac (:etienne) from comment #19) > > Comment on attachment 8544335 [details] [diff] [review] > > swipein.patch > > > > Review of attachment 8544335 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > All good, thank you for this contribution! > > Can you open a pull request to gaia so we can land this patch? > > Dear Etienne, > > I have opened a pull request to gaia, please help me to land it. > > https://github.com/mozilla-b2g/gaia/pull/27178/files Hi, can you: - amend the commit message so that is says : "Bug #1112987 after monkey test phone stay at browser or other apps, couldn't back to homescreen r=etienne" on the first list - open the pull request against gaia-master (and not 2.1), we need to land on master first then uplift - on github add "+autoland" at the end the pull request title (we'll have a bot helping us for the landing)
Flags: needinfo?(etienne)
Reporter | ||
Comment 22•10 years ago
|
||
Dear Etienne, I have updated it, but not be merged, does it have any problem ? https://github.com/mozilla-b2g/gaia/pull/27220/files Thanks.
Flags: needinfo?(etienne)
Comment 23•10 years ago
|
||
Comment 24•10 years ago
|
||
Comment on attachment 8551911 [details] [review] [PullReq] songben423:bug1112987_master to mozilla-b2g:master Autlander didn't picked it up originally, fixed now!
Flags: needinfo?(etienne)
Attachment #8551911 -
Flags: review+
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Keywords: checkin-needed
Comment 25•10 years ago
|
||
Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/ff1b2b631842ed962999cc2a4b184db2bf05767b
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•