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

VERIFIED FIXED

Status

()

Core
Networking
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: smichaud, Assigned: smichaud)

Tracking

(Blocks: 1 bug, {reproducible})

unspecified
All
Mac OS X
reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5+ fixed)

Details

Attachments

(2 attachments)

Crashes have started to accumulate at various addresses in
libsystem_c.dylib on OS X.  Many (if not all) of them happen on OS X
10.7, in Wi-Fi monitor code.

https://crash-stats.mozilla.com/report/list?product=Firefox&platform=mac&query_search=signature&query_type=contains&query=libsystem_c.dylib&reason_type=contains&date=05%2F04%2F2011%2008%3A05%3A55&range_value=3&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=libsystem_c.dylib%400x8c4f0
https://crash-stats.mozilla.com/report/list?product=Firefox&platform=mac&query_search=signature&query_type=contains&query=libsystem_c.dylib&reason_type=contains&date=05%2F04%2F2011%2008%3A05%3A55&range_value=3&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=libsystem_c.dylib%400x8ce90

And many (if not all) of them happen at the following line, when a
call to setSSID() causes a null-dereference, presumably because aSSID
is NULL.

http://hg.mozilla.org/mozilla-central/annotate/a92024952aab/netwerk/wifi/osx_corewlan.mm#l103

I suspect OS X 10.7 has made changes to the CoreWLAN framework used by
code in netwerk/wifi/osx_corelan.mm.
(Assignee)

Updated

6 years ago
Blocks: 514626
(Assignee)

Updated

6 years ago
Blocks: 636455
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 ]
(Assignee)

Comment 1

6 years ago
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).
(Assignee)

Comment 2

6 years ago
And I've only found crashes in 64-bit mode -- again I don't know why.
(Assignee)

Updated

6 years ago
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
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 ]

Comment 3

6 years ago
From triage today - Steve can you investigate a bit since Josh is away right now?
tracking-firefox5: ? → +
(Assignee)

Comment 4

6 years ago
> 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.
(Assignee)

Comment 7

6 years ago
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.
(Assignee)

Comment 8

6 years ago
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)

Updated

6 years ago
Assignee: nobody → smichaud
(Assignee)

Comment 9

6 years ago
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.
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.

Comment 14

6 years ago
if this is still a problem as we approach aurora for 6, please re-nominate.
tracking-firefox6: ? → ---

Comment 15

6 years ago
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 19

6 years ago
Comment on attachment 530637 [details] [diff] [review]
Fix

Review of attachment 530637 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #530637 - Flags: review?(joshmoz) → review+
Comment on attachment 530637 [details] [diff] [review]
Fix

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/fbe2374e7ea0
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 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?
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.
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 25

6 years ago
Comment on attachment 531657 [details] [diff] [review]
Fix for fix

Review of attachment 531657 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #531657 - Flags: review?(joshmoz) → review+
Comment on attachment 531657 [details] [diff] [review]
Fix for fix

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/f209ae2ce125
(Assignee)

Updated

6 years ago
Attachment #531657 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #530637 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #531657 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 27

6 years ago
Please land for Aurora by Monday May 16 or the approval will potentially be lost.  Please mark as status-firefox5 fixed when you do.
Landed on aurora:
http://hg.mozilla.org/mozilla-aurora/rev/b3f70217f07e
status-firefox5: --- → fixed
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.