Closed Bug 1157400 Opened 8 years ago Closed 8 years ago

Windows XP Wifi scanning not implemented


(Core :: Networking, defect)

Windows XP
Not set



Tracking Status
firefox41 --- fixed


(Reporter: garvan, Assigned: garvan)




(2 files)

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 ( 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 fails to show this, release 35 succeeds 
- beta 37 (fails to show wifi geolocation), release 37 (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.)
Telemetry bisection: going back to when this telemetry was available (31 cycle), there is no confirmation of wifi scanning on release or beta
OS: Unspecified → Windows XP
Hardware: Unspecified → x86
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)
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:
Snippet of interesting comments from equivalent chromium code that highlights the possible problems with XP wifi scanning:

// 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.
The function WlanGetNetworkBssList is not supported prior to Vista:
Minimum supported client
	Windows Vista [desktop apps only]
The changeset for the windows wifi scanning is
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.
Flags: needinfo?(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.
Flags: needinfo?(tabraldes)
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.
This is XP wifi scanning code ported from Chromium.

- 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)
Assignee: nobody → gkeeley
Attachment #8611209 - Flags: review?(tabraldes)
Attachment #8611210 - Flags: review?(tabraldes)
Attachment #8611209 - Attachment description: bug1157400_chromium.diff → Part1: port of Chromium XP wifi scanner
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.
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:

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.
Flags: needinfo?(dougt)
See Also: → 600235
(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).
Attachment #8611210 - Flags: review?(tabraldes) → feedback+
Attachment #8611209 - Flags: review?(tabraldes) → feedback+
(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+?
Thanks Tim, I'll grab a Necko peer to review
Summary: [meta bug] Windows XP Wifi Scanning is not reliably reporting wifi APs → Windows XP Wifi scanning not implemented
Attachment #8611209 - Flags: review?(mcmanus)
Attachment #8611210 - Flags: review?(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
Attachment #8611210 - Flags: review?(mcmanus) → review+
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.
> +
> +// This code is ported from chromium:
> +//

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.
Attachment #8611209 - Flags: review?(mcmanus) → review+
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.
Karl, adding you to cc list because everyone loves more bug mail :)
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Flags: needinfo?(dougt)
Depends on: 1329916
You need to log in before you can comment on or make changes to this bug.