512301 - crash @ nsWifiMonitor::DoScan() using GeoGuide add-on

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Brian Crowder]

Keeping security-sensitive until we're sure.

xul.dll!nsWifiMonitor::DoScan(void) Line: 345, Byte Offsets: 0x10	C++
xul.dll!nsWifiMonitor::Run(void) Line: 156, Byte Offsets: 0x44	C++
xul.dll!nsThread::ProcessNextEvent(int mayWait = 0x00000001, int* result = 0x0a62fdbc) Line: 527, Byte Offsets: 0x368	C++
xul.dll!NS_ProcessNextEvent_P(nsIThread* thread = 0x5e2e71a0, int mayWait = 0x00000001) Line: 230, Byte Offsets: 0x84	C++
xul.dll!nsThread::ThreadFunc(void* arg = 0x5e2e71a0) Line: 254, Byte Offsets: 0x15c	C++

Comment 1

[Doug Turner (:dougt)]

fwiw, this doesn't happen (or is alot less likely) when the addon is not installed and you simply do a geolocation request.

Comment 2

[Doug Turner (:dougt)]

crowder, in the bug you mentioned you were dying in PerformQuery() still true?  asking this, sadly, cause I cant reproduce.

Comment 3

[Brian Crowder]

Yeah, PerformQuery()... I was running w/ a debug build, perhaps that is the difference?

Comment 4

[Doug Turner (:dougt)]

could you have oom'd?

Comment 5

[Brian Crowder]

Yes, it's certainly possible, though as I mentioned in IRC, the code would seem to handle that case!

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.1

if buffer does go null because resize failed, we will crash.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 398203 [details] [diff] [review]
patch v.1


>+  if (!buffer)
>+    return ERROR_OUTOFMEMORY;
>+

The buffer isn't being allocated in this function, so its not correct to return OOM from it.

I think it would be better to add some assertions about oid_buffer_size such as != 0 and < kMaximumBufferSize.  Also, checking the buffer for null before passing it to PerformQuery would be good.

Comment 8

[Doug Turner (:dougt)]

> The buffer isn't being allocated in this function, so its not correct to return
OOM from it.

http://msdn.microsoft.com/en-us/library/aa450919.aspx

How about ERROR_NOT_ENOUGH_MEMORY?

> checking the buffer for null before passing it to PerformQuery would be good.

I do not think we want to have another #ifdef WINCE -- it is fine to pass null to the win32 version of PerformQuery.

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 11

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/e11dd2be5fb9

This crash was pretty intermittent.  This patches a obvious crash during OOM.

Comment 12

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/dc79ef0716c6 <-- fixes bustage

Comment 13

[Reed Loden [:reed]]

Doug landed these on 1.9.2:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/47badd5f4c74
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8eed6221990c