895739 - [Wi-Fi] MAC address is always displayed as "not available"

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

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

The patch for bug 880617 introduced a simplified logic of updating the MAC address value to be displayed in "Wi-Fi > Manage networks" and "Device Information > More information" panels of the Settings app by storing the value provided by wifiManager.macAddress in mozSettings under 'deviceinfo.mac'.

Because of invalid values for 'deviceinfo.mac' in mozSettings (e.g. <empty string> when using the Gaia flashing scripts: install-gaia | reset-gaia ), the MAC address doesn't get updated, since the logic introduced by bug 880617 assumes that 'deviceinfo.mac' is not set until a proper value for MAC is obtained. This way, if a value is already present in mozSettings, it is assumed to be the correct MAC address.

This bug aims to fix this regression on the patch for bug 880617.

Comment 1

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

Attachment: Pull Request #11062 - Handle invalid 'deviceinfo.mac'

Check if an invalid value is set for the MAC address under 'deviceinfo.mac' in
mozSettings, and if so, get the MAC address from the wifiManager and update
the settings.

Comment 2

[xiaokang.chen]

Dear Mihai,
Firstly, Thanks for your hardworking.

And I'm afraid your patch will not update wifi macAddress's display instantly after enable wifi.

IMHO, we should update it in the wifiManager.onenabled, may be like this:

  wifiManager.onenabled = function() {
    dispatchEvent(new CustomEvent('wifi-enabled'));
    wifiEnabled();

    var key = 'deviceinfo.mac';
    settings.addObserver(key, function(evt) {
      var rule = '[data-name="' + key + '"]';
      var labels = document.querySelectorAll(rule);
      for (var i = 0; i < labels.length; i++) {
        labels[i].textContent = evt.settingValue;
      }
    });
  };
Then if we enable wifi, we will see the update of mac address instantly. How do you think?

Comment 3

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

(In reply to xiaokang.chen from comment #2)
> Dear Mihai,
> Firstly, Thanks for your hardworking.
> 
> And I'm afraid your patch will not update wifi macAddress's display
> instantly after enable wifi.
> 
> IMHO, we should update it in the wifiManager.onenabled, may be like this:
> 
>   wifiManager.onenabled = function() {
>     dispatchEvent(new CustomEvent('wifi-enabled'));
>     wifiEnabled();
> 
>     var key = 'deviceinfo.mac';
>     settings.addObserver(key, function(evt) {
>       var rule = '[data-name="' + key + '"]';
>       var labels = document.querySelectorAll(rule);
>       for (var i = 0; i < labels.length; i++) {
>         labels[i].textContent = evt.settingValue;
>       }
>     });
>   };
> Then if we enable wifi, we will see the update of mac address instantly. How
> do you think?

Thanks for the tip xiaokang.chen, I guess that will work as well. Nevertheless, as you will notice in the code of apps/settings/js/connectivity.js, the MAC address is updated within the 'updateWifi()' function which is set as a callback for all the Wi-Fi status listeners (https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/connectivity.js#L29) and thus it is called every time the Wi-Fi state changes (i.e. enable/disable).

Comment 4

[xiaokang.chen]

Dear Mihai,
I know updateWifi will be called when wifi status changed and your code just store the macAddress to indexedDB. But didn't update the label which has attibute like this :"data-name='deviceinfo.mac'" unless the user restart the settings app.

Comment 5

[Fabien Cazenave [:kaze]]

Considering xiaokang.chen’s comment as a feedback-, holding the review.

Comment 6

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

(In reply to xiaokang.chen from comment #4)
> Dear Mihai,
> I know updateWifi will be called when wifi status changed and your code just
> store the macAddress to indexedDB. But didn't update the label which has
> attibute like this :"data-name='deviceinfo.mac'" unless the user restart the
> settings app.

For the sake of minimizing overhead and complexity, adding an observer for a setting that would change at most once (from "Not available" to actual MAC address string) is not really desirable.

The change that this patch adds ensures that the MAC address will always have the proper value when the Settings app is opened. There is no actual case in which the MAC address needs to be updated as soon as the Wi-Fi is enabled: just 'make reset-gaia' and you will see that the MAC address is shown correctly -- if you have a reproducible case in which the MAC address is not shown correctly (without restarting the Settings app) feel free to post the steps to reproduce and I will investigate it, otherwise, I don't see the reason why we should increase the complexity for something that never happens in practice.

Comment 8

[Preeti Raghunath(:Preeti)]

HI Mihai,

Have you had a chance to address comment 4?

Comment 9

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

(In reply to Preeti Raghunath(:Preeti) from comment #8)
> HI Mihai,
> 
> Have you had a chance to address comment 4?

Hi Preeti, indeed I have addressed it in comment 6: I do not believe that a listener is needed for the setting, unless there is an edge case I am missing in my logic -- for which I asked about.

Comment 10

[xiaokang.chen]

Dear Mihai,
After add your patch we will see this picture: in Settings macAddress is "not Available" while the wifi connected icon shows in the statusbar. Do you think it is right?

Comment 11

[xiaokang.chen]

The reproduce step is: 
1 Settings>wifi>enable wifi
2 choose one ap and connected with it
3 Settings>wifi>manage networks>mac address shows not available but statusbar shows wifi connected-------not ok1.
4 Settings>Device information>more information>mac address shows not available but statusbar shows wifi connected-------not ok2.

Comment 12

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

(In reply to xiaokang.chen from comment #10)
> Dear Mihai,
> After add your patch we will see this picture: in Settings macAddress is
> "not Available" while the wifi connected icon shows in the statusbar. Do you
> think it is right?

Dear xiaokang.chen,

I am very sorry but I can't reproduce this (neither following your steps from comment 11, nor forcefully setting the deviceinfo.mac setting to null in mozSettings to trigger showing "Not available" in the respective screens) after the patch posted to this bug is applied (curl https://github.com/mozilla-b2g/gaia/pull/11062.patch | git am).

I have added an extra logging commit to the patch that tracks the changes to the MAC address setting, can you please apply the patch and, after following the steps to reproduce, post the log here (get the log with: adb logcat -s "GeckoConsole"). Maybe that will clarify what happens on your build that it keeps on showing 'Not available'.

Fabien, can you also take a look at this since it is high priority leo+ and try to reproduce with my patch applied? (check the log as well)

Comment 13

[xiaokang.chen]

Dear Mihai,
Sorry, this time I can not reproduce that problem I said in comment 11.
But I have another question: When will you update the value of "deviceinfo.mac"? if a user change the macAddress manually for some special reasons. (I changed the macAddress manually for association identification/authentication, but the macAddress always show the old one while wifiManager.macAddress has been updated)

Comment 14

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

(In reply to xiaokang.chen from comment #13)
> Dear Mihai,
> Sorry, this time I can not reproduce that problem I said in comment 11.
> But I have another question: When will you update the value of
> "deviceinfo.mac"? if a user change the macAddress manually for some special
> reasons. (I changed the macAddress manually for association
> identification/authentication, but the macAddress always show the old one
> while wifiManager.macAddress has been updated)

I am not sure a user can change the macAddress manually, it is a hardware specific identification address that doesn't change upon reboot. This was the whole intuition behind the patch that introduced this logic (see bug 880617).

Comment 15

[Alex Keybl [:akeybl]]

comment 13 suggests this is not reproducible, therefore not a blocker.

Comment 16

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

(In reply to Alex Keybl [:akeybl] from comment #15)
> comment 13 suggests this is not reproducible, therefore not a blocker.

Hi Alex, I think what xiaokang.chen meant is that he can't reproduce the issue after applying my patch (see comment 10), so this is still reproducible without my patch.

Comment 17

[Preeti Raghunath(:Preeti)]

QA,

Please test to see if this patch actually fixes the bug

Comment 18

[gbennett]

Attachment: MACaddress_shown

The patch worked! :)

Environmental Variables
Device: Buri 1.2.0 w/ required patch
Build ID: 20130903040204
Gecko: http://hg.mozilla.org/mozilla-central/rev/d54e0cce6c17
Gaia: N/A
Platform Version: 26.0a1

After pushing the patch I connected to our wifi and was able to view the mac address.

Comment 19

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

Thanks for the check gbennett, requesting blocking-b2g:leo again as per comment 16

Comment 20

[Preeti Raghunath(:Preeti)]

Leo+ for uplift.

Comment 21

[William Hsu [:whsu]]

Hi, Wayne,

The helix had the same problem.
Could I have your approval to uplift this patch to V1.1.0 hd?
Thank you.

Comment 22

[Fabien Cazenave [:kaze]]

Comment on attachment 778264 [details]
Pull Request #11062 - Handle invalid 'deviceinfo.mac'

I’m afraid I still prefer Xiaokang Chen’s approach: the MAC address should be stored in the Settings database and updated in the Settings UI as soon as it becomes available (= either at startup if Wi-Fi is on, or right after Wi-Fi is activated).

As we already have a callback for wifi.onenabled, I don’t think it would increase the complexity; but I believe it would be more readable.

Preparing a quick patch.

Comment 23

[Fabien Cazenave [:kaze]]

Attachment: link to pull request

https://github.com/mozilla-b2g/gaia/pull/12134

Trying to keep the spirit of Mihai’s patch while addressing Xiaokang Chen’s concerns. Does this look OK to you, guys?

Comment 24

[xiaokang.chen]

Dear Fabien,
Thanks for your patch, I think it is ok now!

Comment 25

[Fabien Cazenave [:kaze]]

Comment on attachment 803380 [details]
link to pull request

Assuming this was an f+. :-)

Comment 26

[Wayne Chang [:wchang]]

Thanks William, leo+ covers hd.

Can you check with Master that this patch has fixed the problem on helix as well?

Comment 27

[William Hsu [:whsu]]

No problem. I will check it soon.



--- Keep the Needinfo flag to remind myself ---

Comment 28

[William Hsu [:whsu]]

I still cannot see the MAC address that display on settings page.
Have you merged the patch to master branch?
Thank you!

* Test build: Helix
 - Gaia:     a0079597d510ce8ea0b9cbb02c506030510b9eeb
 - Gecko:    http://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
 - BuildID   20130916040205
 - Version   26.0a1


* Test Build: Unagi
 - Gaia:     9cb0bf07dd6b8c23ee4c4db3d866464077f81e45
 - Gecko:    afc0d277a978e95a9c74d1daa41af0bd15c4d6e6
 - BuildID   20130916060051
 - Version   26.0a1

Comment 29

[William Hsu [:whsu]]

Attachment: MAC address

Comment 30

[Preeti Raghunath(:Preeti)]

William,

testing on 26, will not fix it in leo. Please test on b2g-18.

Comment 31

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

Comment on attachment 803380 [details]
link to pull request

Thanks Fabien for providing an alternate solution to this bug.

The patch looks fine and it seems to solve the problem (f+ for this), however, as I detailed in comment 6 and comment 12, I could not find any actual edge case that would require the extra checks for the MAC address every time the WiFi is enabled.

Comment 32

[Fabien Cazenave [:kaze]]

Thanks for the feedback Mihai.

(In reply to William Hsu [:whsu] from comment #28)
> I still cannot see the MAC address that display on settings page.
> Have you merged the patch to master branch?

William, the patch is not merged yet: it’s still awaiting Arthur’s review.

(In reply to Mihai Cirlanaru [:mihai][:mcirlanaru] from comment #31)
> however, as I detailed in comment 6 and comment 12, I could not find any
> actual edge case that would require the extra checks for the MAC address
> every time the WiFi is enabled.

Me neither, and I tried really hard to find such an edge case!
However, we should not be afraid to make the code /too/ robust. I

Comment 33

[Arthur Chen [:arthurcc][inactive after 6/30]]

Kaze, thanks for the patch. It works well. And please check the question I left in github.

Comment 34

[Preeti Raghunath(:Preeti)]

Kaze,

Please check review comments so the uplift can happen ASAP.

Comment 35

[William Hsu [:whsu]]

Removing the "Needinfo". You can added it back if you need QA to verify the patch.
Thank you!

Comment 36

[Francis Lee [:frlee]]

hi Kaze,

may you please clarify the concern raised in comment33 ? per talk with arthur, once his question is clarified, this patch should be good to go.

Comment 37

[Fabien Cazenave [:kaze]]

Merged on master: https://github.com/mozilla-b2g/gaia/commit/c0dc159a1b0b746053c2ca8d6405464b40b8dbab

Comment 38

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted c0dc159a1b0b746053c2ca8d6405464b40b8dbab to:
v1-train: d9c6b0c3384625e467f892c9dafbc40cfe132180
v1.2: c8f20af37a3ca4742685ff44226fc061178fe5de

Comment 39

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

v1.1.0hd: d9c6b0c3384625e467f892c9dafbc40cfe132180