1112987 - [FFOS7715 v2.1][Gaia_System_WindowMgmt] after monkey test phone stay at browser or other apps, couldn't back to homescreen

Status: RESOLVED FIXED

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

Description

[ben.song]

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

Comment 1

[ben.song]

Attachment: 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.

Comment 2

[ben.song]

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

Comment 3

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Ni the correct luke here

Hi Ben Song, could you elaborate more on your modification?

Thanks

Vance

Comment 4

[ben.song]

(In reply to Vance Chen [:vchen][[e-mail]] 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.

Comment 5

[ben.song]

Attachment: two_active_problem.patch

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.

Comment 6

[ben.song]

Attachment: new_patch.patch

Update the previous patch here,thanks.

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8538330 [details] [diff] [review]
two_active_problem.patch

Clear flags on this old patch since you have an updated one.

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8538940 [details] [diff] [review]
new_patch.patch

Hi Alive,

Could you please give us some feedback on this? Thanks.

Comment 9

[(inactive after 6/18) Alive Kuo [: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.

Comment 10

[ben.song]

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

Comment 11

[ben.song]

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

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

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.

Comment 13

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

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

Comment 14

[ben.song]

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

Comment 15

[Etienne Segonzac (:etienne)]

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!

Comment 16

[ben.song]

Attachment: update.patch

Dear Etienne,

Could you help me with the unit tests ? I have update the patch as you said.

Thanks.

Comment 17

[Etienne Segonzac (: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!

Comment 18

[ben.song]

Attachment: swipein.patch

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.

Comment 19

[Etienne Segonzac (:etienne)]

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?

Comment 20

[ben.song]

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

Comment 21

[Etienne Segonzac (: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)

Comment 22

[ben.song]

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.

Comment 23

[GitHub Autolander Bot]

Attachment: [PullReq] songben423:bug1112987_master to mozilla-b2g:master

Comment 24

[Etienne Segonzac (:etienne)]

Comment on attachment 8551911 [details] [review]
[PullReq] songben423:bug1112987_master to mozilla-b2g:master

Autlander didn't picked it up originally, fixed now!

Comment 25

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/ff1b2b631842ed962999cc2a4b184db2bf05767b