Last Comment Bug 654725 - [10.7] Changes in OS X 10.7 break our Wi-Fi monitor code, causing crashes [@ libsystem_c.dylib@0x8c4f0 ] [@ libsystem_c.dylib@0x8ce90 ]
: [10.7] Changes in OS X 10.7 break our Wi-Fi monitor code, causing crashes [@ ...
Status: VERIFIED FIXED
: reproducible
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All Mac OS X
: -- critical (vote)
: ---
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
Depends on:
Blocks: lion-compatibility 514626
  Show dependency treegraph
 
Reported: 2011-05-04 08:59 PDT by Steven Michaud [:smichaud] (Retired)
Modified: 2011-05-16 11:49 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Fix (3.07 KB, patch)
2011-05-06 08:33 PDT, Steven Michaud [:smichaud] (Retired)
jaas: review+
sayrer: approval‑mozilla‑aurora+
Details | Diff | Review
Fix for fix (1.25 KB, patch)
2011-05-11 09:39 PDT, Steven Michaud [:smichaud] (Retired)
jaas: review+
sayrer: approval‑mozilla‑aurora+
Details | Diff | Review

Comment 1 Steven Michaud [:smichaud] (Retired) 2011-05-04 09:04:54 PDT
So far I've only found these crashes on Firefox 4.0 and up.

I'm not sure why.  Maybe it's just because very few people are running
FF 3.6.X or below on OS X 10.7 (which is still only available as
developer previews).
Comment 2 Steven Michaud [:smichaud] (Retired) 2011-05-04 09:06:12 PDT
And I've only found crashes in 64-bit mode -- again I don't know why.
Comment 3 JP Rosevear [:jpr] 2011-05-04 11:50:29 PDT
From triage today - Steve can you investigate a bit since Josh is away right now?
Comment 4 Steven Michaud [:smichaud] (Retired) 2011-05-04 12:15:14 PDT
> From triage today - Steve can you investigate a bit since Josh is
> away right now?

Sure.  But I probably won't get to it until later this week (or more
likely early next week), since I'm currently working on a couple of
other bugs that are (probably) more urgent -- bug 621117 and bug
654583.
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-05-04 12:18:33 PDT
This bug is more important than bug 621117. I can't speak to its relative priority against bug 654583.
Comment 6 Marcia Knous [:marcia - use ni] 2011-05-04 14:20:51 PDT
Steven: I don't have the OS version associated with most of the crashes. Working on getting it now.
Comment 7 Steven Michaud [:smichaud] (Retired) 2011-05-04 14:55:20 PDT
The signature (from comment #0) with the most crashes
(libsystem_c.dylib@0x8c4f0) is encountered only on build 11A430e --
which was current until a newer one came out a few days ago.

The other signature (libsystem_c.dylib@0x8ce90), though, is
encountered only on build 11A390 -- the first DP released by Apple,
and the one that you have.
Comment 8 Steven Michaud [:smichaud] (Retired) 2011-05-04 15:50:14 PDT
Judging by the contents of the CoreWLAN framework header file
CWInterface.h, the behavior of a number of CWInterface properties has
changed -- specifically, a lot of them can now be 'nil' when an error
happens.

Tomorrow I'll post a patch that deals with this.
Comment 9 Steven Michaud [:smichaud] (Retired) 2011-05-06 08:33:33 PDT
Created attachment 530637 [details] [diff] [review]
Fix

Here's a patch that should fix this bug.

And here's a tryserver build made with the patch.  Though it may not
be much use, as I haven't been able to figure out how to reproduce the
crashes.

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-28bfa2c75f79/try-macosx64/firefox-6.0a1.en-US.mac.dmg

The CWInterface.h header file included with the CoreWLAN framework on
OS X 10.7 adds the following comment to a number of this interface's
"properties":

  Returns nil in the case of an error, or if the interface is not
  participating in a network.

This bug's crashes (and the fact that they only happen on OS X 10.7)
show that these comments reflect a genuine change in the framework's
behavior on OS X 10.7.  So I've changed nsWifiAccessPoint::SetMac()
and nsWifiAccessPoint::SetSSID() to behave properly when called with
NULL parameters.

The new header file also deprecates a couple of CWInterface properties
that we've been using.  My new patch accomodates these changes, but
with the following twist:  One of the deprecated properties
(bssidData) is a lot more convenient for our purposes than the one
that's supposed to replace it (bssid).  So I've made my patch continue
to use bssidData for as long as it remains available.
Comment 10 Steven Michaud [:smichaud] (Retired) 2011-05-06 08:48:33 PDT
Doug Turner provided a testcase at
http://people.mozilla.org/~dougt/wifi_access_point_test.html for his
patch for bug 514626.  But this has disappeared from the net.

Here's another testcase, which is actually just the sample code from
https://developer.mozilla.org/En/Monitoring_WiFi_access_points:

https://people.mozilla.com/~stmichaud/bmo/monitor_wifi_access_points.html

You can use it to test that my patch doesn't break WiFi monitoring.
But (like I said above) it's not really a good testcase for this bug,
because I haven't been able to use it to reproduce this bug's crashes
(on builds without my patch).

Testing with FF 4.0.1 on OS X 10.7, I tried turning my WiFi router off
and on, then reloading the page.  Or turning Airport off and on, then
reloading the page.  I didn't see any crashes.
Comment 11 Marcia Knous [:marcia - use ni] 2011-05-06 11:49:50 PDT
Steven: I can reproduce this consistently on the trunk after finally updating to the latest seed.

STR:
1. Connect to the Mozilla WiFi Network
2. http://maps.google.com/
3. Select the "Show my Location" button on the map (located right above the orange man on the map).
4. Click "Share my Location"
5. Crash


https://crash-stats.mozilla.com/report/index/35129f4c-0d0a-474d-8df9-21e312110506
Comment 12 Steven Michaud [:smichaud] (Retired) 2011-05-06 12:25:24 PDT
Thanks, Marcia!

I'll update too (on my laptop, which is currently running build
11A390, the first seed).  But that'll take me a while.

In the meantime, please test your STR on my tryserver build from
comment #9, and see if it makes the crashes go away.
Comment 13 Marcia Knous [:marcia - use ni] 2011-05-06 12:38:37 PDT
Steven: I can confirm that I am not able to reproduce the crash using your build in Comment 9 and the same STR in Comment 11. Good work.

(In reply to comment #12)
> Thanks, Marcia!
> 
> I'll update too (on my laptop, which is currently running build
> 11A390, the first seed).  But that'll take me a while.
> 
> In the meantime, please test your STR on my tryserver build from
> comment #9, and see if it makes the crashes go away.
Comment 14 Asa Dotzler [:asa] 2011-05-06 13:58:54 PDT
if this is still a problem as we approach aurora for 6, please re-nominate.
Comment 15 Josh Aas 2011-05-10 06:13:15 PDT
Steven - have you tested with build 11A444d? That is the latest build.
Comment 16 Steven Michaud [:smichaud] (Retired) 2011-05-10 07:40:32 PDT
> Steven - have you tested with build 11A444d? That is the latest
> build.

Yes, I now have.  I'm not able to reproduce this bug on it (using
Marcia's STR from comment #11), but that probably has to do with the
differences between my WiFi setup and Mozilla's.
Comment 17 Steven Michaud [:smichaud] (Retired) 2011-05-10 10:19:42 PDT
jpr points out that this bug is on a (short) list of bugs that must be
fixed before the first FF5 beta (that is by May 17th):

https://wiki.mozilla.org/Firefox/Aurora/2011-05-05#Triage
Comment 18 Steven Michaud [:smichaud] (Retired) 2011-05-10 11:16:23 PDT
> jpr points out that this bug is on a (short) list of bugs that must
> be fixed before the first FF5 beta (that is by May 17th):
>
> https://wiki.mozilla.org/Firefox/Aurora/2011-05-05#Triage

Which means that my patch (or something like it) needs to get onto
mozilla-central, get approval for aurora, and then get onto aurora --
all by the 17th.
Comment 19 Josh Aas 2011-05-10 12:08:32 PDT
Comment on attachment 530637 [details] [diff] [review]
Fix

Review of attachment 530637 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 20 Steven Michaud [:smichaud] (Retired) 2011-05-10 12:47:59 PDT
Comment on attachment 530637 [details] [diff] [review]
Fix

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/fbe2374e7ea0
Comment 21 Steven Michaud [:smichaud] (Retired) 2011-05-10 13:32:31 PDT
Comment on attachment 530637 [details] [diff] [review]
Fix

This patch is very simple (two null checks, plus some code to avoid
using newly deprecated methods), and therefore very low risk.

This bug needs to be fixed on aurora before May 17th, so that it can
be fixed in the first FF5 beta (see comment #17 and comment #18).
Comment 22 Steven Michaud [:smichaud] (Retired) 2011-05-11 09:39:03 PDT
Created attachment 531657 [details] [diff] [review]
Fix for fix

This morning I realized that part of my patch is incorrect -- not the
null checks (of course), but part of one of the deprecation changes:
In the initial patch, it turns out I scanned parts of the NSString
returned by [anObject bssid] for decimal integer values.  I should
have been scanning for hexadecimal integer values.  This new patch
fixes the problem.
Comment 23 Johnathan Nightingale [:johnath] 2011-05-11 11:44:53 PDT
Can't approve until we have the pieces, leaving it nom'd, but hoping very much that we can get it in this week. Holler loudly when you have a final patch to approve.
Comment 24 Steven Michaud [:smichaud] (Retired) 2011-05-11 11:55:52 PDT
> Can't approve until we have the pieces

Quite right.  I'm currently waiting on Josh.  But if need be I'll ask someone else.

> Holler loudly when you have a final patch to approve.

I'll track you down on IRC :-)
Comment 25 Josh Aas 2011-05-11 12:49:21 PDT
Comment on attachment 531657 [details] [diff] [review]
Fix for fix

Review of attachment 531657 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 26 Steven Michaud [:smichaud] (Retired) 2011-05-11 13:09:04 PDT
Comment on attachment 531657 [details] [diff] [review]
Fix for fix

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/f209ae2ce125
Comment 27 JP Rosevear [:jpr] 2011-05-12 14:45:13 PDT
Please land for Aurora by Monday May 16 or the approval will potentially be lost.  Please mark as status-firefox5 fixed when you do.
Comment 28 Steven Michaud [:smichaud] (Retired) 2011-05-12 15:14:11 PDT
Landed on aurora:
http://hg.mozilla.org/mozilla-aurora/rev/b3f70217f07e
Comment 29 Marcia Knous [:marcia - use ni] 2011-05-16 11:49:07 PDT
Looks good on Aurora using Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:5.0a2) Gecko/20110516 Firefox/5.0a2 as well as the trunk Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:6.0a1) Gecko/20110516 Firefox/6.0a1. Not able to reproduce the crash following the STR in Comment 11.

Note You need to log in before you can comment on or make changes to this bug.