Closed Bug 888227 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox OS Graveyard :: Wifi, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mihai, Assigned: hchang)

References

Details

Attachments

(1 file, 3 obsolete files)

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
Assignee: nobody → vchang
Hi dlee, can you help to check this ?
Assignee: vchang → dlee
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
(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?
Assignee: dlee → 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.
Attached patch bug888227.patch (obsolete) — Splinter Review
Attachment #780830 - Attachment description: bug88827.patch → bug888227.patch
Attachment #780830 - Attachment filename: bug88827.patch → bug888227.patch
Attachment #780830 - Attachment is obsolete: true
Attached patch Proposed patch v1 (obsolete) — Splinter Review
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
Attachment #780839 - Flags: review?(mrbkap)
Attachment #780839 - Flags: review?(mrbkap) → review+
Comment on attachment 780839 [details] [diff] [review]
Proposed patch v1

># HG changeset patch
># Parent dd9b6075038c24eec59ab0adc041ce73778dc0bb
># User Henry Chang <hchang@mozilla.com>
>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 on attachment 780839 [details] [diff] [review]
Proposed patch v1

># HG changeset patch
># Parent dd9b6075038c24eec59ab0adc041ce73778dc0bb
># User Henry Chang <hchang@mozilla.com>
>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;
Keywords: checkin-needed
Attachment #781416 - Attachment description: Propose patch v2 (added r=mrbkap) → Proposed patch v2 (same as v1, added r=mrbkap)
Attachment #780839 - Attachment is obsolete: true
Keywords: checkin-needed
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.
Flags: needinfo?(hchang)
Attached a new but identical patch except shorter commit message. Does it require another review?
Attachment #781416 - Attachment is obsolete: true
Attachment #791768 - Flags: review?(mrbkap)
Flags: needinfo?(hchang)
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.
Attachment #791768 - Flags: review?(mrbkap) → review+
Got it. Thanks
I think it's better run try before add 'checkin-needed'
Sure and that what I am doing now :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4d45f91a1931
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 908426
You need to log in before you can comment on or make changes to this bug.