1187262 - A device cannot connect B device through wifi

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Evan Tseng [:evanxd]]

STR:
1. Set A device as Wi-Fi Hotspot, and turn off its data connection.
2. B device connects with A device.
3. Install a simple http server[1] in B device, and start the server. Then you will see the server IP there.
4. Open Browser in A device, and input the server address to check the server.

Expected result
See "It works!" message in A device's Browser.

Actual result 
See "No access to the Internet right now." message in A device's Browser.

P.S. If you turn on A device's data connection, then it works, A device can connect with B device.

Comment 1

[Evan Tseng [:evanxd]]

Henry,

How do you think about the bug?

Thanks.

Comment 2

[Henry Chang [:hchang]]

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #1)
> Henry,
> 
> How do you think about the bug?
> 
> Thanks.

I guess it's due to the routing table is not configured properly whenever 
the data interface is OFF. It might be or not be a bug but I think we can
definitely make it better.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #0)
> 3. Install a simple http server[1] in B device, and start the server. Then
> you will see the server IP there.

How do I do that? Do we already have a TCP server API for that?

Comment 4

[Evan Tseng [:evanxd]]

Install the simple http server[1].

[1]: https://github.com/evanxd/people-counter/tree/master/app

Comment 5

[Henry Chang [:hchang]]

I will be mentoring our intern to deal with this bug.

Comment 6

[XiaoLe Sun]

We found it is due to the flag "Services.io.offline" is set, even the device is set as Hotspot. And we are now finding the best way to fixed it!

Comment 7

[XiaoLe Sun]

Attachment: Bug1187262.patch

Comment 8

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8642921 [details] [diff] [review]
Bug1187262.patch

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

Thanks for helping on this, please see my comments below.

::: dom/system/gonk/NetworkService.js
@@ +632,5 @@
>          aCallback.wifiTetheringEnabledChange("netd command error");
>        } else {
>          aCallback.wifiTetheringEnabledChange(null);
> +        this.isTetheringEnabled = aEnable;
> +        Services.io.offline = Services.io.offline && (!aEnable);

We should check "network.gonk.manage-offline-status" pref before toggling this flag.

IMO, we could check this pref in TetheringService and control this flag there, just like NetworkManager does. Also, expose an additional property in nsITetheringService, e.g. 'state', for other modules to know whether tethering is active.

@@ +656,5 @@
>          aCallback.usbTetheringEnabledChange("netd command error");
>        } else {
>          aCallback.usbTetheringEnabledChange(null);
> +        this.isTetheringEnabled = aEnable;
> +        Services.io.offline = Services.io.offline && (!aEnable);

ditto.

Comment 9

[XiaoLe Sun]

Attachment: Bug1187262.patch

Comment 10

[XiaoLe Sun]

Attachment: Bug1187262.patch

Comment 11

[XiaoLe Sun]

Attachment: Bug1187262.patch

Comment 12

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8650350 [details] [diff] [review]
Bug1187262.patch

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

Please see my comments below. Thanks!

And I don't see where we handle checking tethering state when network changes in order to set the offline pref. Is the change in NetworkManager missing here?

::: dom/system/gonk/TetheringService.js
@@ +126,4 @@
>    Services.obs.addObserver(this, TOPIC_MOZSETTINGS_CHANGED, false);
>    Services.obs.addObserver(this, TOPIC_CONNECTION_STATE_CHANGED, false);
>    Services.prefs.addObserver(PREF_NETWORK_DEBUG_ENABLED, this, false);
> +  Services.prefs.addObserver(PREF_MANAGE_OFFLINE_STATUS, this, false);

Nit: add new empty line here.

@@ +603,4 @@
>            (!aEnable || aError)) {
>          this.handleDunConnection(false);
>        }
> +      this.state = aEnable ? 1 : 0;

Use defined consts here and elsewhere. And should we change the state when enabling fails?

@@ +677,5 @@
>      this.enableWifiTethering(true, aConfig, aCallback);
>    },
>  
> +  // The state of Tethering
> +  state: 0,

Please put this together with the other attributes.

@@ +840,5 @@
>  
>      callback.call(this);
>    },
> +
> +  _manageOfflineStatus: true,

Ditto.

::: dom/system/gonk/nsINetworkManager.idl
@@ +129,5 @@
> +   * true - Yes, there is!
> +   * false - No, there isn't.
> +   *
> +   */
> +  readonly attribute boolean anyConnected;

'anyConnected' is a local variable in NetworkManager, you can not expose it this way. Please use attribute 'allNetworkInfo' to tell whether there is any connected network, you can write a wrapper in TetheringService for this.

::: dom/system/gonk/nsITetheringService.idl
@@ +28,5 @@
>                          in jsval config,
>                          in nsIWifiTetheringCallback callback);
> +
> +  /**
> +   * The state of Tethering

Nit: please add a period at end of line. Help add it to the comment of setWifiTethering() too, thanks!

@@ +35,5 @@
> +   * 1 : Wifi.
> +   * 2 : Usb.
> +   *
> +   */
> +  readonly attribute long state;

Let's define some consts for this attribute, e.g. TETHERING_STATE_INACTIVE. And please move this attribute before functions (and after the added consts).

Comment 13

[XiaoLe Sun]

Attachment: Bug1187262.patch

:)

Comment 14

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8659793 [details] [diff] [review]
Bug1187262.patch

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

Thanks Xiaole, please see my comments below.
And I am not sure whether the changes in WifiP2pManager.jsm belong to this bug.

::: dom/system/gonk/NetworkManager.js
@@ +847,5 @@
>          }
>  
>          if (this._manageOfflineStatus) {
> +          Services.io.offline = (!anyConnected)
> +            && (Ci.nsITetheringService.state === Ci.nsITetheringService.TETHERING_STATE_INACTIVE ? true : false);

Try to avoid lines longer than 80 characters and seems like the ternary operator is redundant here.

You could refer Mozilla's coding style here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: dom/system/gonk/TetheringService.js
@@ +129,2 @@
>    Services.prefs.addObserver(PREF_NETWORK_DEBUG_ENABLED, this, false);
> +  Services.prefs.addObserver(PREF_MANAGE_OFFLINE_STATUS, this, false);

Please help remove observer on TOPIC_XPCOM_SHUTDOWN.

@@ +244,2 @@
>  
> +  // The state of Tethering

Nit: please add a period at end of line, no need to use uppercase in tethering.

@@ +605,4 @@
>      this._wifiTetheringRequestOngoing = true;
>      gNetworkService.setWifiTethering(aEnable, aConfig, (aError) => {
>        // Disconnect dun on error or when wifi tethering is disabled.
> +      if (this.tetheringSettings[SETTINGS_DUN_REQUIRED] && (!aEnable || aError)) {

Try to avoid lines longer than 80 characters.

@@ +610,3 @@
>          this.handleDunConnection(false);
> +      } else if(aError) this.state = this.TETHERING_STATE_INACTIVE;
> +      else this.state = aEnable ? Ci.nsITetheringService.TETHERING_STATE_WIFI : Ci.nsITetheringService.TETHERING_STATE_INACTIVE;

Can this be written as below?

      if (aEnable && !aError) {
        this.state = Ci.nsITetheringService.TETHERING_TYPE_WIFI;
      } else {
        this.state = Ci.nsITetheringService.TETHERING_STATE_INACTIVE;

        if (this.tetheringSettings[SETTINGS_DUN_REQUIRED]) {
          this.handleDunConnection(false);
        }
      }

@@ +611,5 @@
> +      } else if(aError) this.state = this.TETHERING_STATE_INACTIVE;
> +      else this.state = aEnable ? Ci.nsITetheringService.TETHERING_STATE_WIFI : Ci.nsITetheringService.TETHERING_STATE_INACTIVE;
> +
> +      if (this._manageOfflineStatus) {
> +        Services.io.offline = (!this.isAnyConnected()) && (this.state === Ci.nsITetheringService.TETHERING_STATE_INACTIVE ? true : false);

Try to avoid lines longer than 80 characters and seems like the ternary operator is redundant here.

@@ +759,5 @@
>          this._usbTetheringAction = TETHERING_STATE_IDLE;
>          if (this.tetheringSettings[SETTINGS_DUN_REQUIRED]) {
>            this.handleDunConnection(false);
>          }
> +        this.state = Ci.nsITetheringService.TETHERING_STATE_INACTIVE;

Please move this line after |this._usbTetheringAction = TETHERING_STATE_IDLE;|, thanks.

@@ +760,5 @@
>          if (this.tetheringSettings[SETTINGS_DUN_REQUIRED]) {
>            this.handleDunConnection(false);
>          }
> +        this.state = Ci.nsITetheringService.TETHERING_STATE_INACTIVE;
> +      }

Nit: please help add an extra line here.

@@ +761,5 @@
>            this.handleDunConnection(false);
>          }
> +        this.state = Ci.nsITetheringService.TETHERING_STATE_INACTIVE;
> +      }
> +      if(this._manageOfflineStatus){

Add a space between 'if' and '(' and between ')' and '{'.

@@ +762,5 @@
>          }
> +        this.state = Ci.nsITetheringService.TETHERING_STATE_INACTIVE;
> +      }
> +      if(this._manageOfflineStatus){
> +        Services.io.offline = (!this.isAnyConnected()) && (this.state === Ci.nsITetheringService.TETHERING_STATE_INACTIVE ? true : false);

Ditto.

@@ +845,5 @@
>  
>      callback.call(this);
>    },
> +
> +  isAnyConnected: function(){

Add a space between ')' and '{'.

@@ +847,5 @@
>    },
> +
> +  isAnyConnected: function(){
> +    let anyConnected = false;
> +    let allNetworks = Ci.nsINetworkManager.allNetworkInfo;

You could use the already defined gNetworkManager here.

@@ +849,5 @@
> +  isAnyConnected: function(){
> +    let anyConnected = false;
> +    let allNetworks = Ci.nsINetworkManager.allNetworkInfo;
> +    for each (let network in allNetworks) {
> +      if(network.info.state != Ci.nsINetworkInfo.NETWROK_STATE_CONNECTED) {

Please add a space between 'if' and '(' and fix typo in 'NETWROK_STATE_CONNECTED'.

::: dom/system/gonk/nsITetheringService.idl
@@ +19,5 @@
> +   * The state of Tethering.
> +   *
> +   * 0 : Inactive.
> +   * 1 : Wifi.
> +   * 2 : Usb.

No need for this, just write:

/**
 * Current tethering state. One of the TETHERING_STATE_* constants.
 */

Comment 15

[XiaoLe Sun]

Attachment: Bug1187262.patch

Thank you :目

Comment 16

[XiaoLe Sun]

Attachment: Bug1187262.patch

Thanks!!!! :目

Comment 17

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8664596 [details] [diff] [review]
Bug1187262.patch

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

Thanks Xiaole, we are almost there. Please see my comments below.
BTW, have you tested the patch locally?

And please follow [1] to generate a well-formed patch. Thanks.

::: dom/system/gonk/NetworkManager.js
@@ +840,5 @@
> +              === Ci.nsITetheringService.TETHERING_STATE_INACTIVE)) {
> +            Services.io.offline = true;
> +          } else {
> +            Services.io.offline = false;
> +          }

Can we just write...?
Services.io.offline = !anyConnected &&
                      (gTetheringService.state ===
                       Ci.nsITetheringService.TETHERING_STATE_INACTIVE);

(Remember to add lazy service getter for gTetheringService)

::: dom/system/gonk/TetheringService.js
@@ +242,5 @@
> +  // The state of tethering.
> +  state: Ci.nsITetheringService.TETHERING_STATE_INACTIVE,
> +
> +  // Flag to check if we can modify the Services.io.offline.
> +  _manageOfflineStatus: false,

Set default to true, like NetworkManager.

@@ +273,4 @@
>          Services.obs.removeObserver(this, TOPIC_MOZSETTINGS_CHANGED);
>          Services.obs.removeObserver(this, TOPIC_CONNECTION_STATE_CHANGED);
>          Services.prefs.removeObserver(PREF_NETWORK_DEBUG_ENABLED, this);
> +	Services.prefs.removeObserver(PREF_MANAGE_OFFLINE_STATUS, this);

Indent using spaces.

@@ +614,5 @@
> +        // Disconnect dun on error or when wifi tethering is disabled.
> +        if (this.tetheringSettings[SETTINGS_DUN_REQUIRED]) {
> +          this.handleDunConnection(false);
> +        }
> +      }

Nit: please help add an extra line here.

@@ +621,5 @@
> +          && (this.state === Ci.nsITetheringService.TETHERING_STATE_INACTIVE)) {
> +          Services.io.offline = true;
> +        } else {
> +          Services.io.offline = false;
> +        }

Can we just write...?
Services.io.offline = !this.isAnyConnected() &&
                      (this.state ===
                       Ci.nsITetheringService.TETHERING_STATE_INACTIVE);

@@ +779,5 @@
> +          Services.io.offline = true;
> +        }
> +        else {
> +          Services.io.offline = false;
> +        }

Ditto.

@@ +871,5 @@
> +      }
> +      anyConnected = true;
> +    }
> +    return anyConnected;
> +  },

The for each...in statement is deprecated, let's use for...in instead:

let allNetworkInfo = gNetworkManager.allNetworkInfo;
for (let networkId in allNetworkInfo) {
  if (allNetworkInfo.hasOwnProperty(networkId) &&
      allNetworkInfo[networkId].state === Ci.nsINetworkInfo.NETWORK_STATE_CONNECTED) {
    return true;
  }
}
return false;

::: dom/system/gonk/nsITetheringService.idl
@@ +10,1 @@
>  [scriptable, uuid(993b79df-f10e-4697-a5dc-5981cf8ff7e6)]

Need an uuid bump here.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Add missing link...

(In reply to Jessica Jong [:jjong] [:jessica] from comment #17)
> Comment on attachment 8664596 [details] [diff] [review]
> Bug1187262.patch
> 
> Review of attachment 8664596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Xiaole, we are almost there. Please see my comments below.
> BTW, have you tested the patch locally?
> 
> And please follow [1] to generate a well-formed patch. Thanks.

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

> 
> ::: dom/system/gonk/NetworkManager.js
> @@ +840,5 @@
> > +              === Ci.nsITetheringService.TETHERING_STATE_INACTIVE)) {
> > +            Services.io.offline = true;
> > +          } else {
> > +            Services.io.offline = false;
> > +          }
> 
> Can we just write...?
> Services.io.offline = !anyConnected &&
>                       (gTetheringService.state ===
>                        Ci.nsITetheringService.TETHERING_STATE_INACTIVE);
> 
> (Remember to add lazy service getter for gTetheringService)
> 
> ::: dom/system/gonk/TetheringService.js
> @@ +242,5 @@
> > +  // The state of tethering.
> > +  state: Ci.nsITetheringService.TETHERING_STATE_INACTIVE,
> > +
> > +  // Flag to check if we can modify the Services.io.offline.
> > +  _manageOfflineStatus: false,
> 
> Set default to true, like NetworkManager.
> 
> @@ +273,4 @@
> >          Services.obs.removeObserver(this, TOPIC_MOZSETTINGS_CHANGED);
> >          Services.obs.removeObserver(this, TOPIC_CONNECTION_STATE_CHANGED);
> >          Services.prefs.removeObserver(PREF_NETWORK_DEBUG_ENABLED, this);
> > +	Services.prefs.removeObserver(PREF_MANAGE_OFFLINE_STATUS, this);
> 
> Indent using spaces.
> 
> @@ +614,5 @@
> > +        // Disconnect dun on error or when wifi tethering is disabled.
> > +        if (this.tetheringSettings[SETTINGS_DUN_REQUIRED]) {
> > +          this.handleDunConnection(false);
> > +        }
> > +      }
> 
> Nit: please help add an extra line here.
> 
> @@ +621,5 @@
> > +          && (this.state === Ci.nsITetheringService.TETHERING_STATE_INACTIVE)) {
> > +          Services.io.offline = true;
> > +        } else {
> > +          Services.io.offline = false;
> > +        }
> 
> Can we just write...?
> Services.io.offline = !this.isAnyConnected() &&
>                       (this.state ===
>                        Ci.nsITetheringService.TETHERING_STATE_INACTIVE);
> 
> @@ +779,5 @@
> > +          Services.io.offline = true;
> > +        }
> > +        else {
> > +          Services.io.offline = false;
> > +        }
> 
> Ditto.
> 
> @@ +871,5 @@
> > +      }
> > +      anyConnected = true;
> > +    }
> > +    return anyConnected;
> > +  },
> 
> The for each...in statement is deprecated, let's use for...in instead:
> 
> let allNetworkInfo = gNetworkManager.allNetworkInfo;
> for (let networkId in allNetworkInfo) {
>   if (allNetworkInfo.hasOwnProperty(networkId) &&
>       allNetworkInfo[networkId].state ===
> Ci.nsINetworkInfo.NETWORK_STATE_CONNECTED) {
>     return true;
>   }
> }
> return false;
> 
> ::: dom/system/gonk/nsITetheringService.idl
> @@ +10,1 @@
> >  [scriptable, uuid(993b79df-f10e-4697-a5dc-5981cf8ff7e6)]
> 
> Need an uuid bump here.

Comment 19

[XiaoLe Sun]

Attachment: Bug1187262.patch

Oh...I found my misuse of the allNetworkInfo......Thank you very much QQ
I've tested it, but previously, I just test with tethering, and forgot to test with network connection.
This time I have tested them both!!!! :)

Thanks :目

Comment 20

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8664718 [details] [diff] [review]
Bug1187262.patch

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

Thanks Xiaole, the patch looks good, just want to make sure if it's in the right format. I though I would see something like:

# HG changeset patch
# User xxxx
# Date xxxx

Did you follow the steps in [1]? Thanks.

[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 21

[XiaoLe Sun]

Attachment: Bug1187262.patch

I have changed the format, it is now in mercurial format! :)

Please check >口<！

Thank you very much! :目

Comment 22

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8667674 [details] [diff] [review]
Bug1187262.patch

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

Thanks Xiaole, looks good overall.
But I did find a corner case that we need to handle, please see my comments below.

::: dom/system/gonk/TetheringService.js
@@ +608,5 @@
>      gNetworkService.setWifiTethering(aEnable, aConfig, (aError) => {
> +      if (aEnable && !aError) {
> +        this.state = Ci.nsITetheringService.TETHERING_STATE_WIFI;
> +      } else {
> +        this.state = Ci.nsITetheringService.TETHERING_STATE_INACTIVE;

There are cases where we defer wifi requests when usb requests is ongoing (and viceversa), this happens usually when we turn one off and the other on, and the requests could be out of order. :(
We need to consider this case, e.g.

// Avoid overriding state on interleaving cases.
if (this.state != Ci.nsITetheringService.TETHERING_STATE_USB) {
  this.state = Ci.nsITetheringService.TETHERING_STATE_INACTIVE;
}

And do the same for usb tethering.
This is ugly, but there seems to be no better option before refactoring all these.

Note: UI blocks the case for both tethering to be enabled at the same time, so we can consider one 'off' and one 'on' for now.

Comment 23

[XiaoLe Sun]

Attachment: Bug1187262.patch

Thanks! :)

Comment 24

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8667816 [details] [diff] [review]
Bug1187262.patch

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

Thanks you for the work, and remember to run your patch on try server.

::: dom/system/gonk/TetheringService.js
@@ +607,5 @@
>      this._wifiTetheringRequestOngoing = true;
>      gNetworkService.setWifiTethering(aEnable, aConfig, (aError) => {
> +      if (aEnable && !aError) {
> +        this.state = Ci.nsITetheringService.TETHERING_STATE_WIFI;
> +      } else {

Please leave a comment on why we are doing this. Thanks.

@@ +755,5 @@
>        this.tetheringSettings[SETTINGS_USB_ENABLED] = false;
>        settingsLock.set("tethering.usb.enabled", false, null);
>        // Skip others request when we found an error.
>        this._usbTetheringRequestCount = 0;
>        this._usbTetheringAction = TETHERING_STATE_IDLE;

Ditto.

@@ +756,5 @@
>        settingsLock.set("tethering.usb.enabled", false, null);
>        // Skip others request when we found an error.
>        this._usbTetheringRequestCount = 0;
>        this._usbTetheringAction = TETHERING_STATE_IDLE;
> +      if(this.state != Ci.nsITetheringService.TETHERING_STATE_WIFI) {

nit: space between 'if' and '('.

@@ +770,2 @@
>        } else {
>          this._usbTetheringAction = TETHERING_STATE_IDLE;

Ditto.

Comment 25

[XiaoLe Sun]

Attachment: Bug1187262.patch

My first r+ ！！！！
Thank you very much!!!!! :目

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/e50d6b04dc0b

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e50d6b04dc0b