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)
Tracking
(blocking-b2g:-)
RESOLVED
WONTFIX
blocking-b2g | - |
People
(Reporter: sync-1, Assigned: zhiming.tang)
Details
Attachments
(3 files, 3 obsolete files)
849.17 KB,
text/plain
|
Details | |
1.91 KB,
patch
|
alive
:
review+
|
Details | Diff | Splinter Review |
1.58 KB,
patch
|
Details | Diff | Splinter Review |
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:
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
Comment 5•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
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...
Assignee | ||
Comment 10•11 years ago
|
||
We have a temporary solution to acquire the CPU wake lock before the sleep and release it until the WIFI is turned-off successfully.
Comment 11•11 years ago
|
||
(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.
Comment 13•11 years ago
|
||
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)
Updated•11 years ago
|
Component: Wifi → Gaia::System
Comment 14•11 years ago
|
||
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)
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Attachment #761854 -
Flags: review?(timdream)
Assignee | ||
Updated•11 years ago
|
Attachment #761854 -
Flags: review?(timdream) → review-
Assignee | ||
Comment 17•11 years ago
|
||
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 18•11 years ago
|
||
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-
Assignee | ||
Comment 19•11 years ago
|
||
Release the wake lock until "wifi-disabled" comes to gaia or time-out.
Attachment #763328 -
Flags: review?(alive)
Comment 20•11 years ago
|
||
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+
Assignee | ||
Comment 21•11 years ago
|
||
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 22•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
Thanks a lot Alive! The pull request id is: https://github.com/mozilla-b2g/gaia/pull/10422
Assignee | ||
Comment 24•11 years ago
|
||
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)
Comment 25•11 years ago
|
||
See https://travis-ci.org/mozilla-b2g/gaia/builds/8150962 It's not your fault so I will merge it now.
Flags: needinfo?(alive)
Comment 26•11 years ago
|
||
(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.
Assignee | ||
Comment 27•11 years ago
|
||
I started another pull-request https://github.com/mozilla-b2g/gaia/pull/10425 Does that OK for you?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(alive)
Comment 28•11 years ago
|
||
(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)
Assignee | ||
Comment 29•11 years ago
|
||
Another pull request is done: https://github.com/mozilla-b2g/gaia/pull/10431
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(alive)
Comment 30•11 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/26ae78465aafaab3969f4b931db8254557f8ea6c
Assignee: nobody → zhiming.tang
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(alive)
Resolution: --- → FIXED
Assignee | ||
Comment 31•11 years ago
|
||
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 → ---
Assignee | ||
Comment 32•11 years ago
|
||
Attachment #763328 -
Attachment is obsolete: true
Attachment #765798 -
Flags: review?(vchang)
Comment 33•11 years ago
|
||
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.
Assignee | ||
Comment 34•11 years ago
|
||
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 35•11 years ago
|
||
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)
Assignee | ||
Comment 36•11 years ago
|
||
(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?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(vchang)
Comment 37•11 years ago
|
||
Moving to leo? since TAs are behind us for 1.0.1 partners and tef? is being shut down.
blocking-b2g: tef? → leo?
Comment 38•11 years ago
|
||
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? → -
Updated•11 years ago
|
Flags: needinfo?(vchang)
Comment 39•6 years ago
|
||
Firefox OS is not being worked on
Status: REOPENED → RESOLVED
Closed: 11 years ago → 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•