Closed Bug 881124 Opened 11 years ago Closed 6 years ago

[Buri]Wi-Fi did not disable immediately

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:-)

RESOLVED WONTFIX
blocking-b2g -

People

(Reporter: sync-1, Assigned: zhiming.tang)

Details

Attachments

(3 files, 3 obsolete files)

AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.131
 Firefox os  v1.0.1
 Mozilla build ID:20130606070202		
 
  DEFECT DESCRIPTION:
  Wi-Fi did not disable immediately
 
  REPRODUCING PROCEDURES:
 1.turn on Wi-Fi and connect with one ap.
 2.power off screen and after about 10 mins, wifi will disable,but it doesn't trun off immediately->KO1 
 
  EXPECTED BEHAVIOUR:
  Wi-Fi should turn off immediately.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:5/5
 
  For FT PR, Please list reference mobile's behavior:
blocking-b2g: --- → tef?
Clone from brother
Attached file this is the log
Clone from brother
gaia had send message but gecko didn't recive immediately util power on screen, it maybe caused
by wakeLocked,please help check。 like logs below:

06-07 10:44:37.070 E/GeckoConsole(  146): Content JS LOG at app://system.gaiamobile.org/js/wifi.js:247 in gotAlarm: rll----1212112->
06-07 10:44:37.070 E/GeckoConsole(  146): Content JS LOG at app://system.gaiamobile.org/js/wifi.js:227 in wifi_sleep: rll----124545->
06-07 10:46:16.010 I/Gecko   (  146): rll--gecko--set-2->wifi.enabled
06-07 10:46:16.010 I/Gecko   (  146): rll--gecko--set-2->false
Flags: needinfo?(jcheng)
ni to vliu
Flags: needinfo?(jcheng) → needinfo?(vliu)
 EXPECTED BEHAVIOUR:
  Wi-Fi should turn off immediately.

can you clarify on this? why should wifi turn off immediately when screen turns off? Thanks
Flags: needinfo?(sync-1)
When turn off screen,we have add a Alarm(10 mins) if wifi did not use and no charging. After about 10 mins, when alarm wake up, we will turn off wifi for saving energy. And next power on screen, wifi will reconnect itself.
Flags: needinfo?(vliu)
Flags: needinfo?(sync-1) → needinfo?(vliu)
In current FFOS design, when the time period of turned off screen was above |wifi.screen_off_timeout|, the wifi would be turned off. After that, if we turned on the screen, the wifi also be turned on trigged by gaia. I will check it on buri.
Flags: needinfo?(vliu)
the problem is -- when wifi.screen_off_timeout reached, Gaia send wifi_disable message to Gecko, but Gecko can't receive it until system waken up by turning on screen, then Gecko receive the outdated wifi_disable message which followed by wifi_enbale message.
Dear Mozilla,

We thought this issue is probably linked with Bug 866366.

Now mozilla uses an alarm to turn off the WIFI after screen turned off. When the alarm comes during the CPU suspend mode, it would acquire the CPU wake lock in the alarm handler and release it immediately after the handler returns. (https://mxr.mozilla.org/mozilla-b2g18/source/dom/messages/SystemMessageInternal.js#443)

However we suspect that the WIFI turning-off doesn't finish at that point because it is in an asynchronous process, there is no guarantee it's done before than | SystemMessageManager:HandleMessagesDone |. Worse, the process will be postponed to the point we light up the CPU, which makes the WIFI can not be turned on successfully...
We have a temporary solution to acquire the CPU wake lock before the sleep and release it until the WIFI is turned-off successfully.
(In reply to zhiming.tang from comment #10)
> We have a temporary solution to acquire the CPU wake lock before the sleep
> and release it until the WIFI is turned-off successfully.

This approach makes sense to me. Maybe you can post the patch for review.
Attached file Patch (obsolete) —
The wifi patch
Attachment #760784 - Flags: review?(vchang)
Comment on attachment 760784 [details]
Patch

I am not a reviewer for Gaia, redirect to Tim.
The problem here should be related to wake lock as mentioned in comment 9 and 10.
Attachment #760784 - Flags: review?(vchang) → review?(timdream)
Component: Wifi → Gaia::System
Comment on attachment 760784 [details]
Patch

Interesting. Is the diff +/0 signs inverted here?

This generally looks alright except it doesn't following styling rule.
https://github.com/mozilla-b2g/gaia/blob/master/CONTRIBUTING.md

Please amend the patch and file a pull request to Github.
Attachment #760784 - Flags: review?(timdream)
Attached file Patch_clean_gjslint_warning (obsolete) —
Hi Tim, 

Yes, the +/- sign of the patch was inverted before... don't know why...

I committed another patch cleaning the gjslint warnings and re-start a pull-request on Github, please help to have a check if it looks OK, thanks!
 https://github.com/mozilla-b2g/gaia/pull/10358
Attachment #761854 - Flags: review?(timdream)
Attachment #761854 - Flags: review?(timdream) → review-
Sorry, i just realized the wifi is still on with the alarm... should postpone the wake lock release till 'wifi-disabled' message comes to gaia.
Comment on attachment 761854 [details]
Patch_clean_gjslint_warning

Sure, I'll be waiting for your new patch on this.

You may also request :alive for review.
Attachment #761854 - Flags: review-
Attached patch wifi_patch_v3 (obsolete) — Splinter Review
Release the wake lock until "wifi-disabled" comes to gaia or time-out.
Attachment #763328 - Flags: review?(alive)
Comment on attachment 763328 [details] [diff] [review]
wifi_patch_v3

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

r+ but please fix the redundant indent and rename the wakeLock.

::: apps/system/js/wifi.js
@@ +209,5 @@
> +    // If the CPU is in suspend mode at this moment, alarm servcie would wake
> +    // up the CPU to run the handler and turn it back to suspend immediately
> +    // |sleep| is finished. In this case, we acquire a CPU wake lock to prevent
> +    // the CPU goes to suspend mode before the switching is done.
> +    var wakeLock1 = navigator.requestWakeLock('cpu');

Hmm, I don't like the naming like XXX1, XXX2...
Could you figure out a better one?
Like shortTermWakeLock, longTermWakeLock...
wakeLockForWifi, wakeLockForSettings, ..

@@ +214,3 @@
>      lock.set({ 'wifi.enabled': false });
> +    window.addEventListener('wifi-disabled', function() {
> +        if (wakeLock1) {

Additional indent.

@@ +231,5 @@
> +     // Keep this value in disk so if the phone reboots we'll
> +     // be able to turn the wifi back on.
> +     var wakeLock2 = navigator.requestWakeLock('cpu');
> +     var request = lock.set({ 'wifi.disabled_by_wakelock': true });
> +     request.onerror = request.onsuccess = function() {wakeLock2.unlock()};

function() { wakeLock2.unlock(); };

// put a space between braces and statements.

I think your wakeLock2 is for protecting the period before mozSettings writing is successfully finished. So please rename wakeLock2 to wakeLockForSettings.
Attachment #763328 - Flags: review?(alive) → review+
Attached patch wifi_patch_v4Splinter Review
Some amendments on the code review for v3
Attachment #760784 - Attachment is obsolete: true
Attachment #761854 - Attachment is obsolete: true
Attachment #763354 - Flags: review?(alive)
Comment on attachment 763354 [details] [diff] [review]
wifi_patch_v4

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

Excellent, could you send pull request via github? thanks.
Attachment #763354 - Flags: review?(alive) → review+
Thanks a lot Alive! The pull request id is:
https://github.com/mozilla-b2g/gaia/pull/10422
Hi Alive,

The travis CI failed on my pull request in that Linter reports some errors of the files, which I did not touch. Any idea? What I should do?
Flags: needinfo?(alive)
See https://travis-ci.org/mozilla-b2g/gaia/builds/8150962
It's not your fault so I will merge it now.
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive] from comment #25)
> See https://travis-ci.org/mozilla-b2g/gaia/builds/8150962
> It's not your fault so I will merge it now.

Sorry, but you have to rebase all commits into one before merging.
I started another pull-request 
https://github.com/mozilla-b2g/gaia/pull/10425

Does that OK for you?
Flags: needinfo?(alive)
(In reply to zhiming.tang from comment #27)
> I started another pull-request 
> https://github.com/mozilla-b2g/gaia/pull/10425
> 
> Does that OK for you?

Looks OK but only one thing: the commit message doesn't need a 'Fix' before 'Bug XXXXXX ....'
Flags: needinfo?(alive)
Another pull request is done:

https://github.com/mozilla-b2g/gaia/pull/10431
Flags: needinfo?(alive)
https://github.com/mozilla-b2g/gaia/commit/26ae78465aafaab3969f4b931db8254557f8ea6c
Assignee: nobody → zhiming.tang
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(alive)
Resolution: --- → FIXED
Hi Mozilla,

I re-open this PR temporarily since the fix is only OK for Gaia part. To make sure the driver is unloaded, we should acquire another wake lock in gecko to make sure the wifi turning-off is done before CPU sleep.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #763328 - Attachment is obsolete: true
Attachment #765798 - Flags: review?(vchang)
Recently, we're having a regression about the CPU wake lock mechanism. Please see bug 880931, comment#31. I'm wondering all the CPU wake locks are not working well now.
Huh... In the /gecko/dom/power, I see two parallel modules with alike API: PowerManager and PowerManagerServcie. What's the difference / relationship between them? 

The power-off / reboot / add(remove)WakeLockListener exist in both idl, why?
Comment on attachment 765798 [details] [diff] [review]
Acquire cpu wake lock in wifi sleep process

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

It's a little weird for me to hold a wake lock there. 
How about having a new event such like "wifiDisabled" and use that event to notify 
gaia apps wifi is disabled entirely ? The gaia apps release the wake lock when received "wifiDisabled" event.

BTW, the wifi driver is not unloaded when we disable the wifi by default. The unload driver is controlled by 
android property flag "ro.moz.wifi.unloaddriver". Not sure if it's enabled in Buri.
Attachment #765798 - Flags: review?(vchang)
(In reply to Vincent Chang[:vchang] from comment #35)
> Comment on attachment 765798 [details] [diff] [review]
> Acquire cpu wake lock in wifi sleep process
> 
> Review of attachment 765798 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's a little weird for me to hold a wake lock there. 
> How about having a new event such like "wifiDisabled" and use that event to
> notify 
> gaia apps wifi is disabled entirely ? The gaia apps release the wake lock
> when received "wifiDisabled" event.
> 
> BTW, the wifi driver is not unloaded when we disable the wifi by default.
> The unload driver is controlled by 
> android property flag "ro.moz.wifi.unloaddriver". Not sure if it's enabled
> in Buri.

I also thought of a new event of wifiDisabled to Gaia. But I worry that it become strange for Gaia to receive another "wifiDisabled" after "wifi-disabled". 

Besides, no matter if the property("ro.moz.wifi.unloaddriver") is set, the callback would be triggered nevertheless, is that right?
Flags: needinfo?(vchang)
Moving to leo? since TAs are behind us for 1.0.1 partners and tef? is being shut down.
blocking-b2g: tef? → leo?
Leo-, not a regression from 1.0.1 so not going to hold the 1.1 release for it. Let's pick up the patch in 1.2.
blocking-b2g: leo? → -
Flags: needinfo?(vchang)
Firefox OS is not being worked on
Status: REOPENED → RESOLVED
Closed: 11 years ago6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: