Closed Bug 479898 Opened 11 years ago Closed 11 years ago

support for WiFi access point scanning

Categories

(Core :: Networking, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
fennec 1.0b2+ ---

People

(Reporter: dougt, Assigned: dougt)

References

Details

(Keywords: dev-doc-complete, fixed1.9.1)

Attachments

(2 files, 8 obsolete files)

Having the platform be able to scan for available wifi access points would allow us to implement WIFI -> GeoLocation.

Basically we would need a service that could be started and stopped:

interface nsIWifiMonitor : nsISupports
{
  void startWatching(in nsIWifiListener aListener);
  void stopWatching(in nsIWifiListener aListener);
};


A listening interface that could be notified when there is any change in the list of access points that are visable:

interface nsIWifiListener : nsISupports
{
  void onChange(in nsIArray accessPoints);
};

And, of course, a simple interface that defines what an access point is:

interface nsIWifiAccessPoint : nsISupports
{
  readonly attribute string mac;
  readonly attribute string ssid;
  readonly attribute long   signal;
};
the main use case for such a feature at present is the ability for anyone to roll out their own network geolocation provider.  See the google network protocol specification on how something like this might work:

http://code.google.com/apis/gears/geolocation_network_protocol.html
Attached patch patch v.1 (obsolete) — Splinter Review
first cut.  wip.  close though, just need to clean stuff up a bit. tested on vista, xp, mac, maemo.  needs some work on linux (link error).
Attachment #364609 - Flags: review?(jduell.mcbugs)
Some API comments from looking at the IDL...

* Does XPConnect automagically convert nsIArrays to JS arrays? Could this use the |array| idl type instead? It would be nice to be able to use the JS Array extras... eg:

  function onChange(wifiList) {
    wifiAPs.filter(...); // or .some(), .map(), etc.
  }

* I wonder if it would be good to have a synchronous getCurrentAPs() API. [The core might have to fudge this if the native interfaces are async.] Seems like a common use case is "show me all the current APs available", but someone would have to hook up a listener, wait a while (how long?) for data to come in, and eventually deem that the currently available list of APs. That's a bit awkward to use, and could be slow.

* Is the argument to onChange the full list of seen APs, or just APs added since the last onChange?

* Add nsIWifiAccessPoint.isConnected and something.getConnectedAP(), so that we can know which AP is connected to? I suspect that can be useful info for determining a more specific position, and it would also be useful for implementing a simple "am I at home or work" type of provider.

* Add something like nsIWifiListener.isWifiAvailable, so code can know if the device even has the capability for Wifi? What if the device supports Wifi but it's disabled? What happens if there are no APs in range at all (though Starbucks saturation is making this increasing unlikely :).

* Add nsIWifiListener.onError(), lest one disable Wifi while a listener is running? [Hmm, what happens on reconnect? Just fire onChange again? .onWifiAvailable()?]

* nsIWifiAccessPoint.mac and .ssid should be type nsAString.

* From the Gears page; should nsIWifiAccessPoint also expose a .signal_to_noise and .channel?
Attached patch patch v.2 (obsolete) — Splinter Review
same as before, but on linux, use dlopen for libiw.  note that some distro's did not provide a sym link, so we have to specifically test for libiw.so.29
Attachment #364609 - Attachment is obsolete: true
Attachment #365065 - Flags: review?
Attachment #364609 - Flags: review?(jduell.mcbugs)
> Does XPConnect automagically convert nsIArrays to JS arrays?

as an outvar, it think:

out unsigned long count,
[retval, array, size_is(count)] out nsIWifiAccessPoints acs

might be cleaner.  what do you think?  The common use case is going enumeration.


> I wonder if it would be good to have a synchronous getCurrentAPs() API.

nope.  scanning == time, and that shouldn't be done on the UI thread.  having an async api allows us to do stuff without hanging the UI.


> Is the argument to onChange the full list of seen APs, or just APs added
since the last onChange?

Yea, I added comments to the IDL.  The list is all of the APs in view.


> Add nsIWifiAccessPoint.isConnected and something.getConnectedAP()

and

> Add something like nsIWifiListener.isWifiAvailable,

Good feature request.  we could do management of connected ACs, but this is out of scope for this set of changes.

> Add nsIWifiListener.onError()

not sure when this would be called.  Getting no data after some period of time?  no wifi available?  Right now, no acs and an error are handled in the same way which seems to be the right thing for our use.

> nsIWifiAccessPoint.mac and .ssid should be type nsAString.

nsAString?  why?  

* From the Gears page; should nsIWifiAccessPoint also expose a .signal_to_noise
and .channel?

They are not used atm.


Thanks for the review.  let me post another patch with your comments addressed.
Attached patch patch v.3 (obsolete) — Splinter Review
incorporates justin's feedback.
Attachment #365065 - Attachment is obsolete: true
Attachment #365072 - Flags: review?(dolske)
Attachment #365065 - Flags: review?
(In reply to comment #5)
> > Does XPConnect automagically convert nsIArrays to JS arrays?
> 
> as an outvar, it think:
> 
> out unsigned long count,
> [retval, array, size_is(count)] out nsIWifiAccessPoints acs
> 
> might be cleaner.  what do you think?  The common use case is going
> enumeration.

I guess it's no big deal either way.

> > I wonder if it would be good to have a synchronous getCurrentAPs() API.
> 
> nope.  scanning == time, and that shouldn't be done on the UI thread.  having
> an async api allows us to do stuff without hanging the UI.

Right, I was assuming the list would be cached by the backend, so that it's instantly available to the caller.

How long does it normally take for onChange to fire after a listener is added?


> > Add nsIWifiListener.onError()
> 
> not sure when this would be called.

So, if Wifi is on but nothing's around, what happens? Does onChange get called once with an empty array?

If wifi isn't available (no hardware, or it's turned off), what happens?

I think we need to be able to differentiate between the two, so that code using this can let the user know it's going to work (eg, by disabling UI for wifi-based geolocation, or auto-fallback to geoip lookup). Otherwise users will get grumpy when they install an extension but it doesn't do anything / looks broken.

> > nsIWifiAccessPoint.mac and .ssid should be type nsAString.
> 
> nsAString?  why?  

AFAIK it's general XPCOM/IDL preference to use nsAString/nsACString over wstring/string.

> expose a .signal_to_noise and .channel?
> 
> They are not used atm.

Is the info readily available to expose anyway? Seems like it would be useful to have, even it's not going to be used.
> How long does it normally take for onChange to fire after a listener is added?

it should be immediate.  We fire with a cached copy until we notice a real change.

> If wifi isn't available (no hardware, or it's turned off), what happens?

you will get an initial empty array, then no changes until we detect a new access point.

> Is the info readily available to expose anyway?

It isn't consistent, or at least clear.  For example, it is easy on the Mac to get all of that information, but on Windows XP, it doesn't look like it is provided in the WLAN_BSS_ENTRY struct.  There might be another way to get it, but I didn't invest any time into investigating this since it isn't used by the primary consumer (the geolocation stuff).

> AFAIK it's general XPCOM/IDL preference to use nsAString/nsACString over
wstring/string.

Ah!  you mean AString, right?  we can do that.

i can add on onError method to nsIWifiListener, and switch the strings to use AString.   Anything else before I go off and do this?
(In reply to comment #8)
> > How long does it normally take for onChange to fire after a listener is added?
> 
> it should be immediate.  We fire with a cached copy until we notice a real
> change.

So would it be easy to add a synchronous getCurrentAPs() interface? Or would the first request be slow because the cache isn't populated?

Hmm, I guess we wouldn't want to immediately return really stale data, and we also wouldn't want to force the Wifi hardware to be powered up all the time, so I guess there isn't a sure-fire way to ensure immediate results. So, nevermind!

> > AFAIK it's general XPCOM/IDL preference to use nsAString/nsACString over
> wstring/string.
> 
> Ah!  you mean AString, right?  we can do that.

nsOops, nsYes.
Attached patch patch v.4 (obsolete) — Splinter Review
incorporate justin's feedback.
Attachment #365072 - Attachment is obsolete: true
Attachment #365242 - Flags: review?(dolske)
Attachment #365072 - Flags: review?(dolske)
(In reply to comment #7)
> > out unsigned long count,
> > [retval, array, size_is(count)] out nsIWifiAccessPoints acs
> > 
> > might be cleaner.  what do you think?  The common use case is going
> > enumeration.
> 
> I guess it's no big deal either way.

A getter on nsIWifiMonitor that returns a JS array would be much nicer than an nsIArray param for JS. Also would make it simpler to retrieve a one-time list of current access points without having to add/remove a watcher.
@gavin - the "one-time list of current access points without having to add/remove a watcher" might be empty the first time you call it because we have to wait some time before we get any response from the wifi subsystems.  Given this constraint, is it still "nice-to-have"?
I'm mostly looking to avoid use of nsIArray in JS wherever possible, since it's so crappy.
(In reply to comment #13)
> I'm mostly looking to avoid use of nsIArray in JS wherever possible, since it's
> so crappy.

Yeah. I'd rather have a JS array, although it still makes the API kind of stupid because XPCOM wants to pass around the array and count as separate params. Maybe there's a clever way to avoid that now?

(In reply to comment #8)

> It isn't consistent, or at least clear.  For example, it is easy on the Mac to
> get all of that information, but on Windows XP, it doesn't look like it is
> provided in the WLAN_BSS_ENTRY struct.  There might be another way to get it,
> but I didn't invest any time into investigating this since it isn't used by the
> primary consumer (the geolocation stuff).

Out of curiosity, how do the (multiple) MoCo Wifi APs show up? It would be neat to do intrabuilding positioning based on triangulation of the APs and their strengths. Do the Wifi bits give multiple entries with the same SSID but different MACs?
Comment on attachment 365242 [details] [diff] [review]
patch v.4

>+  /*
>+   * Public name of a wireless network.  null if not available.
>+   */
>+
>+  readonly attribute ACString ssid;

Hmmm, what charset this is expected to be in. Some quick Googling doesn't give a definitive answer, but leads me suspect the IEEE spec probably just treats this as a blob of bytes (ascii?), but that I wouldn't be surprised if some devices emit whatever charset the admin happens to be using when configuring them.

I wonder if the the Gears folks know what happens in the real world?

Maybe this should be type AString, so that we can potentially add charset guessing/conversion later without changing the API?

>+  /*
>+   * Called when there is a problem with listening to wifi
>+   *
>+   * @param error the error which caused this event.
>+   */
>+  
>+  void onError(in long error);

Should doc if these are intended to be nsresult errors or custom error values, and what likely values are.

>+++ b/netwerk/wifi/src/Makefile.in
...
>+ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)
>+CPPSRCS    += nsWifiScannerMac.cpp
>+else
>+ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
>+CPPSRCS    += nsWifiScannerWin.cpp
>+else
>+CPPSRCS    += nsWifiScannerUnix.cpp
>+endif # windows
>+endif # mac

Wrap the unix bits with a MOZ_WIDGET_TOOLKIT/gtk2 check?

>+++ b/netwerk/wifi/src/nsWifiAccessPoint.h
...
>+  void setMac(const uint8 mac_as_int[6])
>+  {
>+    // mac_as_int is big-endian. Write in byte chunks.
>+    // Format is XX-XX-XX-XX-XX-XX.

Note the format in the IDL.

>+  void setSSID(const char* aSSID, long len) {
>+    if (len < 31) {
>+        strncpy(mSsid, aSSID, len);
>+        mSsid[len] = 0;
>+    }
>+    else
>+    {
>+      mSsid[len] = 0;
>+    }
>+  };

A legal SSID can be 32 characters, but this code would effectively ignore a SSID with 31 or 32 characters. Make mSsid 33 characters to allow the max size. Should probably use sizeof() here instead of constants. Or just make mSsid some flavor of nsString?


>+NS_IMETHODIMP nsWifiMonitor::StopWatching(nsIWifiListener *aListener)
>+{

Shouldn't the monitor thread be shutdown when the last watcher is removed?

>+NS_IMETHODIMP nsWifiMonitor::Run()
>+{
>+  LOG(("@@@@@ wifi monitor run called\n"));
>+
>+  nsresult rv = DoScan();

The implementations of DoScan all seem to basically be |do { stuff; sleep; } while(keepgoing)|. It would seem a bit clearer if that loop was hoisted up to live in Run(), and then DoScan is just a single shot. [Probably would need a statup() and shutdown() too?]. This could also allow an error condition to not be fatal.
Attachment #365242 - Flags: review?(dolske) → review+
Attached patch patch v.5 (obsolete) — Splinter Review
incorporates gavin's suggestion.
Attachment #365242 - Attachment is obsolete: true
Thanks for the review Justin!  I fixed up mostly everything.  Good catch on the setSSID.  The rest was covered my changes to the IDL.  

The one part I disagreed on was the part about DoScan.

the DoScan does:

{

   init stuff that is platform specific

   do {
     
     scan

     sleep/wait

   }
   
   clean up stuff that is platform specific

}


If we break anything up, then we will need 3 functions a platform specific "startup", a platform specific "scan", and a platform specific cleanup.  We probably could do this, but I didn't find it that useful.
Attached patch patch v.6 (obsolete) — Splinter Review
incorporates justin's feedback.
Attachment #365306 - Attachment is obsolete: true
@gavin nsIArray in JS do suck.  I changed it since the primary consumers are going to be js i would guess.  and if your in C, it isn't that hard to loop over a nsIWifiAccessPoint** -- just more error prone.

@dolske The multiple MoCo Wifi APs show up with duplicated SSID and different macs.
(In reply to comment #15)

> Hmmm, what charset this is expected to be in. Some quick Googling doesn't give
> a definitive answer, but leads me suspect the IEEE spec probably just treats
> this as a blob of bytes (ascii?), but that I wouldn't be surprised if some
> devices emit whatever charset the admin happens to be using when configuring
> them.

A little more googling turns up:
  https://bugzilla.redhat.com/show_bug.cgi?id=167517
and
  https://bugs.launchpad.net/wicd/+bug/269664

So, I think this is likely to be a real issue. Two options come to mind:

A) Have both .displaySSID (AString, does charset detection (followup)) and .rawSSID (ACString, octets from the wire). This issue is tricky enough that it seems like it should be handled at the API level... But charset detection would probably cause issues for geolocation (which presumably just wants the raw, unparsed bytes from the wire), so both flavors of the ssid would need to be provided.

B) Just provide a raw .ssid, and make it clear in the docs that these are just raw bytes from the wire and that anyone wanting to actually *display* these as strings will need to deal with charset detection. Accept fate that most code won't bother, and non-western charset users will have to suffer. [Hmm, guess I'm feeling emo today!]

Magic 8-Ball says that (B) is probably more appealing to you, since it's just a simple doc change, and .displaySSID could always be added to the interface later...
more than a doc change for (b) (which I do like).  the type should be const byte.

it is easy enough to change a byte array into a displayable string in js, right?
Comment on attachment 365320 [details] [diff] [review]
patch v.6

>+  void setSSID(const char* aSSID, long len) {
>+    if (len < 32) {
>+        strncpy(mSsid, aSSID, len);
>+        mSsid[len] = 0;
>+    }

I think you want |len <= 32|, |len < 33|, or better yet |len < sizeof(mSsid)|.

Also, I didn't look very closely at the native nsWifiScanner[Mac|Unix|Win].cpp parts. r+ on the other parts, though.
Attachment #365320 - Flags: review+
thanks.  the other code is strongly based on the BSD code that we are importing. 

Probably cant be an attribute in IDL, but something like:

void getRawSSID([array,size_is(aDataLen)] out octet aData,
                out unsigned long aDataLen);
tracking-fennec: --- → 1.0b2+
Attached patch patch v.7 (obsolete) — Splinter Review
thanks dolske for the review.

this includes a getRawSsid() function that will allow you to just "get the bytes".

it also includes the ssid attribute that will return something that is lossy but displayable.  idl comments, ftw.
Attachment #365320 - Attachment is obsolete: true
Attachment #365343 - Flags: review?
Attachment #365343 - Flags: review?(ted.mielczarek)
Attachment #365343 - Flags: review?(jduell.mcbugs)
Attachment #365343 - Flags: review?
Attached patch patch v.8 (obsolete) — Splinter Review
Attachment #365343 - Attachment is obsolete: true
Attachment #365671 - Flags: review?(jst)
Attachment #365343 - Flags: review?(ted.mielczarek)
Attachment #365343 - Flags: review?(jduell.mcbugs)
Comment on attachment 365671 [details] [diff] [review]
patch v.8

>+  void getRawSSID([array,size_is(aLen)] out octet aData,
>+                  out unsigned long aLen);

I think ACString is fine for rawSSID -- it should handle nulls and arbitrary byte sequences just fine. It still comes with the caveat that the charset is unspecified, of course.

>+  readonly attribute ACString ssid;

This should be AString, which implies it's always UCS2.

I took a quick skim of the Linux NetworkManager code, it has this comment:

/**
 * nm_utils_ssid_to_utf8:
 * @ssid: pointer to a buffer containing the SSID data
 * @len: length of the SSID data in @ssid
 *
 * WiFi SSIDs are byte arrays, they are _not_ strings.  Thus, an SSID may
 * contain embedded NULLs and other unprintable characters.  Often it is
 * useful to print the SSID out for debugging purposes, but that should be the
 * _only_ use of this function.  Do not use this function for any persistent
 * storage of the SSID, since the printable SSID returned from this function
 * cannot be converted back into the real SSID of the access point.
 *
 * This function does almost everything humanly possible to convert the input
 * into a printable UTF-8 string, using roughly the following procedure:
 *
 * 1) if the input data is already UTF-8 safe, no conversion is performed
 * 2) attempts to get the current system language from the LANG environment
 *    variable, and depending on the language, uses a table of alternative
 *    encodings to try.  For example, if LANG=hu_HU, the table may first try
 *    the ISO-8859-2 encoding, and if that fails, try the Windows-1250 encoding.
 *    If all fallback encodings fail, replaces non-UTF-8 characters with '?'.
 * 3) If the system language was unable to be determined, falls back to the
 *    ISO-8859-1 encoding, then to the Windows-1251 encoding.
 * 4) If step 3 fails, replaces non-UTF-8 characters with '?'.

We don't necessarily have to implement all this in the initial landing of this patch, although I hope it doesn't get dropped on the floor. I'd be ok with spinning this part out to a followup bug, and starting off with just a simple implementation that treats the ssid as ASCII, and maps that to the UCS2 value via simple expansion (NS_ConvertASCIItoUTF16, or whatever).
Comment on attachment 365671 [details] [diff] [review]
patch v.8

- In netwerk/build/nsNetModule.cpp:

+#define WIFI_MONITOR_COMPONENT_CID \
+  { 0x3FF8FB9F, 0xEE63, 0x48DF, { 0x89, 0xF0, 0xDA, 0xCE, 0x02, 0x42, 0xFD, 0x82 } }

This (with an NS_ prefix) is already defined in nsNetCID.h, which is also what's being used, so delete this definition.

- In netwerk/wifi/Makefile.in:

+# Portions created by the Initial Developer are Copyright (C) 2008

Make that 2009 here, and in all other new files.

- In netwerk/wifi/src/TODO:

new file mode 100755
--- /dev/null
+++ b/netwerk/wifi/src/TODO
@@ -0,0 +1,7 @@
+*) remember to use the access token
+     it needs to expire
+
+*) Extension addon UI stuff
+
+*) Preference for http url

Do we really want this checked in? Shouldn't we just file bugs on those issues instead, and not include this file?

- In nsWifiAccessPoint::nsWifiAccessPoint():

+{
+}

It'd make me feel a whole lot better if this constructor made sure the char array members were null terminated.

- In AccessPointsEqual():

+    return 0;

return PR_FALSE.

+    if (!found)
+      return 0;

Ditto.

+  return 1;

return PR_TRUE.

- In nsWifiMonitor::nsWifiMonitor():

+  mMonitor = PR_NewMonitor();

Use nsAutoMonitor::NewMonitor() for better deadlock debuggability.

- In nsWifiMonitor::~nsWifiMonitor():

+  PR_DestroyMonitor(mMonitor);

Use nsAutoMonitor::DestroyMonitor()


- In nsWifiMonitor::Observe():

+    PR_EnterMonitor(mMonitor);
+    PR_Notify(mMonitor);
+    PR_ExitMonitor(mMonitor);

Any reason not to use an nsAutoMonitor, and its Notify() method here?

- In nsWifiMonitor::StartWatching():

+  PR_EnterMonitor(mMonitor);

Use an nsAutoMonitor.

- In nsWifiMonitor::StopWatching():

+  PR_EnterMonitor(mMonitor);

Ditto.

- In class nsWifiMonitor:

+  nsTArray<nsWifiListener*> mListeners;

This should be an nsTArray<nsWifiListener>, which would fix the leak that happens if someone forgets to call StopWatching() on the wifi monitor class, and would let you remove a bunch of manual new and deletes.

- In nsWifiMonitor::DoScan(), all versions:

+    PR_EnterMonitor(mMonitor);

Use the appropriate nsAutoMonitor fu here, for creating, destroying, entering, etc.

+      for (PRUint32 i = 0; i < resultCount; i++)
+        NS_ADDREF(result[i] = lastAccessPoints[i]);

lastAccessPoints owns all the access points here, no need to make the temporary result array own them too, right?

+      for (PRUint32 i = 0; i < resultCount; i++)
+        NS_RELEASE(result[i]);

Do the above, and you can remove this loop.

Also, above this, is the loop that uses sync proxies to proxy a call to the main thread. Is that really the best way to get these notifications to the main thread? Wouldn't an async runnable (or one per access point if so need be) be less prone to lock issues etc? What about shutdown, do sync proxies deal properly if we're in the middle of shutting down and we attempt to do a number of sync calls to the main thread?

That's as far as I got so far...
Attached patch patch v.9Splinter Review
fixes all concerns, but...

i didn't move away from proxies.  I don't think they are better, but I don't think they are any worse.  It is a small change which to move to something like runnables or just simple plevents.  lets review this point after the rest of the patch is reviewed.
Attachment #365671 - Attachment is obsolete: true
Attachment #365929 - Flags: review?(jst)
Attachment #365671 - Flags: review?(jst)
Comment on attachment 365929 [details] [diff] [review]
patch v.9

jst - pretend that this patch has updated dates in the license headers.  fwiw, the patch queue i am working on is here:

http://hg.mozilla.org/users/dougt_mozilla.com/bug_479898_wifi_support/
Blocks: 482613
Comment on attachment 365929 [details] [diff] [review]
patch v.9

- In netwerk/wifi/src/nsWifiScannerMac.cpp

+    PR_EnterMonitor(mMonitor);

Use a scoped nsAutoMonitor here, this pattern appears more than once in this file. Same thing in the unix and windows versions.

r=jst with that.
Attachment #365929 - Flags: review?(jst) → review+
Comment on attachment 365929 [details] [diff] [review]
patch v.9

jst, this was reviewed by dolske and stuart (he looked over it, but not sure about the depth).

do you mind sr'ing, or suggestion who else should be looking at this patch?
Attachment #365929 - Flags: superreview?
Attachment #365929 - Flags: superreview? → superreview?(jst)
Comment on attachment 365929 [details] [diff] [review]
patch v.9

>+++ b/netwerk/wifi/public/nsIWifiAccessPoint.idl
...
>+  /*
>+   * Public name of a wireless network.  The charset of this string is ASCII.
>+   * This string will be null if not available.
>+   *
>+   * Note that this is a conversion of the SSID which makes it "displayable".
>+   * for any comparisons, you want to use the Raw SSID.
>+   */
>+
>+  readonly attribute ACString ssid;

I really think this needs to be an AString, otherwise code using it still needs to deal with charset headaches. The NetworkManager code implies non-ASCII sids are in the wild, so just ASCII is insufficient. [The interface could be UTF8, but that's more pain than it's worth.]
dolske, fair enough.  however, the rawSSID remains a ACString.
Attachment #365929 - Flags: superreview?(jst) → superreview+
Attachment #365929 - Flags: approval1.9.1?
http://hg.mozilla.org/mozilla-central/rev/f29948232171
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 484115
When building with VC7.1 nsWifiScannerWin.cpp manages not to include any definitions for malloc/realloc/free; would you prefer a patch to add stdlib.h or malloc.h or convert the calls to use the XPCOM memory allocator?
#includes probably are the best.  sorry to have busted v7.1 users.
The file has DOS line endings, would you mind if I switched them to unix?
Attachment #368623 - Flags: review?(doug.turner)
Comment on attachment 368623 [details] [diff] [review]
VC7.1 bustage fix

please!
Attachment #368623 - Flags: review?(doug.turner) → review+
Comment on attachment 368623 [details] [diff] [review]
VC7.1 bustage fix

Pushed changeset 8d17fb3a6197 to mozilla-central.
I presume this is not going into 1.9.1?
it is marked approval1.9.1? and is blocking fennec.
Attachment #365929 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 365929 [details] [diff] [review]
patch v.9

a191=beltzner

Need at least a litmus test, but it would be good to get a set of testcases even if just to assure that we don't crash when calling the stuff.
Blocks: 495647
Target Milestone: --- → mozilla1.9.2a1
Depends on: 739132
You need to log in before you can comment on or make changes to this bug.