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

VERIFIED FIXED in Firefox 21

Status

VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: vchang, Assigned: vchang)

Tracking

({regression})

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)
regression

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
(Assignee)

Updated

6 years ago
QA Contact: vchang
(Assignee)

Updated

6 years ago
Blocks: 823783
(Assignee)

Updated

6 years ago
Assignee: nobody → vchang
QA Contact: vchang
(Assignee)

Comment 1

6 years ago
Created attachment 703271 [details] [diff] [review]
Patch v1.0
Attachment #703271 - Flags: review?(mrbkap)
(Assignee)

Comment 2

6 years ago
Created attachment 704465 [details] [diff] [review]
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.
Attachment #703271 - Attachment is obsolete: true
Attachment #703271 - Flags: review?(mrbkap)
Attachment #704465 - Flags: review?(mrbkap)
Attachment #704465 - Flags: review?(dflanagan)
(Assignee)

Comment 3

6 years ago
Created attachment 704474 [details] [diff] [review]
Patch v1.2

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])) {
          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 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+
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 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+
(Assignee)

Comment 8

6 years ago
Created attachment 705685 [details] [diff] [review]
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.
Attachment #704474 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
blocking-b2g: --- → tef?
(Assignee)

Comment 9

6 years ago
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
blocking-b2g: tef? → tef+
Keywords: regression
https://hg.mozilla.org/mozilla-central/rev/9c792b677b4c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/releases/mozilla-b2g18/rev/715e2e7ce60a
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
Target Milestone: --- → B2G C4 (2jan on)
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.
status-b2g18-v1.0.0: --- → fixed

Comment 13

6 years ago
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
Status: RESOLVED → VERIFIED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 847765
You need to log in before you can comment on or make changes to this bug.