Closed Bug 831716 Opened 10 years ago Closed 10 years ago

[Settings] [Internet sharing][wifi] Tapping Wi-fi hotspot/wifi ON disable Wi-fi connection forever


(Firefox OS Graveyard :: Wifi, defect)

Gonk (Firefox OS)
Not set


(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed


(Reporter: vchang, Assigned: vchang)



(Keywords: regression)


(1 file, 3 obsolete files)

This is a follow up for bug 823783 comment 44. 


1) Tap on Settings from homescreen
2) Select "Internet sharing" Under Network & Connectivity
3) Tap on "Wifi-hotspot" to enable it
4) Notice that "Wi-Fi" is disabled on the status bar
5) Click "Back" button
6) From Settings menu Click on "Wi-fi"
7) tap on "wifi" to enable it

repeat step 3~7.
QA Contact: vchang
Blocks: 823783
Assignee: nobody → vchang
QA Contact: vchang
Attached patch Patch v1.0 (obsolete) — Splinter Review
Attachment #703271 - Flags: review?(mrbkap)
Attached patch Patch v1.1 (obsolete) — Splinter Review
I found that gNetworkManager.wifiTetheringEnabled and WifiManager.enabled may not reflect the state of wifi hotspot and wifi correctly while receiving settings change event. Also, I integrate some of David's patch in Bug 823783. But I remove do while loop in nextRequest() function, and always serves the next request in the queue when calling nextRequest function.
Attachment #703271 - Attachment is obsolete: true
Attachment #703271 - Flags: review?(mrbkap)
Attachment #704465 - Flags: review?(mrbkap)
Attachment #704465 - Flags: review?(dflanagan)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Sorry, I forget to remove the patch comes from Bug 830299 - Crash in settings app.
Attachment #704465 - Attachment is obsolete: true
Attachment #704465 - Flags: review?(mrbkap)
Attachment #704465 - Flags: review?(dflanagan)
Attachment #704474 - Flags: review?(mrbkap)
Attachment #704474 - Flags: review?(dflanagan)
Comment on attachment 704474 [details] [diff] [review]
Patch v1.2

Review of attachment 704474 [details] [diff] [review]:

As part of this patch, could you also look at the code in _notifyAfterStateChange?  I'm concerned about this code:

      do {
        if (!("callback" in this._stateRequests[0])) {
        // Don't remove more than one request if the previous one failed.
      } while (success &&
               this._stateRequests.length &&
               this._stateRequests[0].enabled === state);

I cannot tell that it will not enter an infinite loop, and that scares me. It seems to me that the while condition should include a test for the callback property.

Building now. I'll comment again when I have tested.

::: dom/wifi/WifiWorker.js
@@ +1163,5 @@
> +              if (result) {
> +                manager.tetheringState = "UNINITIALIZED";
> +              } else {
> +                manager.tetheringState = "COMPLETED";
> +              }

In this function you're always calling the callback before updating manager.tetheringState. Is that intentional?

Just from an API point of view, it seems more useful to update the state and then call the callback, so that the callback function can know what the outcome is.  Or even pass true on success and false on failure.

@@ +1183,5 @@
>            if (status < 0) {
>              debug("Fail to unload wifi driver");
>            }
> +          callback();
> +          manager.tetheringState = "UNINITIALIZED";

Same comment here. Set tetheringState before calling the callback?

@@ +2826,5 @@
>        }
>      }
>    },
> +  notifyTetheringStateChange: function notifyTetheringStateChange() {

Please change the name of this method to notifyTetheringOff or something similar, since it only handles one kind of state change.

@@ +2847,1 @@
>        this.setWifiEnabledInternal(false, function(data) {

While I'm suggesting name changes, I think setWifiEnabledInternal should change as well. If I understand it correctly, I think this function should be called queueRequest().

@@ +2847,2 @@
>        this.setWifiEnabledInternal(false, function(data) {
> +        this.setWifiApEnabled(data, this.notifyTetheringStateChange.bind(this));

I think the code would be clearer if you hardcoded false here instead of data.  The handling of that first argument to setWifiEnabledInternal is one of the most confusing aspects of this code to me.
Comment on attachment 704474 [details] [diff] [review]
Patch v1.2

This seems to fix the bug for me, so r+, but please consider my review comments as well.

If I repeast wifi on, hotspot on, wifi on, hotspot on and keep going, I was able to get the phone into a state where wifi was not turning on. But apparently it was just very backed up. After what seemed like a long time (15 seconds maybe) wifi came on.

Also, I once got the phone into a state where the status bar showed both hotspot and wifi.  I turned on the hotspot, quit settings. Then restarted settings, which turned on wifi.  I can't reproduce this now, though.
Attachment #704474 - Flags: review?(dflanagan) → review+

Feel free to take bug 823783, attach this patch to it, and land it there under the tef+ (especially if you fix the scary possible infinite loop thing I describe in my review.)
Comment on attachment 704474 [details] [diff] [review]
Patch v1.2

Review of attachment 704474 [details] [diff] [review]:

I'm fine with this patch with David's changes applied.
Attachment #704474 - Flags: review?(mrbkap) → review+
Attached patch Patch v1.3Splinter Review
This patch address the comment 4 and fix the possible infinite loop bug.
I have tested it and seems work fine for me.
Attachment #704474 - Attachment is obsolete: true
blocking-b2g: --- → tef?
This should be marked as tef+ because it blocks Bug 823783 which is a tef+ bug.
Bug 823783 is provided the gaia part fix while this patch provides gecko part fix. So we could close Bug 823783 once this bug is landed.  

I have got the r+ in patch v1.2 and addressed the comment 4 in patch v1.3. 
So landed my patch v1.3.
blocking-b2g: tef? → tef+
Keywords: regression
Closed: 10 years ago
Resolution: --- → FIXED
Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.
Verified this issue has been fixed on Unagi 

Build ID 20130214070203
Kernel Dec 5
Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49
You need to log in before you can comment on or make changes to this bug.