Joshua Mitchell (Inactive) Reporter
	Description • 9 years ago

Description:
With Edge swiping on you can transition between apps by pressing and sliding near the edge of the screen. If Settings is one of the apps open sometimes transiting to an app will cause a black-screen. The black screen can occur in other apps beside the Settings app. but will not occur without the settings app open. This will occur with a min of 2 apps open (as needed to transition) but occurs more frequently with 3 or 4 apps open. The black screen has been seen when transitioning to the settings app, clock app, video app, FM radio and dialer

When a black screen is encountered the status bar is still present, and everything still functions (home button, lockscreen timeout, notification bar / menu), except you can not screen transition out of the blackscreened app. To recover you must long press the home button to go to card view and the affected app will 'reset'. After the app has gone blackscreen and NOT closed but 'reset' it is possible to screen transition into an empty frame with only the background visible (no icons, no status bar, no home button functionality) but you can still pull down the notification menu. 

When transitioning to the Settings app the screen will usually flash black before loading fully. This is more apparent / pronounced in 2.2 however the persistent black-screen does not repro there. This black flash before fully loading may be related. 


Repro Steps:
1) Update a Flame to 20150121010204
2) Open several apps including Settings
3) Edge transition between the apps

Actual:
Transiting to an app causes a black-screen

Expected:
apps will open / transition normally

Environmental Variables:
Device: Flame 3.0
Build ID: 20150121010204
Gaia: 5e98dc164b17fd6decb48a9eaddef0e55b82e249
Gecko: 540077a30866
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Repro frequency: The repro is 100% but a question of how many transitions one must make until encountering. I've seen it after only 2 or 3 transitions, to as many as 20-25 transition.
Link to failed test case:
See attached: logcat, video: http://youtu.be/Ur_AfQTvuHo

----------------------------------------------------------------------

This issue does NOT repro on Flame 2.2 (v18d), 2.2 (v18d-1), or 2.1 (v18d-1)

Device: Flame 2.2 (KK - Nightly - Full Flash)
Build ID: 20150121002607
Gaia: e4f9b5da3751798f9cc5d95f302c30722cc11fca
Gecko: 75a462a58d7a
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 2.2 (KK - Nightly - Full Flash)
Build ID: 20150120002507
Gaia: f5b3d1b6cfa3e702033f613915ae637cb735cbfb
Gecko: 5d7497ce4cc7
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Device: Flame 2.1 (KK - Nightly - Full Flash)
Build ID: 20150120001202
Gaia: 77c57eb8a985d5cbd34a597fb1b978ba6e205af6
Gecko: f05d0a2d2378
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

	Peter Bylenga [:PBylenga]
	Comment 1 • 9 years ago

[Blocking Requested - why for this release]:
Functional regression, poor UX.

Requesting a window.

	Joshua Mitchell (Inactive) Reporter
	Comment 2 • 9 years ago

This might not be specific to Settings app - I just hit this bug with only email and contacts open and transitioning back and forth. (but it seems a higher repro with settings open)

Device: Flame 3.0
Build ID: 20150122010203
Gaia: 917b6c36717fddc6e71ffc1ec249633c8044c93c
Gecko: 34e2d2bd7ec4
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

	Jayme Mercado [:JMercado] (Inactive 3/4/2016)
	Comment 3 • 9 years ago

Bug 1112987 seems to be the cause of this issue.

B2g-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 3.0
BuildID: 20150120093158
Gaia: 4be474007ef5b3f2ed530286559d6245d3a66e89
Gecko: e5e9a30a81c8
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

First Broken 
Environmental Variables:
Device: Flame 3.0
BuildID: 20150120105259
Gaia: 0a13aad9e437d12334ba5d54619e668f938895e7
Gecko: 011d7cd7e928
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Last Working gaia / First Broken gecko - Issue does NOT occur
Gaia: 4be474007ef5b3f2ed530286559d6245d3a66e89
Gecko: 011d7cd7e928

First Broken gaia / Last Working gecko - Issue DOES occur
Gaia: 0a13aad9e437d12334ba5d54619e668f938895e7
Gecko: e5e9a30a81c8

Gaia Pushlog: https://github.com/mozilla-b2g/gaia/compare/4be474007ef5b3f2ed530286559d6245d3a66e89...0a13aad9e437d12334ba5d54619e668f938895e7

	KTucker [:KTucker][Inactive 3/4/2016]
	Comment 4 • 9 years ago

Ben, can you take a look at this please? Looks like the landing for bug 1112987 may be the culprit here.

	ben.song Assignee
	Comment 5 • 9 years ago

(In reply to KTucker [:KTucker] from comment #4)
> Ben, can you take a look at this please? Looks like the landing for bug
> 1112987 may be the culprit here.

Dear KTucker,

Base on my analysis, the code modified in bug 1112987 is only for the app been crashed. But sometimes AppWindow not been crashed, but encounter a black screen, so it should not into the code as below.I would follow up the problem.

if (this.isCrashed) {
  if (this.transitionController) {
    this.transitionController.clearTransitionClasses();
  }
  return;
}

	bhavana bajaj [:bajaj]
	Comment 6 • 9 years ago

eitienne, ben can you help with this aspa ? we cannot have a regression slip-in.

	ben.song Assignee
	Comment 7 • 9 years ago

(In reply to bhavana bajaj [:bajaj] from comment #6)
> eitienne, ben can you help with this aspa ? we cannot have a regression
> slip-in.

Dear Bhavana,

Base on my analysis of logcat, probably there is one problem at apps/system/js/app_window.js.

Since We couldn't reproduce the problem, please apply my modify and verify this problem.

AppWindow.prototype._handle_mozbrowsererror =
    function aw__handle_mozbrowsererror(evt) {
      if (evt.detail.type !== 'fatal') {
        return;
      }
      // Send event instead of call crash reporter directly.
      this.publish('crashed');

      if (this.constructor.SUSPENDING_ENABLED && !this.isActive()) {
        this.debug(' ..sleep! I will come back.');
        this.destroyBrowser();
        if (this.frontWindow) {
          this.frontWindow.kill();
        }
      } else {
        this.isCrashed = true;
        this.kill(evt);
      }
    };

	ben.song Assignee
	Comment 8 • 9 years ago

Dear Bhavana,

For we close the setting of SUSPENDING_ENABLED, so haven't find this problem. The origin of this problem is that while edge swipe transitioning, the app which would open has been crashed, so we stop it continuing to open. But we forget a problem, if SUSPENDING_ENABLED was true app crashed would not be killed, so we still could edge swipe transition to it, but only it is not been opened, and appears a black screen. We would add a check of SUSPENDING_ENABLED before set isCrashed to true to fix this problem.

	Etienne Segonzac (:etienne)
	Comment 9 • 9 years ago

Comment on attachment 8562554 [details] [diff] [review]
edge_swipe_transition_black_screen.patch

Review of attachment 8562554 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.
Can you add a small unit test and open a pull request for this?

	ben.song Assignee
	Comment 10 • 9 years ago

Dear Etienne,

I have added a small unit test, and opened a pull request, please help me to merge it.

https://github.com/mozilla-b2g/gaia/pull/28162

Thanks.

	Etienne Segonzac (:etienne)
	Comment 11 • 9 years ago

(In reply to ben.song from comment #10)
> Dear Etienne,
> 
> I have added a small unit test, and opened a pull request, please help me to
> merge it.
> 
> https://github.com/mozilla-b2g/gaia/pull/28162
> 
> Thanks.

Thanks!
We still need a test for when AppWindow.SUSPENDING_ENABLED is true :)
we should have one where suspending is true asserting that isCrashed is set to true too.
And another one where suspending if false asserting that we leave isCrashed untouched.

	ben.song Assignee
	Comment 12 • 9 years ago

(In reply to Etienne Segonzac (:etienne) from comment #11)
> (In reply to ben.song from comment #10)
> > Dear Etienne,
> > 
> > I have added a small unit test, and opened a pull request, please help me to
> > merge it.
> > 
> > https://github.com/mozilla-b2g/gaia/pull/28162
> > 
> > Thanks.
> 
> Thanks!
> We still need a test for when AppWindow.SUSPENDING_ENABLED is true :)
> we should have one where suspending is true asserting that isCrashed is set
> to true too.
> And another one where suspending if false asserting that we leave isCrashed
> untouched.

Dear Etienne,

I have added what you said like it as below:

test('Destroy only the browser when app crashed and ' +
          'suspending is enabled',
      function() {
        var apps = openPopups(2);
        var app1 = apps[0];
        var popup1 = apps[1];
        var stubKill = this.sinon.stub(popup1, 'kill');
        var stubDestroyBrowser = this.sinon.stub(app1, 'destroyBrowser');
        var stubIsActive = this.sinon.stub(app1, 'isActive');

        stubIsActive.returns(false);
        AppWindow.SUSPENDING_ENABLED = true;
        app1.handleEvent({
          type: 'mozbrowsererror',
          detail: {
            type: 'fatal'
          }
        });
        assert.isFalse(app1.isCrashed);
        assert.isTrue(stubDestroyBrowser.called);
        assert.isTrue(stubKill.called);
        AppWindow.SUSPENDING_ENABLED = false;
      });

    test('Kill the app directly even suspending is enabled ' +
          'when the app is active',
      function() {
        var app1 = new AppWindow(fakeAppConfig1);
        var stubKill = this.sinon.stub(app1, 'kill');
        var stubIsActive = this.sinon.stub(app1, 'isActive');
        stubIsActive.returns(true);
        AppWindow.SUSPENDING_ENABLED = true;
        app1.handleEvent({
          type: 'mozbrowsererror',
          detail: {
            type: 'fatal'
          }
        });

        assert.isTrue(stubKill.called);
        assert.isTrue(app1.isCrashed);
        AppWindow.SUSPENDING_ENABLED = false;
      });

Now In this patch, For app would not be killed but destroybrowser while suspending is enable and app.isActive is true. So we would assert app.isCrashed be false in this case. 

In another case, app would still be killed while suspending is enable and app.isActive is false. So we would assert app.isCrashed be true in this case.

Thanks.

	Etienne Segonzac (:etienne)
	Comment 13 • 9 years ago

Looking good, can you update the pull request and attach it to this bug for review?

	ben.song Assignee
	Comment 14 • 9 years ago

(In reply to Etienne Segonzac (:etienne) from comment #13)
> Looking good, can you update the pull request and attach it to this bug for
> review?

Dear Etienne,

I have updated test cases in last pull request based on patch merged in bug 1112987.

Thanks.

	Etienne Segonzac (:etienne)
	Comment 15 • 9 years ago

(In reply to ben.song from comment #14)
> (In reply to Etienne Segonzac (:etienne) from comment #13)
> > Looking good, can you update the pull request and attach it to this bug for
> > review?
> 
> Dear Etienne,
> 
> I have updated test cases in last pull request based on patch merged in bug
> 1112987.
> 
> Thanks.

I still see one test instead of 2 in the pull request : https://github.com/mozilla-b2g/gaia/pull/28162
Can you update it with the 2 tests and ask me for review? thanks!

	ben.song Assignee
	Comment 16 • 9 years ago

(In reply to Etienne Segonzac (:etienne) from comment #15)
> (In reply to ben.song from comment #14)
> > (In reply to Etienne Segonzac (:etienne) from comment #13)
> > > Looking good, can you update the pull request and attach it to this bug for
> > > review?
> > 
> > Dear Etienne,
> > 
> > I have updated test cases in last pull request based on patch merged in bug
> > 1112987.
> > 
> > Thanks.
> 
> I still see one test instead of 2 in the pull request :
> https://github.com/mozilla-b2g/gaia/pull/28162
> Can you update it with the 2 tests and ask me for review? thanks!

Dear Etienne,

I have updated test cases with new pull request as below:

https://github.com/mozilla-b2g/gaia/pull/28233/files

Please help me to review it, thanks.

	ben.song Assignee
	Comment 18 • 9 years ago

Dear Etienne,

Please ignore comment16, the pull request has some problem during test.

I have updated it as below:

https://github.com/mozilla-b2g/gaia/pull/28236/files

Thanks.

	Etienne Segonzac (:etienne)
	Comment 19 • 9 years ago

Comment on attachment 8564845 [details] [review]
[gaia] songben423:bug1124398__master > mozilla-b2g:master

Note: please use the review menu in the "detail" page to flag me for review, I'll get to it much more quickly :)

	bhavana bajaj [:bajaj]
	Comment 20 • 9 years ago

Etienne, can you please help check this in?

	ben.song Assignee
	Comment 21 • 9 years ago

(In reply to Etienne Segonzac (:etienne) from comment #19)
> Comment on attachment 8564845 [details] [review]
> [gaia] songben423:bug1124398__master > mozilla-b2g:master
> 
> Note: please use the review menu in the "detail" page to flag me for review,
> I'll get to it much more quickly :)

Dear Etienne,

OK, I would do it next time, thanks.

	Etienne Segonzac (:etienne)
	Comment 23 • 9 years ago

Comment on attachment 8569063 [details] [review]
[gaia] etiennesegonzac:bug-1124398 > mozilla-b2g:master

rebased PR, waiting for a green try

	GitHub Autolander Bot
	Comment 24 • 9 years ago

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

	Etienne Segonzac (:etienne)
	Comment 25 • 9 years ago

Comment on attachment 8569063 [details] [review]
[gaia] etiennesegonzac:bug-1124398 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1112987
[User impact] if declined: bad user experience when edge-swiping to an OOM killed app
[Testing completed]: edge swiping scenarios with 8+ apps on a 319mb flame
[Risk to taking this patch] (and alternatives if risky):low
[String changes made]: none

	Peter Bylenga [:PBylenga]
	Comment 26 • 9 years ago

Adding verifyme to check on next nightly.

	Jayme Mercado [:JMercado] (Inactive 3/4/2016)
	Comment 27 • 9 years ago

This issue is verified fixed on the latest Nightly Flame KK build.

Actual Results: Edge swiping between apps does not show a black screen.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150226010233
Gaia: 7894b929f1b0394f3c997f72a6482bc7813e758d
Gecko: dd6353d61993
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

	bhavana bajaj [:bajaj]
	Comment 28 • 9 years ago

(In reply to Etienne Segonzac (:etienne) from comment #25)
> Comment on attachment 8569063 [details] [review]
> [gaia] etiennesegonzac:bug-1124398 > mozilla-b2g:master
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): bug 1112987
> [User impact] if declined: bad user experience when edge-swiping to an OOM
> killed app
> [Testing completed]: edge swiping scenarios with 8+ apps on a 319mb flame
> [Risk to taking this patch] (and alternatives if risky):low
> [String changes made]: none

The regressing bug seems to have landed on master (current 3.0) alone, and 2.2 seems unaffected here, then not sure why we need the uplift. Etienne, agree or am I missing something  ?

	Etienne Segonzac (:etienne)
	Comment 29 • 9 years ago

Comment on attachment 8569063 [details] [review]
[gaia] etiennesegonzac:bug-1124398 > mozilla-b2g:master

(In reply to bhavana bajaj [:bajaj] from comment #28)
> (In reply to Etienne Segonzac (:etienne) from comment #25)
> > Comment on attachment 8569063 [details] [review]
> > [gaia] etiennesegonzac:bug-1124398 > mozilla-b2g:master
> > 
> > [Approval Request Comment]
> > [Bug caused by] (feature/regressing bug #): bug 1112987
> > [User impact] if declined: bad user experience when edge-swiping to an OOM
> > killed app
> > [Testing completed]: edge swiping scenarios with 8+ apps on a 319mb flame
> > [Risk to taking this patch] (and alternatives if risky):low
> > [String changes made]: none
> 
> The regressing bug seems to have landed on master (current 3.0) alone, and
> 2.2 seems unaffected here, then not sure why we need the uplift. Etienne,
> agree or am I missing something  ?

Oh no you're right! Sorry for the noise.