bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

B2G Wifi scanning using wpa_supplicant to improve geolocation.

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bsurender, Assigned: bsurender)

Tracking

unspecified
mozilla18
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 12 obsolete attachments)

5.79 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.41 KB, patch
mrbkap
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
B2G wifi scanning to improve geolocation.
(Assignee)

Comment 1

6 years ago
Created attachment 663109 [details] [diff] [review]
Patch refactors existing wifi scanning in WifiWorker.js, C++ module to call refactored wifi scan is WIP.

Patch refactors existing wifi scanning in WifiWorker.js. 

The C++ module to call re-factored wifi scan is WIP.
Attachment #663109 - Flags: review?(mrbkap)
Comment on attachment 663109 [details] [diff] [review]
Patch refactors existing wifi scanning in WifiWorker.js, C++ module to call refactored wifi scan is WIP.

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

::: dom/wifi/WifiWorker.js
@@ +13,5 @@
>  
>  const DEBUG = false; // set to true to show debug messages
>  
> +const WIFISCANRESULT_CONTRACTID = "@mozilla.org/wifi/wifiscanresult;1";
> +const WIFISCANRESULT_CID        = Components.ID("{9fb11700-01f3-11e2-a21f-0800200c9a66}");

You don't need either a contractid or a cid. Nobody else should be creating these things for the moment.

@@ +1358,5 @@
>    httpProxyPort: null,
>  
>  };
>  
> +let WifiScanResultInterface = {

This is unused and unnecessary.

@@ +1379,5 @@
> +                                    contractID: WIFISCANRESULT_CONTRACTID,
> +                                    classDescription: "WifiScanResult",
> +                                    interfaces: [Ci.nsIWifiScanResult]}),
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWifiScanResult])

If you really want a QI implementation, you can keep this. However, it's mostly unnecessary. The rest of this stuff (the classid and classinfo) isn't necessary.

@@ +2062,5 @@
>        this._sendMessage(message, false, "ScanFailed", msg);
>      }).bind(this));
>    },
>  
> +  _processResults: function(results, self, wifiScanResults) {

This is totally unnecessary. You should be able to use the existing waitForScan machinery to parse the scan results instead of mostly duplicating the code here.

@@ +2075,5 @@
> +        r.bssid = match[1];
> +
> +        var capabilities = getKeyManagement(match[4]);
> +        if (capabilities.length) {
> +          if (capabilities[0] === "WPA-PSK") {

Even in the corrected code, please don't ignore capabilities beyond the first one.

@@ +2080,5 @@
> +            r.capabilities = Ci.nsIWifiScanResult.WPA2_PSK;
> +          } else if (capabilities[0] === "WEP") {
> +            r.capabilities = Ci.nsIWifiScanResult.WEP;
> +          } else {
> +            r.capabilities = -1;

Why -1? Seems like this should be 0.

@@ +2096,5 @@
> +  },
> +
> +  getWifiScanResults: function(callback) {
> +    var count = 0;
> +    var timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

It should be extremely rare that we need this, so I'd avoid creating it until the first time the scan command fails.

@@ +2102,5 @@
> +
> +    function doScan(thisObj, callback) {
> +      WifiManager.scan(true, (function(ok) {
> +        if (ok) {
> +          WifiManager.getScanResults((function(results) {

This is not correct, either. The SCAN command asks the supplicant to perform a scan and returns immediately. This is why there's a waitForScan internal API. You need to wait for the scan to complete and the supplicant to tell us that the new scan results are available.

@@ +2105,5 @@
> +        if (ok) {
> +          WifiManager.getScanResults((function(results) {
> +            if (results) {
> +              var wifiScanResults = new Array();
> +              thisObj._processResults(results, this, wifiScanResults);

Using the new API will change this a bit... FWIW, the easiest way to map from the internal version to the WifiScanResult interface would be to use Array.prototype.map.

@@ +2109,5 @@
> +              thisObj._processResults(results, this, wifiScanResults);
> +
> +              // Count set to 4 indicates obtaining scanning results was
> +              // successfull so do not need to retry scanning.
> +              count = 4;

I don't think this does what you think it does... This code doesn't flow into the count++ code below, as it's in its own function that runs off of a callback.

@@ +2110,5 @@
> +
> +              // Count set to 4 indicates obtaining scanning results was
> +              // successfull so do not need to retry scanning.
> +              count = 4;
> +              if (callback) {

No need to null-check callback -- if someone passes null they deserve the JS exception that they'll get.

@@ +2129,5 @@
> +        }
> +
> +        // Else it's still running, continue waiting.
> +        timer.initWithCallback(doScan(thisObj), 10000, Ci.nsITimer.TYPE_ONE_SHOT);
> +          }).bind(thisObj));

Looks like the indentation is off here.

::: dom/wifi/nsIWifi.idl
@@ +26,5 @@
> +    readonly attribute DOMString SSID;
> +    readonly attribute DOMString BSSID;
> +
> +    const int32_t WPA2_PSK = 1;
> +    const int32_t WEP = 2;

These constants need to include all of the possibilities found at <http://hg.mozilla.org/mozilla-central/file/5dde4b86acfb/dom/wifi/WifiWorker.js#l1234>, which is to say they should be WPA_PSK, WEP, and WPA_EAP. Also, please make them flags, so 0x01, 0x02, and 0x04.

@@ +77,5 @@
> +     *
> +     * On success a callback is notified with the list of networks.
> +     * On failure after 3 scan retry attempts a callback is notified of failure.
> +     */
> +    void getWifiScanResults(in nsIWifiScanResultsReady callback);

Need to bump the IID here.
Attachment #663109 - Flags: review?(mrbkap) → review-
(Assignee)

Comment 3

6 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #2)
> Comment on attachment 663109 [details] [diff] [review]
> Patch refactors existing wifi scanning in WifiWorker.js, C++ module to call
> refactored wifi scan is WIP.
> 
> Review of attachment 663109 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +2105,5 @@
> > +        if (ok) {
> > +          WifiManager.getScanResults((function(results) {
> > +            if (results) {
> > +              var wifiScanResults = new Array();
> > +              thisObj._processResults(results, this, wifiScanResults);
> 
> Using the new API will change this a bit... FWIW, the easiest way to map
> from the internal version to the WifiScanResult interface would be to use
> Array.prototype.map.
> 

After completing the review comments and trying a few things out, I don't think that there is a way around having to parse the results as done in the current patch. Currently in the new patch waitForScan calls the callback when the requested scan has completed, but without an associative array of scan results. Instead, after I waitForScan for the scan command to succeed and receive a callback that the results are available, I have to get the scan results with a callback that receives a string of results. In this string of results each access point info is separated by "\n". 

I could merge parsing with the existing parsing function in WifiManager...
(In reply to bsurender from comment #3)
> After completing the review comments and trying a few things out, I don't
> think that there is a way around having to parse the results as done in the
> current patch. Currently in the new patch waitForScan calls the callback
> when the requested scan has completed, but without an associative array of
> scan results.

Can you post your new patch, please?
(Assignee)

Comment 5

6 years ago
Created attachment 663516 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning. WIP.

This patch contains the changes. Changes 2062 and 2105 need to be resolved wrt comment 3 and 4.
Attachment #663109 - Attachment is obsolete: true
Comment on attachment 663516 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning. WIP.

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

::: dom/wifi/WifiWorker.js
@@ +2101,5 @@
> +          RetryScan();
> +          return;
> +        }
> +
> +        let callback = (function (networks) {

What is the value of networks when this function gets called?
(Assignee)

Comment 7

6 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #6)
> Comment on attachment 663516 [details] [diff] [review]
> Bug 792952 - B2G Wifi scanning. WIP.
> 
> Review of attachment 663516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/WifiWorker.js
> @@ +2101,5 @@
> > +          RetryScan();
> > +          return;
> > +        }
> > +
> > +        let callback = (function (networks) {
> 
> What is the value of networks when this function gets called?

networks is not null when I do a null check. When I enumerate through it to get the object name and object property it has nothing. How do I access the data in networks?
(Assignee)

Comment 8

6 years ago
(In reply to bsurender from comment #7)
> (In reply to Blake Kaplan (:mrbkap) from comment #6)
> > Comment on attachment 663516 [details] [diff] [review]
> > Bug 792952 - B2G Wifi scanning. WIP.
> > 
> > Review of attachment 663516 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/wifi/WifiWorker.js
> > @@ +2101,5 @@
> > > +          RetryScan();
> > > +          return;
> > > +        }
> > > +
> > > +        let callback = (function (networks) {
> > 
> > What is the value of networks when this function gets called?
> 
> networks is not null when I do a null check. When I enumerate through it to
> get the object name and object *value it has nothing. How do I access the
> data in networks?
If you do debug(uneval(networks)) what gets printed to logcat?
(Assignee)

Comment 10

6 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> If you do debug(uneval(networks)) what gets printed to logcat?

I prints "null". 

Got it. Getting the object name and property well 'object Object' now.
(Assignee)

Comment 11

6 years ago
Created attachment 663719 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.WIP
Attachment #663516 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Created attachment 663720 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning
Attachment #663719 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #663720 - Attachment description: Bug 792952 - B2G Wifi scanning. WIP. → Bug 792952 - B2G Wifi scanning
Attachment #663720 - Flags: review?(mrbkap)
Comment on attachment 663720 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning

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

Closer!

::: dom/wifi/WifiWorker.js
@@ +2053,5 @@
> +        return;
> +      }
> +
> +      // Else it's still running, continue waiting.
> +      timer.initWithCallback(doScan(this), 10000, Ci.nsITimer.TYPE_ONE_SHOT);

I think you mean doScan.bind(this, callback) here. As it is, this should throw as it'll call doScan instead of setting it as the callback for the timer (and then trying to set undefined as the callback).

@@ +2057,5 @@
> +      timer.initWithCallback(doScan(this), 10000, Ci.nsITimer.TYPE_ONE_SHOT);
> +    }
> +
> +    doScan(this);
> +    function doScan(thisObj, callback) {

Since you use .bind below, there's no need to take thisObj as a parameter. You can simply use 'this'.

@@ +2072,5 @@
> +          }
> +
> +          function tranformResult(element) {
> +            var result = new WifiScanResult();
> +            result.SSID = element['ssid'];

As we talked about in person, if we make the ssid and bssid lowercase in the IDL it'll make the two versions of the objects more similar. It'll also make sense to do:

for (let id in element) {
  if (id === "__exposedProps__) continue;
  if (id === "capabilities") ...;
  else result[id] = element[id];
}

which avoids having the full list of properties on ScanResults in another place.

@@ +2099,5 @@
> +          var key, value;
> +          var results = new Array();
> +          for (key in networks) {
> +            value = networks[key];
> +            results.push(value);

My bad. As we talked about in person, you can simply push the transformed value and not use Array.prototype.map.

::: dom/wifi/nsIWifi.idl
@@ +36,5 @@
> +     */
> +    readonly attribute uint32_t signalStrength;
> +
> +    /**
> +     * Strength of the signal to network, in dBm between -55 and -100 dBm.

This comment isn't correct.
Attachment #663720 - Flags: review?(mrbkap) → review-
(Assignee)

Comment 14

6 years ago
Created attachment 664553 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.
Attachment #663720 - Attachment is obsolete: true
(Assignee)

Comment 15

6 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #13)
> Comment on attachment 663720 [details] [diff] [review]
> Bug 792952 - B2G Wifi scanning
> 
> Review of attachment 663720 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +2057,5 @@
> > +      timer.initWithCallback(doScan(this), 10000, Ci.nsITimer.TYPE_ONE_SHOT);
> > +    }
> > +
> > +    doScan(this);
> > +    function doScan(thisObj, callback) {
> 
> Since you use .bind below, there's no need to take thisObj as a parameter.
> You can simply use 'this'.
> 

I still seem to need to pass the 'thisObj' to 'doScan' because it is needed to bind the 'waitForScanCallback' function.(In reply to bsurender from comment #14)
> Created attachment 664553 [details] [diff] [review]
> Bug 792952 - B2G Wifi scanning. WIP.
(Assignee)

Updated

6 years ago
Attachment #664553 - Attachment description: Bug 792952 - B2G Wifi scanning. WIP. → Bug 792952 - B2G Wifi scanning.
Attachment #664553 - Flags: review?(mrbkap)
Comment on attachment 664553 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.

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

::: dom/wifi/WifiWorker.js
@@ +2057,5 @@
> +      timer.initWithCallback(doScan.bind(this, callback),
> +                             10000, Ci.nsITimer.TYPE_ONE_SHOT);
> +    }
> +
> +    doScan(this);

Hmm, if we're going to do this, just do:

var self = this;

and then use self instead of this inside. That'll allow you to get rid of the .bind()s as well.

@@ +2074,5 @@
> +
> +          function transformResult(element) {
> +            var result = new WifiScanResult();
> +            for (let id in element) {
> +              if (id === "__exposedProps__") continue;

Nit: The continue on its own line, please.

@@ +2096,5 @@
> +            }
> +
> +            if (result['bssid']) {
> +              result['connected'] =
> +                (result['bssid'] === WifiManager.connectionInfo.bssid);

This shouldn't be necessary. The scan-result-parsing code sets connected on the proper ScanResult already. Also obj['foo'] is kind of ugly and should be writen as obj.foo (the two are exactly equivalent, to the point where the JS parser actually changes the former into the latter).

@@ +2106,5 @@
> +          var wifiScanResults = new Array();
> +          for (key in networks) {
> +            value = networks[key];
> +            var result = transformResult(value);
> +            wifiScanResults.push(result);

There are a couple of single-use variables here. Also, this is a great opportunity to use the one useful bit of E4X! I'd prefer:

for each (let net in networks) {
  wifiScanResults.push(transformResult(net));
}

::: dom/wifi/nsIWifi.idl
@@ +36,5 @@
> +     */
> +    readonly attribute uint32_t signalStrength;
> +
> +    /**
> +     * Signal strength represented as a non-negative, 8-bit integer.

This comment doesn't add much value.
Attachment #664553 - Flags: review?(mrbkap)
(Assignee)

Comment 17

6 years ago
Created attachment 665213 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning. WIP.
Attachment #664553 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
Created attachment 665214 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.
Attachment #665213 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #16)
> Comment on attachment 664553 [details] [diff] [review]
> Bug 792952 - B2G Wifi scanning.
> 
> Review of attachment 664553 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/WifiWorker.js
> @@ +2106,5 @@
> > +          var wifiScanResults = new Array();
> > +          for (key in networks) {
> > +            value = networks[key];
> > +            var result = transformResult(value);
> > +            wifiScanResults.push(result);
> 
> There are a couple of single-use variables here. Also, this is a great
> opportunity to use the one useful bit of E4X! I'd prefer:
> 
> for each (let net in networks) {
>   wifiScanResults.push(transformResult(net));
> }

I still had to get the value from networks using the key. Please see attached patch lines 2099, 2100. The above and other variations did not work.

I not so sure if you were referring to Javascript E4X here https://developer.mozilla.org/en-US/docs/E4X 

But if that's what you were referring to then there is a warning that says "E4X is deprecated. It will be disabled by default for content in Firefox 16, disabled by default for chrome in Firefox 17, and removed in Firefox 18. Use DOMParser/DOMSerializer or a non-native JXON algorithm instead."
(Assignee)

Updated

6 years ago
Attachment #665214 - Attachment description: Bug 792952 - B2G Wifi scanning. WIP. → Bug 792952 - B2G Wifi scanning.
Attachment #665214 - Flags: review?(mrbkap)
Comment on attachment 665214 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.

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

::: dom/wifi/WifiWorker.js
@@ +2050,5 @@
> +      }
> +
> +      if (count++ >= 3) {
> +        timer = null;
> +        callback.onfailure();

So, I realized that you have to remove the callback from this.wantScanResults here (as we do in the error case above) and that you only want to add it one time, not every time we try to re-scan.

@@ +2055,5 @@
> +        return;
> +      }
> +
> +      // Else it's still running, continue waiting.
> +      timer.initWithCallback(doScan(), 10000, Ci.nsITimer.TYPE_ONE_SHOT);

The call to doScan is still wrong here. You want to pass it as a callback not call it.
Attachment #665214 - Flags: review?(mrbkap)
(Assignee)

Comment 21

6 years ago
Created attachment 665754 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.
Attachment #665214 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
Created attachment 665755 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning. Contains only the diff of the latest changes.

This patch contains only the diff of the latest changes for the reviewer's convenience.
(Assignee)

Comment 23

6 years ago
Comment on attachment 665754 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.

This is the entire patch. A patch with just the diff of the latest changes has also been uploaded for your convenience. Indicated in patch title.
Attachment #665754 - Attachment description: Bug 792952 - B2G Wifi scanning. WIP. → Bug 792952 - B2G Wifi scanning.
Attachment #665754 - Flags: review?(mrbkap)
(Assignee)

Updated

6 years ago
Attachment #665755 - Attachment description: WIP. Bug 792952 - B2G Wifi scanning. Contains only the diff of the latest changes. → Bug 792952 - B2G Wifi scanning. Contains only the diff of the latest changes.
(Assignee)

Updated

6 years ago
Attachment #665754 - Flags: review?(mrbkap)
(Assignee)

Comment 24

6 years ago
Created attachment 666029 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning. WIP.
Attachment #665754 - Attachment is obsolete: true
Attachment #665755 - Attachment is obsolete: true
(Assignee)

Comment 25

6 years ago
Created attachment 666030 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.
Attachment #666029 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #666030 - Attachment description: Bug 792952 - B2G Wifi scanning. WIP. → Bug 792952 - B2G Wifi scanning.
Attachment #666030 - Flags: review?(mrbkap)
Comment on attachment 666030 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.

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

Getting really close!

::: dom/wifi/WifiWorker.js
@@ +2045,5 @@
> +
> +    self.waitForScan(waitForScanCallback);
> +    doScan();
> +    function doScan() {
> +      WifiManager.scan(true, handleScan);

Why not: WifiManager.scan(true, function(ok) { ...
?

@@ +2056,5 @@
> +    }
> +
> +    function waitForScanCallback(networks) {
> +      if (networks === null) {
> +        RetryScan();

This should actually probably call callback.onfailure(). This isn't a simple failure to get the scan results, but we actually failed to get the existing scan results. I think this change would let you merge doScan and RetryScan into a single function (scan) since then RetryScan will only be used in one place.

@@ +2062,5 @@
> +      }
> +
> +      var wifiScanResults = new Array();
> +      var net;
> +      for (net in networks) {

for (let net in networks)

@@ +2105,5 @@
> +      }
> +
> +      if (count++ >= 3) {
> +        timer = null;
> +        this.wantScanResults.splice(this.wantScanResults.indexOf(callback), 1);

|callback| here should be waitForScanCallback.
Attachment #666030 - Flags: review?(mrbkap)
(Assignee)

Comment 27

6 years ago
Created attachment 666117 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.
Attachment #666030 - Attachment is obsolete: true
(Assignee)

Comment 28

6 years ago
Created attachment 666119 [details] [diff] [review]
Bug 792952, B2G wifi scanning. diff of latest changes only.

This patch is just a diff of the latest changes for the reviewer's convenience.
(Assignee)

Updated

6 years ago
Attachment #666117 - Attachment description: Bug 792952 - B2G Wifi scanning. WIP. → Bug 792952 - B2G Wifi scanning.
Attachment #666117 - Flags: review?(mrbkap)
Comment on attachment 666117 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning.

Looks pretty good. Let's get this in.

It looks like you're going to have to rebase the .idl changes on top of some other stuff that landed recently, though.
Attachment #666117 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 30

6 years ago
Created attachment 666787 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning js. Rebased patch.
Attachment #666119 - Attachment is obsolete: true
(Assignee)

Comment 31

6 years ago
Comment on attachment 666787 [details] [diff] [review]
Bug 792952 - B2G Wifi scanning js. Rebased patch.

I forgot to remove the WIP tag. This is the rebased patch. I've uploaded it for building on try. Once the builds pass, will upload for all tests and then paste a link here.
Attachment #666787 - Attachment description: Bug 792952 - B2G Wifi scanning js. Rebased patch. WIP → Bug 792952 - B2G Wifi scanning js. Rebased patch.
(Assignee)

Comment 32

6 years ago
(In reply to bsurender from comment #31)
> Comment on attachment 666787 [details] [diff] [review]
> Bug 792952 - B2G Wifi scanning js. Rebased patch.
> 
> I forgot to remove the WIP tag. This is the rebased patch. I've uploaded it
> for building on try. Once the builds pass, will upload for all tests and
> then paste a link here.

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

Looks good. Just waiting on a Rev3 WINNT 6.1 try opt test reftest retest.
https://hg.mozilla.org/mozilla-central/rev/5d0e0a533775
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

6 years ago
Blocks: 778235
You need to log in before you can comment on or make changes to this bug.