906631 - [Wifi] Hotspot and Wifi might both be enabled at same time.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

STR
1. Enable hotspot
2. Enable Wifi
3. Disable Wifi and then Enable Wifi quickly.

Expect:
Wifi enabled, Hotspot disabled

Actual:
Status bar shows Wifi and Hotspot are enabled.

Comment 2

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Refactory command queue.

1. Make enable/disable request queue into producer-consumer pattern.
2. Make sure every request is executed in order without race condition.

Comment 3

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Move status check into callback function.

Move check rule into queue callback to make sure the state is synchronized.

Comment 4

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Refactory command queue.

Fix nit

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 794440 [details] [diff] [review]
0001. Refactory command queue.

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

::: dom/wifi/WifiWorker.js
@@ +2060,1 @@
>  

Why do we need to call requestDone() here ?

@@ +2074,5 @@
>      WifiManager.state = "UNINITIALIZED";
>      debug("Supplicant died!");
>  
> +    // Wifi disable request is done here.
> +    self.requestDone();

The Same here. Why do we need to call requestDone() ? If we really have to do something here, can we have a better naming for the function ?

@@ +2883,5 @@
> +        let retryTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +        retryTimer.initWithCallback(function(timer) {
> +          self._setWifiEnabledCallback(status).bind(self);
> +        }, 1000, Ci.nsITimer.TYPE_ONE_SHOT);
> +

Do we still need to check driver state ? This function always called after loadDriver or unLoadDriver in WifiManager.setWifiEnabled(). In that case, driver should be loaded or unloaded successfully.

@@ +3249,2 @@
>        }
> +    } while (!("callback" in request));

The request should contain callback, right. If it is true, we don't need the do {}while here, and make the code clearly.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 794440 [details] [diff] [review]
0001. Refactory command queue.

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

::: dom/wifi/WifiWorker.js
@@ +2874,5 @@
>    },
>  
> +  _setWifiEnabledCallback: function(status) {
> +    if (status) {
> +      let newState = (status === "no change" ? true : false);

Nit: Please write this as |let newState = (status === "no change");|

@@ +2904,5 @@
> +  },
> +
> +  queueRequest: function(enabled, callback) {
> +    if (callback) {
> +      // Push only valid request

Who would call this function without a callback? What'd be the meaning of that? I'd rather throw an exception if it's truly unexpected.

@@ +2985,5 @@
>    setWifiApEnabled: function(enabled, callback) {
>      let configuration = this.getWifiTetheringParameters(enabled);
>  
>      if (!configuration) {
> +      this.requestDone();

Why is it ok to not call callback here? Can you add a comment?

@@ +3203,5 @@
> +  requestProcessing: false,   // Hold while dequeue and execution a request.
> +                              // Released upon the request is fully executed,
> +                              // i.e, mostly after callback is done.
> +  requestTimer: null,         // Timer for handling request queue.
> +  requestTimerRunning: false, // Is requestTimer running.

Is the timer really necessary? My hope here was that we'd be able to do everything based on callbacks without having to guess (which we always do conservatively). Especially when e.g. we're turning the wifi off, we don't want to have to wake the phone up every 500 seconds... I'd really rather run this off of callbacks called from the WifiManager.

Comment 7

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 794440 [details] [diff] [review]
0001. Refactory command queue.

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

::: dom/wifi/WifiWorker.js
@@ +2074,5 @@
>      WifiManager.state = "UNINITIALIZED";
>      debug("Supplicant died!");
>  
> +    // Wifi disable request is done here.
> +    self.requestDone();

After some review, I think we might be able to get ride of these two calls.
I'll test it.

@@ +2883,5 @@
> +        let retryTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +        retryTimer.initWithCallback(function(timer) {
> +          self._setWifiEnabledCallback(status).bind(self);
> +        }, 1000, Ci.nsITimer.TYPE_ONE_SHOT);
> +

You are right, I will remove the check and test on phones.

@@ +2904,5 @@
> +  },
> +
> +  queueRequest: function(enabled, callback) {
> +    if (callback) {
> +      // Push only valid request

Will do!

@@ +2985,5 @@
>    setWifiApEnabled: function(enabled, callback) {
>      let configuration = this.getWifiTetheringParameters(enabled);
>  
>      if (!configuration) {
> +      this.requestDone();

I think it's because the callback is |notifyTetheringOff()| or |notifyTetheringOn| here.
When we have no |configuration| for |WifiManager.setWifiApEnabled()|, these callbacks can't be called.

@@ +3203,5 @@
> +  requestProcessing: false,   // Hold while dequeue and execution a request.
> +                              // Released upon the request is fully executed,
> +                              // i.e, mostly after callback is done.
> +  requestTimer: null,         // Timer for handling request queue.
> +  requestTimerRunning: false, // Is requestTimer running.

The timer will be canceled after request queue is empty, so phone won't be woken up continuously.

And if every request callbacks call |requestDone()| on complete, we don't need this timer.
It's more like a backup plan here to make sure the request queue can work correctly if some callback doesn't follow the rule.

I'll remove the timer and add some comment to |queueRequest()| to address this rule.

Comment 8

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Refactory command queue. V2

Address reviewers' comments.

Comment 9

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. Move status check into callback function.

Rebase.

Comment 10

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 800034 [details] [diff] [review]
0001. Refactory command queue. V2

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

Looks good, thanks you.

Comment 11

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Refactory command queue. V2

Add reviewer in summary.

Comment 12

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try : https://tbpl.mozilla.org/?tree=Try&rev=effacd9ae015

Comment 13

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try is OK, let's see what will happen in the real world....

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8ff506c57197
https://hg.mozilla.org/integration/b2g-inbound/rev/b3cb8e312203

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ff506c57197
https://hg.mozilla.org/mozilla-central/rev/b3cb8e312203