Closed Bug 654725 Opened 9 years ago Closed 9 years ago

[10.7] Changes in OS X 10.7 break our Wi-Fi monitor code, causing crashes [@ libsystem_c.dylib@0x8c4f0 ] [@ libsystem_c.dylib@0x8ce90 ]

Categories

(Core :: Networking, defect, critical)

All
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox5 + fixed

People

(Reporter: smichaud, Assigned: smichaud)

References

(Blocks 1 open bug)

Details

(Keywords: reproducible)

Attachments

(2 files)

Blocks: 514626
Summary: Changes in OS X 10.7 break our Wi-Fi monitor code, causing crashes [@ libsystem_c.dylib@0x8c4f0 ] → Changes in OS X 10.7 break our Wi-Fi monitor code, causing crashes [@ libsystem_c.dylib@0x8c4f0 ] [@ libsystem_c.dylib@0x8ce90 ]
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).
And I've only found crashes in 64-bit mode -- again I don't know why.
Summary: 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 [@ libsystem_c.dylib@0x8c4f0 ] [@ libsystem_c.dylib@0x8ce90 ]
From triage today - Steve can you investigate a bit since Josh is away right now?
> 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.
This bug is more important than bug 621117. I can't speak to its relative priority against bug 654583.
Steven: I don't have the OS version associated with most of the crashes. Working on getting it now.
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.
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.
Assignee: nobody → smichaud
Attached patch FixSplinter Review
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.
Attachment #530637 - Flags: review?(joshmoz)
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.
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
Keywords: reproducible
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.
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.
if this is still a problem as we approach aurora for 6, please re-nominate.
Steven - have you tested with build 11A444d? That is the latest build.
> 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.
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
> 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 on attachment 530637 [details] [diff] [review]
Fix

Review of attachment 530637 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #530637 - Flags: review?(joshmoz) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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).
Attachment #530637 - Flags: approval-mozilla-aurora?
Attached patch Fix for fixSplinter Review
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.
Attachment #531657 - Flags: review?(joshmoz)
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.
> 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 on attachment 531657 [details] [diff] [review]
Fix for fix

Review of attachment 531657 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531657 - Flags: review?(joshmoz) → review+
Attachment #531657 - Flags: approval-mozilla-aurora?
Attachment #530637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #531657 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Please land for Aurora by Monday May 16 or the approval will potentially be lost.  Please mark as status-firefox5 fixed when you do.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.