Closed Bug 1112987 Opened 7 years ago Closed 7 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)

x86_64
Linux
defect
Not set
normal

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
Attached file error_log
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)
Attached patch two_active_problem.patch (obsolete) — Splinter Review
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)
Attached patch new_patch.patch (obsolete) — Splinter Review
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
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 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 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 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)
(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)
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 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)
(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)
(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 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)
Attached patch update.patch (obsolete) — Splinter Review
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 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)
Flags: needinfo?(etienne)
Attached patch swipein.patchSplinter Review
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)
Attachment #8543563 - Attachment is obsolete: true
Flags: needinfo?(etienne)
Attachment #8538940 - Attachment is obsolete: true
Attachment #8538330 - Attachment is obsolete: true
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+
(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)
(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)
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)
Blocks: 1123554
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+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.