1036236 - [LockScreen] Unlock to Camera twice would make it unavailable

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

Description

[Greg Weng [:snowmantw][:gweng][:λ]]

STR:

1. Unlock to Camera
2. Camera launch as usual
3. Lock again
4. Unlock to Camera again
5. Camera window would be broken

I would fix this ASAP.

Comment 1

[Greg Weng [:snowmantw][:gweng][:λ]]

After the offline discussion with Alive, here is the solution:

1. visibility manager would not block 'showwindow' to be fired when it accepted the 'lockscreen-request-unlock' event

2. what it would do is to pass the activity detail to the 'showwindow' event, and AppWindowManager would fire it as activity

3. this means LockScreenWindowManager would fire no activity, and so that we can avoid this race condition

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

The code was introduced in Bug 1030415. So I set the dependency.

Comment 3

[Jason Smith [:jsmith]]

Greg - Can you be more specific in terms of what's broken here?

Comment 4

[Greg Weng [:snowmantw][:gweng][:λ]]

You can see the video:

https://www.youtube.com/watch?v=g7BySvo7Szs

And the code is actually the root cause of the bug, which was also discussed with Alive, the committee.

Comment 5

[Johan Lorenzo [:jlorenzo]]

I reproduced it on my 2.0 Flame. Nominating as a 2.0 blocker because of the regression.

Comment 7

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch v2.0

Comment 8

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch

Set review first to see if I implement Alive's opinion correctly.

Comment 9

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

Comment on attachment 8455111 [details] [review]
Patch

My final proposal is let AppWindowManager to wait the homescreen opened to launch the record activity requested by LWM. With this we won't have race condition between homescreen's opened event and launchactivity's procedure.

Could you try it?

For transition state change I wonder it's dangerous and we will have race condition if the opened app's dimension(orientation) is not the same as the current dimension(orientation).

Comment 10

[Greg Weng [:snowmantw][:gweng][:λ]]

Oh, sorry, I implement it with the wrong (old) method. I'll update it ASAP, thanks.

Comment 11

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8455111 [details] [review]
Patch

Use new method to implement. I'll update 2.0 patch after reviewing with the same method.

Comment 12

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Patch v2.0

Comment 13

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

Comment on attachment 8455111 [details] [review]
Patch

Don't we need to wait homescreen opened to launch activity avoid to regress the bug that camera from lockscreen stuck the home button?
https://bugzilla.mozilla.org/show_bug.cgi?id=1030415

Comment 14

[Greg Weng [:snowmantw][:gweng][:λ]]

Should I use getActiveApp and the nextPaint callback to guarantee that? Or there is other ways that I can do that?

Comment 15

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

I am not sure I have the solution.
The problem is
1. Unlock will trigger homescreen open
2. Unlock will trigger web activity if unlock reaches camera area
The two opened event races so AppWindowManager has the wrong info (homescreen) but active is camera.

So maybe find out the code patch of 1. and 2. and see if we could avoid open homescreen if we needs to open camera.

Comment 16

[Greg Weng [:snowmantw][:gweng][:λ]]

The code path 1 & 2 now are all in the app_window_manager.js as my patch shows, and they're even in the same event handling section. So I think I can do what you said to avoid open homescreen if the 'showwindow' event has the activity content, thus we may avoid the race condition. The pseudo code would be:
    if (the event now has activity content) {
        fire the activity
    } else {
      if (there is an activate app) {
          set the activate app visible
      } else {
          get homescreen and launch or set visible for it
      }
    }

The second section of 'else' is what the current code does. What I'm not sure about is what would happen if we fire the activity twice? And should I set the activate app visible first before I fire the activity?

Comment 17

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

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #16)
> The code path 1 & 2 now are all in the app_window_manager.js as my patch
> shows, and they're even in the same event handling section. So I think I can
> do what you said to avoid open homescreen if the 'showwindow' event has the
> activity content, thus we may avoid the race condition. The pseudo code
> would be:
>     if (the event now has activity content) {
>         fire the activity
>     } else {
>       if (there is an activate app) {
>           set the activate app visible
>       } else {
>           get homescreen and launch or set visible for it
>       }
>     }
> 
> The second section of 'else' is what the current code does. What I'm not
> sure about is what would happen if we fire the activity twice? And should I
> set the activate app visible first before I fire the activity?

1. If same web activity is launched twice - there will be only one instance because we are checking manifestURL in AppWindowFactory.
2. I think so. If the web activity launch failed then no one will set visible again.

Thanks.

Comment 18

[Greg Weng [:snowmantw][:gweng][:λ]]

Comment on attachment 8455111 [details] [review]
Patch

Update the patch. The handler now has 3 paths:

1. If there is an activate app, fire the activity after set the app as visible
2. If there is no activate app and we need to fire the activity, fire it and do not open homescreen
3. If there is no activate app and we don't need to fire the activity, open the homescreen

Comment 19

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

Comment on attachment 8455111 [details] [review]
Patch

Thanks!

Comment 20

[Greg Weng [:snowmantw][:gweng][:λ]]

Will update the 2.0 patch and land both of them at once.

Comment 21

[Greg Weng [:snowmantw][:gweng][:λ]]

master: https://github.com/mozilla-b2g/gaia/commit/e377ad326832eb945c7f4e9a4e63fd0bff14dc7d

Comment 22

[Greg Weng [:snowmantw][:gweng][:λ]]

2.0 patch only failed at some irrelevant cases:

https://travis-ci.org/mozilla-b2g/gaia/builds/30232030

So I'll land this patch to 2.0.

Comment 23

[Greg Weng [:snowmantw][:gweng][:λ]]

2.0: https://github.com/mozilla-b2g/gaia/commit/34c24749e78496ac1317f4ddb573281c0ef2995e

Comment 24

[Carsten Book [:Tomcat]]

sorry had to reopen this bug and revert the change for failing gaia unit tests like https://tbpl.mozilla.org/php/getParsedLog.php?id=44075988&tree=B2g-Inbound

21:44:30     INFO -  gaia-unit-tests TEST-UNEXPECTED-FAIL | system/test/unit/lockscreen_window_manager_test.js | system/LockScreenWindowManager Handle events LockScreen request to unlock with activity detail | it didn't construct the activity while the request denote to do it: expected false to be true
21:44:30     INFO -  gaia-unit-tests INFO       | stack trace:
21:44:30     INFO -      Error: it didn't construct the activity while the request denote to do it: expected false to be true
21:44:30     INFO -          at chaiAssert (app://system.gaiamobile.org/common/test/helper.js:31:1)
21:44:30     INFO -          at get (app://system.gaiamobile.org/common/vendor/chai/chai.js:370:1)
21:44:30     INFO -          at isTrue (app://system.gaiamobile.org/common/vendor/chai/chai.js:1367:5)
21:44:30     INFO -          at (anonymous) (app://system.gaiamobile.org/test/unit/lockscreen_window_manager_test.js:226:1)
21:44:30     INFO -          at wrapper (app://system.gaiamobile.org/common/test/mocha_generators.js:62:19)
21:44:30     INFO -          at run (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:3709:7)
21:44:30     INFO -          at runTest (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:4081:5)
21:44:30     INFO -          at (anonymous) (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:4127:7)
21:44:30     INFO -          at next (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:4007:1)
21:44:30     INFO -          at (anonymous) (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:4016:7)
21:44:30     INFO -          at next (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:3964:1)
21:44:30     INFO -          at (anonymous) (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:3984:5)
21:44:30     INFO -          at (anonymous) (app://system.gaiamobile.org/common/vendor/mocha/mocha.js:4932:28)
21:44:30     INFO -  gaia-unit-tests TEST-PASS  | system/test/unit/lockscreen_window_manager_test.js | system/LockScreenWindowManager Handle events Open the app when asked via lock-immediately setting
21:44:30     INFO -  gaia-unit-tests TEST-END   | system/test/unit/lockscreen_window_manager_test.js | Handle events
21:44:30     INFO -  gaia-unit-tests TEST-END   | system/test/unit/lockscreen_window_manager_test.js | system/LockScreenWindowManager
21:44:30     INFO -  gaia-unit-tests TEST-END   | system/test/unit/lockscreen_window_manager_test.js |
21:44:30     INFO -  gaia-unit-tests INFO       | suite results (pass/fail): 11/1
21:44:31     INFO -  JavaScript warning: app://system.gaiamobile.org/common/vendor/chai/chai.js?time=1405658671161, line 1135: mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create
21:

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master revert: 70754ca0d317c1d09b89b88b6e93815e7451e95c

Comment 26

[Greg Weng [:snowmantw][:gweng][:λ]]

Just remove the invalid test and merge again. Late since the tree got closed.

master: https://github.com/mozilla-b2g/gaia/commit/2ca15b5dbd70f8d5740492eaa9114d50c6e93cf0

Comment 27

[Shine(Leave from Mozilla)]

Attachment: video

This issue has been verified successfully on Flame 2.0 and 2.1
See attachment: Verify_1036236.MP4
Reproducing rate: 0/5

Flame 2.0 build:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/29222e215db8
Build-ID        20141203000201
Version         32.0

Flame 2.1 build:
Gaia-Rev        dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebce587d2194
Build-ID        20141203001205
Version         34.0