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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Vincent Chang[:vchang][changyihsin]]

This is a follow up for bug 823783 comment 44. 

STR

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.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0

Comment 2

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.1

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.

Comment 3

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.2

Sorry, I forget to remove the patch comes from Bug 830299 - Crash in settings app.

Comment 4

[David Flanagan [:djf]]

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])) {
          this._stateRequests.shift();
        }
        // 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 5

[David Flanagan [:djf]]

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.

Comment 6

[David Flanagan [:djf]]

Vincent,

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 7

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 8

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.3

This patch address the comment 4 and fix the possible infinite loop bug.
I have tested it and seems work fine for me.

Comment 9

[Vincent Chang[:vchang][changyihsin]]

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. 

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c792b677b4c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c792b677b4c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/715e2e7ce60a

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 13

[pallavi]

Verified this issue has been fixed on Unagi 

Build ID 20130214070203
Kernel Dec 5
Gecko:http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e
Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49