888227 - [Wi-Fi] wifiManager returns the string "undefined" instead of the value for macAddress when MAC addr is not available

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

As discovered when debugging the issue highlighted in bug 880617, 'wifiManager.macAddress' returns the string 'undefined' instead of the value undefined when it is not available.

Since the patch for bug 880617 changes the logic for updating the MAC address in Settings, the case when 'wifiManager.macAddress' is undefined (even as a string) doesn't have impact on the user and this fix will most probably be a correctness fix only (though the checks for the string "undefined" might need to be updated in apps/settings/js/settings.js and apps/settings/js/connectivity.js).

To reproduce add the following debugging code in apps/settings/js/connectivity.js:

+++ b/apps/settings/js/connectivity.js
@@ -101,6 +101,8 @@ var Connectivity = (function(window, document, undefined) {
       return; // init will call updateWifi()
     }
 
+    console.log('MAC addr:', wifiManager.macAddress);
+    console.log('typeof MAC addr:', typeof wifiManager.macAddress);
     if (wifiManager.enabled) {
       // network.connection.status has one of the following values:

toggle Wi-Fi off and restart the device.

Checking the console logs after reboot, when accessing Settings > Device Info you will see the following:

E/GeckoConsole(  401): Content JS LOG at app://settings.gaiamobile.org/js/connectivity.js:104 in updateWifi: MAC addr: undefined
E/GeckoConsole(  401): Content JS LOG at app://settings.gaiamobile.org/js/connectivity.js:105 in updateWifi: typeof MAC addr: string

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Hi dlee, can you help to check this ?

Comment 2

[Dimi Lee [:dimi]]

UI get "undefined" string because IDL declare macAddress as DOMString, so even in gecko layer the variable is undefined type, gaia will retrieve this value as string type with content "undefined"

There are three possible solution we may fix this issue, so i'll need suggestion about which solution is better in current architecture. If there are any other solution i do not list, please help share it, thanks

1. Change type of macAddress from DOMString to jsval in DOM API
   UI need to treat macAddress as jsval instead of string type

2. DOM API implementation set macAddress to null when we found its type is undefined.
   UI will get an null object in this case

3. Do not modify DOM API implementation
   UI need to check if macAddress is "undefined" string

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to dlee from comment #2)
> UI get "undefined" string because IDL declare macAddress as DOMString, so
> even in gecko layer the variable is undefined type, gaia will retrieve this
> value as string type with content "undefined"

Yes, indeed. Good catch.


> 1. Change type of macAddress from DOMString to jsval in DOM API
>    UI need to treat macAddress as jsval instead of string type

We should avoid doing this as much as possible. jsval provides the least amount of information about what's going on with the interface.

> 2. DOM API implementation set macAddress to null when we found its type is
> undefined.
>    UI will get an null object in this case

This is definitely the way to go. Can you write a patch to do this?

Comment 4

[Henry Chang [:hchang]]

What I am going to do is to set the default value of WifiWorker.js::this.macAddress to null or empty string, which avoids the unnecessary type check.

Comment 5

[Henry Chang [:hchang]]

Attachment: bug888227.patch

Comment 6

[Henry Chang [:hchang]]

Attachment: Proposed patch v1

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 780839 [details] [diff] [review]
Proposed patch v1

As the solution I proposed before, I just initiate macAddress as null, which will propagate to gaia and be treated as null as well. After applying this patch and reproduce the bug, you'll see

E/GeckoConsole(  401): Content JS LOG at app://settings.gaiamobile.org/js/connectivity.js:104 in updateWifi: MAC addr:
E/GeckoConsole(  401): Content JS LOG at app://settings.gaiamobile.org/js/connectivity.js:105 in updateWifi: typeof MAC addr: object

Comment 8

[Henry Chang [:hchang]]

Comment on attachment 780839 [details] [diff] [review]
Proposed patch v1

># HG changeset patch
># Parent dd9b6075038c24eec59ab0adc041ce73778dc0bb
># User Henry Chang <[e-mail]>
>Bug 888227 - [Wi-Fi] wifiManager returns the string "undefined" instead of the value for macAddress when MAC addr is not available; r=mrbkap
>
>diff --git a/dom/wifi/WifiWorker.js b/dom/wifi/WifiWorker.js
>--- a/dom/wifi/WifiWorker.js
>+++ b/dom/wifi/WifiWorker.js
>@@ -1845,16 +1845,17 @@ function WifiWorker() {
>   // Note that we don't have to worry about escaping embedded quotes since in
>   // all cases, the supplicant will take the last quotation that we pass it as
>   // the end of the string.
>   this.configuredNetworks = Object.create(null);
>   this._addingNetworks = Object.create(null);
> 
>   this.currentNetwork = null;
>   this.ipAddress = "";
>+  this.macAddress = null;
> 
>   this._lastConnectionInfo = null;
>   this._connectionInfoTimer = null;
>   this._reconnectOnDisconnect = false;
> 
>   // Users of instances of nsITimer should keep a reference to the timer until
>   // it is no longer needed in order to assure the timer is fired.
>   this._callbackTimer = null;

Comment 9

[Henry Chang [:hchang]]

Comment on attachment 780839 [details] [diff] [review]
Proposed patch v1

># HG changeset patch
># Parent dd9b6075038c24eec59ab0adc041ce73778dc0bb
># User Henry Chang <[e-mail]>
>Bug 888227 - [Wi-Fi] wifiManager returns the string "undefined" instead of the value for macAddress when MAC addr is not available; r=mrbkap
>
>diff --git a/dom/wifi/WifiWorker.js b/dom/wifi/WifiWorker.js
>--- a/dom/wifi/WifiWorker.js
>+++ b/dom/wifi/WifiWorker.js
>@@ -1845,16 +1845,17 @@ function WifiWorker() {
>   // Note that we don't have to worry about escaping embedded quotes since in
>   // all cases, the supplicant will take the last quotation that we pass it as
>   // the end of the string.
>   this.configuredNetworks = Object.create(null);
>   this._addingNetworks = Object.create(null);
> 
>   this.currentNetwork = null;
>   this.ipAddress = "";
>+  this.macAddress = null;
> 
>   this._lastConnectionInfo = null;
>   this._connectionInfoTimer = null;
>   this._reconnectOnDisconnect = false;
> 
>   // Users of instances of nsITimer should keep a reference to the timer until
>   // it is no longer needed in order to assure the timer is fired.
>   this._callbackTimer = null;

Comment 10

[Henry Chang [:hchang]]

Attachment: Proposed patch v2 (same as v1, added r=mrbkap)

Comment 11

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

Did this land on mozilla-central? I see the 'checkin-needed' flag was canceled quite a while ago, but the status of the bug was not updated to resolved.

Comment 12

[Henry Chang [:hchang]]

Attachment: patch v3 (modifying commit message)

Attached a new but identical patch except shorter commit message. Does it require another review?

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 791768 [details] [diff] [review]
patch v3 (modifying commit message)

If you've addressed all of the comments for a patch and the reviewer has already stamped r+ on it, there's no need to re-ask for review.

Comment 14

[Henry Chang [:hchang]]

Got it. Thanks

Comment 15

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

I think it's better run try before add 'checkin-needed'

Comment 16

[Henry Chang [:hchang]]

Sure and that what I am doing now :)

Comment 17

[Henry Chang [:hchang]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d45f91a1931