1204110 - [Window Mgmt] The device will fall asleep while watching youtube videos

Status: VERIFIED FIXED

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

Description

[KTucker [:KTucker][Inactive 3/4/2016]]

The device will start falling asleep while watching Youtube videos if a user goes into fullscreen mode while watching a video and then locks and unlocks the phone. 

Repro Steps:
1) Update a Aries to 20150911153729
2) Open the browser and go to m.youtube.com
3) Tap on a video to play it. 
4) Tap on the video to show the video controls and tap on the "Fullscreen" icon.
5) When prompted, tap the "Allow" button to enter fullscreen mode.
6) Lock and unlock the device.
7) Continuing watching the video and observe.

Actual:
The device will fall asleep while watching a video. 

Expected:
The device does not fall asleep while watching Youtube videos.

Environmental Variables:
Device: Aries 2.5
Build ID: 20150911153729
Gaia: 758c75ee087ea3722213ea2c185cca1d952c8a29
Gecko: 7b1cfb1606ec447506bf7373b645b7a09f3aa238
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 43.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

Repro frequency: 5/5 100%
See attached: video, logcat

Comment 1

[KTucker [:KTucker][Inactive 3/4/2016]]

This issue also reproduces on the Flame 2.5

The device will fall asleep while watching Youtube videos. 

Environmental Variables:
Device: Flame 2.5 (Full Flash)(KK)(319mb)
Build ID: 20150911030227
Gaia: 6280500a6cb8d1b178cdd163450e36d22846fbed
Gecko: c0abc2a6e11f52761366e029eb1bae4c9864a8a3
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 43.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:43.0) Gecko/43.0 Firefox/43.0

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

This issues does not reproduce on the Flame 2.2

The device is not falling asleep during Youtube videos after performing the steps from Comment 0.

Environmental Variables:
Device: Flame 2.2 (Full Flash)(KK)(319mb)
BuildID: 20150911032501
Gaia: 7a427e0f8aa6c185a9e22358006b97c19435ca4a
Gecko: 0d9c46d01861
Gonk: bd9cb3af2a0354577a6903917bc826489050b40d
Version: 37.0 (2.2) 
Firmware Version: v18D

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Attachment: YoutubeFallingAsleepLog.txt

Comment 3

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

[Blocking Requested - why for this release]:
This is a regression of Youtube functionality.  Let's get a window here.

Comment 4

[Martin Shuman [:Marty]]

This issue appears to be caused by:
Bug 1173284 

B2g-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150729025342
Gaia: 4e1638e910f1657a9e940c99cb3421dbb71411e6
Gecko: b476e6bd9f1c
Version: 42.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

First Broken 
Environmental Variables:
Device: Flame 2.5
BuildID: 20150729031841
Gaia: b066d68030dac08cb545b37195bcab67593f365b
Gecko: e7454992d521
Version: 42.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Last Working gaia / First Broken gecko - This issue does NOT reproduce
Gaia: 4e1638e910f1657a9e940c99cb3421dbb71411e6
Gecko: e7454992d521

First Broken gaia / Last Working gecko - This issue DOES reproduce
Gaia: b066d68030dac08cb545b37195bcab67593f365b
Gecko: b476e6bd9f1c

Gaia Pushlog:
https://github.com/mozilla-b2g/gaia/compare/4e1638e910f1657a9e940c99cb3421dbb71411e6...b066d68030dac08cb545b37195bcab67593f365b

Comment 5

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Greg this issue seems to have been caused by the fix for bug 1173284.  Can you please take a look?

Comment 6

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

From the symptom I don't think the patch caused that, while the fall asleep is controlled by ScreenManager I think, and the code in Bug 1173284 didn't touch it. By the way, is it a special case for YouTube video? Or when any video is playing the device shouldn't fall asleep?

Comment 7

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

And I will try to dig it while the NI is pending, at least to make sure if Bug 1173284 really caused that.

Comment 8

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Setting qawanted to check comment 6.

Comment 9

[KTucker [:KTucker][Inactive 3/4/2016]]

This seems to occur on any Youtube video after performing the steps in Comment 0.

Comment 10

[Martin Shuman [:Marty]]

I was able to reproduce this issue when streaming an .mp4 file directly through the browser, but not when viewing a video in the Gallery or Video app. This seems to be specific to the browser.

Streaming an .mp4 file or a YouTube video in the browser ordinarily does not allow the device to go to sleep, but following the STR of this bug will improperly allow the device to fall asleep during video playback.

Comment 11

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Greg please see comment 10.

Comment 12

[Mahendra Potharaju [:mahe]]

Blocks 2.5 with a P2 priority

Comment 13

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

I'm dealing with this. First, bisect to find out the root cause.

Comment 14

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

Now I finally have some clue. Need to pin it down though.

Comment 15

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

Hmm I'm sure this is NOT a regression, but a racing defect that already exists since long time ago. The problem is in ScreenManager, it will reconfig the timeout if ScreenWakeLockManager is held and WakeLockManagerBase triggers the |locked-foreground| change to call the function:

    _reconfigScreenTimeout: function scm_reconfigScreenTimeout() {
      // Remove idle timer if screen wake lock is acquired or
      // if no app has been displayed yet.
      if (this._wakeLockManager.isHeld ||
          (!Service.query('AppWindowManager.getActiveWindow') &&
           !Service.query('locked'))) {
        this._setIdleTimeout(0);

And this step will cancel the timeout when there is a video is playing. However, another timeout  configuring request will come from event |unlocking-stop|, which will call the same function ('_reconfigScreenTimeout'). So now we have:

1. When the WakeLockManager trigger the change *after* the |unlocking-stop| event, the timeout will be set *correctly*, since it's the timeout for the video.

2. When the WakeLockManager trigger the change *before* the |unlocking-stop| event, the timeout will be set *INCORRECTLY*, since it's the timeout for other apps, not for the video

I can try to re-order events to workaround the bug, and solve the "regression". Nevertheless, this serious defect exists even before the patch exposed it, so I think a much rigorous patch should be considered.

Comment 16

[GitHub Autolander Bot]

Attachment: [gaia] snowmantw:bug1204110 > mozilla-b2g:master

Comment 17

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

Comment on attachment 8668791 [details] [review]
[gaia] snowmantw:bug1204110 > mozilla-b2g:master

Hi Tim, the patch works well on my device, but I want to know if it's a good solution to you. Thanks.

Comment 18

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

Comment on attachment 8668791 [details] [review]
[gaia] snowmantw:bug1204110 > mozilla-b2g:master

I don't agree this is a "race". This is simply a state handling inconsistency (yeah, maybe that can be called a race by your definition but let's stop arguing about jargon here).

Any reason why we cannot simply call |_reconfigScreenTimeout()| at the event handler other than "you don't like the centralized design"?

I agree |_reconfigScreenTimeout()| itself is not pretty but a centralized route will certainly help us to rewrite that into a proper state machine in the future.

Comment 19

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

While the perfect solution to me is to make different reconfigScreenTimeouts with clearly different intensions (and let the condition checks outside a function that in fact is three more functions, which will eventually become the state machine you mentioned), to just call it as you suggested is to delegate it to the puzzle again. And I believe you know the state machine you mentioned may not happen "soon", unless you know there are already some people working on that while I don't know. So, whether I like it not is not important, but to describe why set or not  set the timeout according to different input (event) and status (wake lock) clearly in the currently foreseeable future is more critical. At least, without any further explanation, I can't see why to put all the thing inside that function as the old way shows will be more clear than to put in another place more close to the input and check the status in-place.

Comment 20

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

Comment on attachment 8668791 [details] [review]
[gaia] snowmantw:bug1204110 > mozilla-b2g:master

Greg made his case on calling |this._setIdleTimeout()| directly. Let's not stuck on the small detail like this -- it can be easily corrected if we find out otherwise.

Comment 21

[Etienne Segonzac (:etienne)]

Anything preventing us from landing this fix? :)

Comment 22

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

Yes: I just finished my PTO. And you just commented it before I press the merge button now. :D

Comment 23

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

The try-server says OK:

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=c45e66dca6c98807072e1e78dacaae68fb20fbd3

Comment 24

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

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

Comment 25

[zouyi(Leave from Mozilla)]

Attachment: Aries_KK v2.5.3gp

This bug has been verified as "pass" on the latest build of  Flame KK v2.5 and Aries KK v2.5  by the STR in comment 0.

Actual results: While watching a MP4 at the website (m.youtube.com) through browser, if user locks and unlocks device, then continues watching the video, device will not fall asleep.

See attachments: Aries_KK v2.5.3gp.

Reproduce rate: 0/10.

Device: Aries KK 2.5(pass)
Build ID               20151026223015
Gaia Revision          b564b21e08ffc3e4962f08850843c7482932ee7b
Gaia Date              2015-10-26 14:22:21
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/6c7c983bce46a460c2766fbdd73283f6d2b03a69
Gecko Version          44.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151026.214940
Firmware Date          Mon Oct 26 21:49:48 UTC 2015
Bootloader             s1

Device: Flame KK 2.5(pass)
Build ID               20151026030217
Gaia Revision          a677ddd3aa3a81058775938bd56008d96dbc78b0
Gaia Date              2015-10-26 04:48:31
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/5ca03a00d26823ce91ee0eaa2937bed605bd53c1
Gecko Version          44.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151026.070911
Firmware Date          Mon Oct 26 07:09:23 EDT 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0