985970 - nsWifiMonitor does not perform an actual scan on Windows

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Wesley Hardman]

While playing with the Mozilla Geolocation service, I noticed that Firefox was only sending 2 APs to the service when there should have been many available.  After looking through the code I found that, on Windows, nsWifiMonitor does not perform a scan, it only calls WlanGetNetworkBssList, which only returns the APs that the device happens to notice at that time.

Advantages to scanning:
1. More accurate.
2. Will include more APs.  This is more useful in a residential setting where there is only one strong AP and a few weaker APs.
    I noticed the issue to begin with because Mozilla's Geolocation requires 3 APs, instead of 2.

Disadvantages:
1. Takes time.  The scan take 5-10 seconds to complete.
2. Scanning will usually consume more power.
3. I believe it also has to pause sending/receiving to scan (though only for milliseconds at a time).

Comment 1

[Doug Turner (:dougt)]

yeah, we to fix this.  Wesley, you interested?

Comment 2

[Wesley Hardman]

(In reply to Doug Turner (:dougt) from comment #1)
> we to fix this?

Want?

I'll take a crack at it.  C++ isn't my forte though.

Comment 3

[Doug Turner (:dougt)]

yes, we want to fix this. :)

Awesome -- let me know if you need any help.  #developers on irc.mozilla.org too can help!

Comment 4

[Wesley Hardman]

Attachment: Bug 985970 - nsWifiMonitor does not perform an actual scan on Windows

WIP Patch

I wanted to see if this approach look fine.  Right now it does not build.  

If the approach looks fine, I have whats probably a newbie question:
How should I go about setting up the callback (ScanComplete)?

As is, it will break at compile with 'ScanComplete' undeclared identifier.  If I add ScanComplete to nsWifiMonitor.h (and move the necessary includes), I get 'unresolved external symbol _WlanFreeMemory' (and the same for all of the wlanapi methods).

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8396441 [details] [diff] [review]
Bug 985970 - nsWifiMonitor does not perform an actual scan on Windows

Hi Wesley! Thanks for contributing!

When uploading patches, please check the "patch" checkbox. If you do, then bugzilla will provide additional helpful functionality like a "diff view"

I'll provide feedback momentarily

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8396441 [details] [diff] [review]
Bug 985970 - nsWifiMonitor does not perform an actual scan on Windows

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

(In reply to Wesley Hardman from comment #4)
> Created attachment 8396441 [details] [diff] [review]
> Bug 985970 - nsWifiMonitor does not perform an actual scan on Windows
> 
> WIP Patch
> 
> I wanted to see if this approach look fine.  Right now it does not build.  

This does look like the right approach!

> If the approach looks fine, I have whats probably a newbie question:
> How should I go about setting up the callback (ScanComplete)?
> 
> As is, it will break at compile with 'ScanComplete' undeclared identifier. 
> If I add ScanComplete to nsWifiMonitor.h (and move the necessary includes),
> I get 'unresolved external symbol _WlanFreeMemory' (and the same for all of
> the wlanapi methods).

I believe you're getting the undeclared identifier simply because `ScanComplete` is declared below the place it is used. Try moving the definition of `ScanComplete` to the top of the file. Alternatively you could add a "forward declaration" for `ScanComplete` at the top of the file:
  void ScanComplete(PWLAN_NOTIFICATION_DATA data,PVOID context);


General comments:
  - Your editor seems to be indenting by 4 spaces. Please configure it to indent by 2 spaces. See [1] for more info on Mozilla Coding Style
  - You're not intending to make `ScanComplete`, `wlan_handle`, `lastAccessPoints`, or `accessPoints` available outside of this file, so please make them `static`
  - In `ScanComplete`, you'll need to check that the `NotificationCode` member of the `PWLAN_NOTIFICATION_DATA` argument you got passed is equal to `wlan_notification_acm_scan_complete` (see [2])
  - Looks good overall! Thanks again for contributing!

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
[2] http://msdn.microsoft.com/en-us/library/windows/desktop/ms706902%28v=vs.85%29.aspx

Comment 7

[Wesley Hardman]

Attachment: Bug 985970 - nsWifiMonitor does not perform an actual scan on Windows

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
> > If the approach looks fine, I have whats probably a newbie question:
> > How should I go about setting up the callback (ScanComplete)?
> > 
> > As is, it will break at compile with 'ScanComplete' undeclared identifier. 
> > If I add ScanComplete to nsWifiMonitor.h (and move the necessary includes),
> > I get 'unresolved external symbol _WlanFreeMemory' (and the same for all of
> > the wlanapi methods).
> 
> I believe you're getting the undeclared identifier simply because
> `ScanComplete` is declared below the place it is used. Try moving the
> definition of `ScanComplete` to the top of the file. Alternatively you could
> add a "forward declaration" for `ScanComplete` at the top of the file:
>   void ScanComplete(PWLAN_NOTIFICATION_DATA data,PVOID context);
Ah, order matters in C++.

> 
> 
> General comments:
>   - Your editor seems to be indenting by 4 spaces. Please configure it to
> indent by 2 spaces. See [1] for more info on Mozilla Coding Style
That's what I get for using Visual Studio.
nsWifiScannerWin.cpp has an initial 4 space indent, followed by 2 space indents.  I kept the initial 4 space indent, but can change the file if you want.

>   - You're not intending to make `ScanComplete`, `wlan_handle`,
> `lastAccessPoints`, or `accessPoints` available outside of this file, so
> please make them `static`
>   - In `ScanComplete`, you'll need to check that the `NotificationCode`
> member of the `PWLAN_NOTIFICATION_DATA` argument you got passed is equal to
> `wlan_notification_acm_scan_complete` (see [2])
Do we really want that?  If the scan fails, we still want to get the list of APs that the interface sees even if the scan didn't succeed.  The behaviour in this patch is the same as before, except we are telling the interface to scan first and we are waiting for the scan before checking the list.

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 8398105 [details] [diff] [review]
Bug 985970 - nsWifiMonitor does not perform an actual scan on Windows

lgtm - Awesome work Wesley.

Tim, feel free to review.

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch

Hey Wesley, great work on the patch! I hope you don't mind me hijacking it, but I realized that some aspects of using the WLAN API are more complicated than I had thought at first, and it would probably be more helpful for me to show in code where the complexities lie rather than to try to explain them in text. That being said, here's a laundry list of some of the changes I made in this attached patch and the reasoning behind them.

Whitepsace/misc:
  Mixed-indentation files are a bummer :( The rule of thumb is to do whatever the file already does but since this file is already mixed, I've just used two-space indentation.

WinWifiScanner class:
  `nsWifiMonitor` is declared in netwerk/wifi/nsWifiMonitor.h and has most of its definition in network/wifi/nsWifiMonitor.cpp. The function `nsWifiMonitor::DoScan` is defined/implemented in the platform-specific files nsWifiScannerWin.cpp, nsWifiScannerMac.cpp, and nsWifiScannerSolaris.cpp. I noticed that a lot of the code in these various implementations of `nsWifiMonitor::DoScan` is actually identical, and some of the more generic code was getting tangled with the Windows-specific code. I moved the Windows-specific bits to a class called `WinWifiScanner`. Now nsWifiScannerWin.cpp more closely matches nsWifiScannerMac.cpp and hopefully in the future we can coalesce them further.

WinWLANLibrary class (for managing the lifetime of "Wlanapi.dll" within our process):
  Background from the documentation for LoadLibrary [2]: "The system maintains a per-process reference count on all loaded modules. Calling LoadLibrary increments the reference count. Calling the FreeLibrary or FreeLibraryAndExitThread function decrements the reference count. The system unloads a module when its reference count reaches zero or when the process terminates (regardless of the reference count)."

  The existing code calls `LoadLibrary` every time `nsWifiMonitor::DoScan` is called. It also fails to ever call `FreeLibrary`. This means 2 things:
  1. Once we load "Wlanapi.dll" we're going to keep it loaded until the process exits, regardless of whether it is still used
  2. We will increment our reference count meaninglessly and gratuitously every time `nsWifiMonitor::DoScan` is called

  Caching the handle to the library helps us avoid the meaningless and gratuitous reference count incrementing, but we must also call `FreeLibrary` or we're still going to have "Wlanapi.dll" loaded for the entire lifetime of our application.

  One way to make our application load and unload "Wlanapi.dll" at sensible times would be to make each instance of `nsWifiMonitor` hold on to a handle obtained from `LoadLibrary` and to call `FreeLibrary` on that handle in the `nsWifiMonitor` destructor.

  Because of the way `nsWifiMonitor` is written, making changes to its contructor/destructor affects cross-platform code, so instead I've created a `WinWLANLibrary` class that encapsulates our usage of the Windows WLAN library.

ScopedWLANObject class:
  It's easy to forget to call `WlanCloseHandle` on the handle we obtain from `WlanOpenHandle`, especially in early-return cases. To avoid this common pitfall, I've created the `ScopedWLANObject` class for automatically calling `WlanCloseHandle` when a WLAN object goes out of scope.

Threading issues:
  Once we call `WlanRegisterNotification`, our callback will be called for all sorts of notifications, and will be called on different threads. We call `WlanScan` for each interface on the machine, and we want our main thread to wait until our callback has been called for all of the interfaces (or a reasonable timeout). To accomplish this, I've made it so that our callback function atomically decrements a variable (representing the number of interfaces left to scan), and when that variable reaches 0 we trigger an Event [3] that notifies the main thread that it is OK to proceed.

Checking the `data` parameter in our interface scanning callback:
  As part of this change, we register to receive notifications whose source is `WLAN_NOTIFICATION_SOURCE_ACM`. There are a ton of notifications we might receive, and almost none of them have to do with whether the scan has ended or not. Specifically, we could be receiving any of the `wlan_notification_acm_*` values at [1]. We don't want to be calling ReadScanResult for any of those notifications except `wlan_notification_acm_scan_complete` and `wlan_notification_acm_scan_fail`. Thus, we check for those values.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/ms706902%28v=vs.85%29.aspx
[2] http://msdn.microsoft.com/en-us/library/windows/desktop/ms684175%28v=vs.85%29.aspx
[3] http://msdn.microsoft.com/en-us/library/windows/desktop/ms682396%28v=vs.85%29.aspx

Comment 10

[Wesley Hardman]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #9)

> Hey Wesley, great work on the patch! I hope you don't mind me hijacking it,
> but I realized that some aspects of using the WLAN API are more complicated
> than I had thought at first, and it would probably be more helpful for me to
> show in code where the complexities lie rather than to try to explain them
> in text. That being said, here's a laundry list of some of the changes I
> made in this attached patch and the reasoning behind them.

Thanks.  By all means go ahead.  As I said previously, I am not overly familiar with C++.  A lot of the things you mentioned are things that I am not used to dealing with in managed code.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 8401098 [details] [diff] [review]
Patch

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

Nicely implemented. Thanks!

::: netwerk/wifi/win_wifiScanner.cpp
@@ +13,5 @@
> +public:
> +  InterfaceScanCallbackData(uint32_t numInterfaces)
> +    : mCurrentlyScanningInterfaces(numInterfaces)
> +  {
> +    mAllInterfacesDoneScanningEvent =

Lets assert when this fails.

@@ +29,5 @@
> +  void
> +  OnInterfaceScanComplete()
> +  {
> +    uint32_t val = ::InterlockedDecrement(&mCurrentlyScanningInterfaces);
> +    if (val == 0) {

nit - !val

@@ +54,5 @@
> +    return;
> +  }
> +
> +  if (wlan_notification_acm_scan_complete != data->NotificationCode
> +      && wlan_notification_acm_scan_fail != data->NotificationCode) {

nit - put && on the previous line.

@@ +72,5 @@
> +  // and make our assumption incorrect. We opt to avoid making a bunch of
> +  // spurious LoadLibrary calls in the common case rather than load the
> +  // WLAN API in the edge case.
> +  mWlanLibrary = WinWLANLibrary::Load();
> +  // TODO: Maybe log if couldn't load library

how about an assert instead?

@@ +96,5 @@
> +  if (ERROR_SUCCESS !=
> +      (*mWlanLibrary->GetWlanEnumInterfacesPtr())(mWlanLibrary->GetWLANHandle(),
> +                                                  nullptr,
> +                                                  &interface_list)) {
> +    // TODO: Log

if you're going to nspr log these failures, then lets add that, otherwise lets just skip adding todo comments.

@@ +103,5 @@
> +
> +  // This ensures we call WlanFreeMemory on interface_list
> +  ScopedWLANObject scopedInterfaceList(mWlanLibrary, interface_list);
> +
> +  if (0 == interface_list->dwNumberOfItems) {

nit - !interface_list->dwNumberOfItems

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

Review comments addressed.

dougt - I haven't landed this because I realized that :jimm can't actually technically give review for the file that this patch touches. You're the owner for netwerk/wifi so I've marked you r?

Comment 13

[Doug Turner (:dougt)]

jimm can review this.

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/221ded3982a7

Comment 15

[Mike Hommey [:glandium]]

Fixup for non-unified builds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e7ef3f7aa5

Comment 16

[Mike Hommey [:glandium]]

And another fixup.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8061b23d2a4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/221ded3982a7
https://hg.mozilla.org/mozilla-central/rev/92e7ef3f7aa5
https://hg.mozilla.org/mozilla-central/rev/b8061b23d2a4