1075290 - [dolphin][flame] Test proximity sensor in a call, screen off until call ends

Status: RESOLVED FIXED

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

Description

[Shiwei Zhang]

STR:

1. Make a phone call.

2. Very quickly changing the distance between hand and the phone for three times. 

3. Can see screen turns on and off.

RESULTS: 

Screen won't turn on screen off until call ends.

Comment 1

[Shiwei Zhang]

[Blocking Requested - why for this release]:

We meet this issue both on Dophin and Flame.

gaia : remote="mozillaorg" revision="efa2b8cb095407df942fee7732a5547c7034ef9b" upstream="v1.4"

gecko : remote="mozillaorg" revision="a040c4e10e140fc234e6ee0bcfd53c0967dc7ba3" upstream="v1.4"

Comment 2

[Shiwei Zhang]

We also found this issue on v2.0

Comment 3

[Shiwei Zhang]

I think this timer is the main cause, 

gaia/apps/system/js/screen_manager.js
var screenOff = function scm_screenOff() {
...
setTimeout(function realScreenOff() {
        self.setScreenBrightness(0, true);
        // Sometimes the ScreenManager.screenEnabled and mozPower.screenEnabled
        // values are out of sync. Since the rest of the world relies only on
        // the value of ScreenManager.screenEnabled it can be some situations
        // where the screen is off but ScreenManager think it is on... (see
        // bug 822463). Ideally a callback should have been used, like
        // ScreenManager.getScreenState(function(value) { ...} ); but there
        // are too many places to change that for now.
        self.screenEnabled = false;
        navigator.mozPower.screenEnabled = false;
      }, 20);
...

issue can not reproduced if this timer was not used.

Comment 4

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

Hi Shiwei -

Could you help to provide a video of how you reproduce this one on Flame?

Thanks for your help

Comment 5

[Shiwei Zhang]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #4)
> Hi Shiwei -
> 
> Could you help to provide a video of how you reproduce this one on Flame?
> 
> Thanks for your help

Hi Vance, can you open this link

http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=357168

the video is too large and attached in our bugzilla.

thanks.

Comment 6

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

(In reply to Shiwei Zhang from comment #5)
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #4)
> > Hi Shiwei -
> > 
> > Could you help to provide a video of how you reproduce this one on Flame?
> > 
> > Thanks for your help
> 
> Hi Vance, can you open this link
> 
> http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=357168
> 
> the video is too large and attached in our bugzilla.
> 
> thanks.

Hi Shiwei -

Thanks, I just check the video, however I find it is extremely hard to reproduce this one on Flame. From the video I am thinking maybe you can try to adjust the screen off timer in order to accommodate the sensitivity of p-sensor?

Thanks

Comment 7

[Shiwei Zhang]

Yes, I have tried, but nothing has changed unless removing the timer, this issue is not inevitable，but occured with a high probability.

Comment 8

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

(In reply to Shiwei Zhang from comment #7)
> Yes, I have tried, but nothing has changed unless removing the timer, this
> issue is not inevitable，but occured with a high probability.

It will still happen even if you prolong the timer?

Comment 9

[Shiwei Zhang]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #8)
> (In reply to Shiwei Zhang from comment #7)
> > Yes, I have tried, but nothing has changed unless removing the timer, this
> > issue is not inevitable，but occured with a high probability.
> 
> It will still happen even if you prolong the timer?

Yes, the longer we set the easier issue to be reproduced, I think modifying mozPower.screenEnabled in a timer is NOT entirely correct, if we call turnScreenOn, mozPower.screenEnabled is assigned TRUE, but meanwhile, the turnScreenOff timer isn't timeout. Finally, mozPower.screenEnabled is assigned FALSE, and then screen is still off.

Comment 10

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

Hi Etienne, could you help to check this one and provide some suggestions?

i put the reproduce video here:

https://www.youtube.com/watch?v=OgxiYIFN-_g&feature=youtu.be

It can also be reproduced on Flame, but with lower probability.

Comment 11

[Etienne Segonzac (:etienne)]

Attachment: WIP

I this fixing the issue?

Comment 12

[Shiwei Zhang]

(In reply to Etienne Segonzac (:etienne) from comment #11)
> Created attachment 8498879 [details] [diff] [review]
> WIP
> 
> I this fixing the issue?

Unfortunately, issue still exist unless removing the timer.

Can we remove the timer?

Comment 13

[Etienne Segonzac (:etienne)]

Attachment: WIP v2

New patch! Let me know.

(I wouldn't recommend just removing the timeout for now since it could cause edge cases bug).

Comment 14

[Shiwei Zhang]

(In reply to Etienne Segonzac (:etienne) from comment #13)
> Created attachment 8499442 [details] [diff] [review]
> WIP v2
> 
> New patch! Let me know.
> 
> (I wouldn't recommend just removing the timeout for now since it could cause
> edge cases bug).

Yes! issue fixed, perfect patch!

Could you tell me why not remove this timer?

Thank you.

Comment 15

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

Comment 16

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

Comment on attachment 8500318 [details] [review]
Gaia PR

Sad setTimeout pattern :/

Comment 17

[Etienne Segonzac (:etienne)]

Landed on master https://github.com/mozilla-b2g/gaia/commit/84ebbc6d1fdc3d2d8691e61430e1d220b08c3114

Comment 18

[Etienne Segonzac (:etienne)]

Not sure how we want to go about uplifting or not on which branch (partner branch?) etc...
But I've checked that the patch can be cherry-picked without conflict on v1.4.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

It'll need approval for Gaia v1.4 at a minimum. It'll also need explicit approval for any other branches it affects.

Comment 20

[Etienne Segonzac (:etienne)]

The bug is hardware dependent, but the race can theoretically happen on all branches.

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8500318 [details] [review]
Gaia PR

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: screens needs to be waken up with the power button
[Testing completed]: basic call + proximity sensor scenarios, on a flame
[Risk to taking this patch] (and alternatives if risky): low, small patch on a file with very low churn
[String changes made]: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Based on comment 20, did you mean to request approval for the other branches as well then?

Comment 23

[Etienne Segonzac (:etienne)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> Based on comment 20, did you mean to request approval for the other branches
> as well then?

Frankly I'm not sure.

Since the user-facing bug is hardware dependent, do we want to land the fix on all the branches "just to be safe" or do we want the uplifts to be driven by actual devices launch?

Comment 24

[bhavana bajaj [:bajaj]]

Comment on attachment 8500318 [details] [review]
Gaia PR





Approving given this is a low risk patch handling a race and be safe here.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/8cb275457b19743649de977a9c24e989824b2ccd
v2.0: https://github.com/mozilla-b2g/gaia/commit/85acf1dd504eec9eef83f211086f35215e27ed8e
v1.4: https://github.com/mozilla-b2g/gaia/commit/077d245f6795c01cb0b77bceec9da23274206a61

Comment 26

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/85acf1dd504eec9eef83f211086f35215e27ed8e

Comment 27

[Coler]

This issue has been verified successfully on Flame2.0&2.1&2.2
Reproducing rate: 0/5
See attachment: Verify_Woodduck_P-Sensor.MP4

Note:Woodduck doesn't support P-Sensor now,The screen will turn off with Screen timeout in Settings.

Woodduck build version:
Gaia-Rev        87b23fa81c3b59f2ba24b84f7d686f4160d4e7cb
Gecko-Rev       d049d4ef127844121c9cf14d2e8ca91fd9045fcb
Build-ID        20141124050313
Version         32.0

Flame 2.0 build:
Gaia-Rev        124c2f85f1b956e9e8429dab5121de702a3bc197
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6b59dd2837c1
Build-ID        20141123000206
Version         32.0

Flame 2.1 build:
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
Build-ID        20141123001201
Version         34.0

Flame2.2 build:
Gaia-Rev        c5bad6d78c5fe168e3bb894fc5cb70902c9b19b1
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/c9cfa9b91dea
Build-ID        20141123040209
Version         36.0a1

Comment 28

[Coler]

Attachment: Verify_Woodduck_P-Sensor.MP4