1157400 - Windows XP Wifi scanning not implemented

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[:garvan]

This bug is to document the investigation of lack of WiFi AP based geolocation lookups on Windows XP.

Known problems:
1) Under XP Home VM (on a mac host), Firefox will report not wifi APs, and other windows applications *do* report correctly. I tested Nightly, and Releases 38, 37, and 25. (After that my time locked XP VM expired.)

2) The one and done task for this investigation (https://oneanddone.mozilla.org/en-US/tasks/92/) resulted in one successful report of XP64 scanning and one of XP home failing to scan:
- success XP64: bug 1147897 comment 9
- failure XP home: bug 1147897 comment 11

3) Telemetry for wifi AP geolocation lookups shows no indication that non-release builds can perform this lookup (location results <200 meters approx):
- beta 35 http://mzl.la/1zIz73m fails to show this, release 35 succeeds http://mzl.la/1cXqKfx 
- beta 37 http://mzl.la/1zIAfUH (fails to show wifi geolocation), release 37 http://mzl.la/1bpiG6D (succeeds)

(I am going to be lazy and not post every link, but I continued this pattern for various releases, and didn't see anything to refute the findings.)

Comment 1

[:garvan]

Telemetry bisection: going back to when this telemetry was available (31 cycle), there is no confirmation of wifi scanning on release or beta
beta http://mzl.la/1d0YvfG
release http://mzl.la/1PjwmhS

Comment 2

[:garvan]

Aaaand there is a potential obvious explanation here, there is a timeout on the wifi scan in the geo code that I am investigating. This bug may be geo-specific after all.
(In case anyone else suddenly wanted to look at this, don't just yet)

Comment 3

[:garvan]

Regarding the previous comment, NetworkGeolocationProvider.js onChange() callback from nsWifiScanner arrives with an empty array of access points.  Timers in NetworkGeolocationProvider have no affect on this. 
Here is a 5 second wait for wifi results that should be investigated:
https://dxr.mozilla.org/mozilla-central/source/netwerk/wifi/win_wifiScanner.cpp#143

Comment 4

[:garvan]

Snippet of interesting comments from equivalent chromium code that highlights the possible problems with XP wifi scanning: 
https://github.com/adobe/chromium/blob/master/content/browser/geolocation/wifi_data_provider_win.cc

// Windows Vista uses the Native Wifi (WLAN) API for accessing WiFi cards. Windows XP Service Pack 3 (and Windows XP Service Pack 2, if upgraded with a hot fix)
// also support a limited version of the WLAN API.
// Windows XP from Service Pack 2 onwards supports the Wireless Zero
// Configuration (WZC) programming interface.
// The MSDN recommends that one use the WLAN API where available, and WZC
// otherwise.
// However, it seems that WZC fails for some wireless cards. Also, WLAN seems
// not to work on XP SP3. So we use WLAN on Vista, and use NDIS directly
// otherwise.

Comment 5

[:garvan]

The function WlanGetNetworkBssList is not supported prior to Vista:
https://dxr.mozilla.org/mozilla-central/source/netwerk/wifi/win_wifiScanner.cpp#161


https://msdn.microsoft.com/en-us/library/windows/desktop/ms706735%28v=vs.85%29.aspx
""
Requirements
Minimum supported client
	Windows Vista [desktop apps only]
""

Comment 6

[:garvan]

The changeset for the windows wifi scanning is 
https://hg.mozilla.org/mozilla-central/rev/221ded3982a7

Comment 7

[:garvan]

Hi Tim, do you know if the win wifi scanner was confirmed to work on XP at any point? It doesn't seem to be reporting any APs.

Comment 8

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

Sorry for the delay! I was unavailable for 2 weeks but forgot to update my Bugzilla name/status.

I don't believe this was tested on Windows XP; I didn't have a Windows XP machine when working on this and I don't recall whether someone else did any testing for me.

Comment 9

[:garvan]

np! and thanks for the info, I am planning to set up an XP VM for dev. and get this fixed in the next few weeks. HTML5 geolocation will only show GeoIP-level position accuracy without the wifi scan.

Comment 10

[:garvan]

Attachment: Part1: port of Chromium XP wifi scanner

This is XP wifi scanning code ported from Chromium.
https://chromium.googlesource.com/chromium/src/+/master/content/browser/geolocation/wifi_data_provider_win.cc

- kept license intact
- replaced Chromium-specific types with either platform-neutral types, or mozilla types (such as replacing Chromium's AccessPoint class with nsWifiAccessPoint)
- removed unused code, because it required additional porting
- Wrapped it in a tiny interface WindowsWifiScannerInterface so that the calling code can use it interchangeably with the non-XP scanning code (see other patch for how this is used)

Comment 11

[:garvan]

Attachment: Part2: call chromium code from gecko win wifi scanner

Comment 12

[:garvan]

Tim I am taking a guess that you are the most familiar with the win wifi scanning code -based on brief log perusal- feel free to bounce the review request.

Comment 13

[:garvan]

Doug, ni you so you read this and shout if anything seems shout-worthy.

While browsing logs, I found that 5 years ago there was XP wifi scanning code added, then pulled:
https://bugzilla.mozilla.org/show_bug.cgi?id=600235

At the time, the bug states Geo IP was considered acceptable as a geolocation result for XP users, and the patch wasn't worth the risk because it was unstable.

I didn't look into the problems in the patch in bug 600235. However, my argument for the validity of the code in this patch is that it is a port of healthy chromium code, and works quite well in my own testing.
Regarding the importance of this patch: a big portion of our userbase is on XP, and (arguably) HTML5 geolocation is more prevalent than it was 5 years ago.
Also, as we move to MLS geo, there will be closer eyes on HTML5 geolocation results -more than we have ever had- so we want the geo stack in gecko to not have gaps like this one.
The switch will be newsworthy, and we don't want hordes of users (the XP users) suddenly noticing (due to heightened awareness from media releases) that they get GeoIP results when other browsers on their system geolocate them more accurately.

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Garvan Keeley [:garvank] from comment #13)
> Regarding the importance of this patch: a big portion of our userbase is on
> XP, and (arguably) HTML5 geolocation is more prevalent than it was 5 years
> ago.

About 17% of all Firefox users run Windows XP (9% in the US).

Comment 15

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

(In reply to Garvan Keeley [:garvank] from comment #12)
> Tim I am taking a guess that you are the most familiar with the win wifi
> scanning code -based on brief log perusal- feel free to bounce the review
> request.

I might be the most familiar with that code, but I don't think I can do this review. The work I did on WiFi scanning was mostly a one-off project to help a new contributor. Also I don't have an XP machine or VM to test with and I'm not a peer of any of the areas of code that these patches touch.

The code looks reasonable to me and if you've tested it thoroughly then I'm happy to have it land. Maybe someone who worked on bug 1141775 can actually grant r+?

Comment 16

[:garvan]

Thanks Tim, I'll grab a Necko peer to review

Comment 17

[:garvan]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e78081fb749e
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e78081fb749e

Comment 18

[Patrick McManus [:mcmanus]]

Comment on attachment 8611210 [details] [diff] [review]
Part2: call chromium code from gecko win wifi scanner

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

lgtm. thanks for tihs!

::: netwerk/wifi/nsWifiScannerWin.cpp
@@ +30,5 @@
>  nsresult
>  nsWifiMonitor::DoScan()
>  {
>      if (!mWinWifiScanner) {
> +      if (IsWin2003OrLater()) {

let's LOG which one gets used

Comment 19

[Patrick McManus [:mcmanus]]

Comment on attachment 8611209 [details] [diff] [review]
Part1: port of Chromium XP wifi scanner

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

this is a bit of a rubber stamp.. I don't think we have anyone more familiar with these old windows apis or with a good test rig than you garvan :)

::: netwerk/wifi/win_xp_wifiScanner.cpp
@@ +22,5 @@
> +// otherwise.
> +
> +// MOZILLA NOTE:
> +// This code is ported from chromium:
> +// https://chromium.googlesource.com/chromium/src/+/master/content/browser/geolocation/wifi_data_provider_win.cc

can we get some kind of a cset on that, so we can track diffs in the future?

@@ +44,5 @@
> +
> +// Length for generic string buffers passed to Win32 APIs.
> +const int kStringLength = 512;
> +
> +// The time periods, in milliseconds, between successive polls of the wifi data.

the four polling interval constants appear to be unused.

Comment 20

[:garvan]

Thanks for the review, I'll update for those comments.
Rubber-stamping will be mitigated as I'll be getting QA involved in testing this.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/faacf69f83ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceac60b1409e

Comment 22

[:garvan]

Karl, adding you to cc list because everyone loves more bug mail :)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/faacf69f83ad
https://hg.mozilla.org/mozilla-central/rev/ceac60b1409e