853917 - crash in imgRequestProxy::UnblockOnload

Status: RESOLVED WORKSFORME

(Core :: Graphics: ImageLib, defect)

Description

[Scoobidiver (away)]

It first showed up in 22.0a1/20130321090706. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d6fe70c79c5&tochange=a73a2b5c423b
It's likely a regression from bug 716140.

Signature 	nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) | imgRequestProxy::UnblockOnload() More Reports Search
UUID	3ae281b9-1b84-402c-b1d1-b51502130322
Date Processed	2013-03-22 15:57:57
Uptime	202
Last Crash	9.7 minutes before submission
Install Age	1.2 hours since version was first installed.
Install Time	2013-03-22 14:46:36
Product	Firefox
Version	22.0a1
Build ID	20130322031028
Release Channel	nightly
OS	Mac OS X
OS Version	10.8.3 12D78
Build Architecture	amd64
Build Architecture Info	family 6 model 42 stepping 7
Crash Reason	EXC_BAD_ACCESS / 0x0000000d
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x 116GL Context? GL Context+ GL Layers? GL Layers+ 
Processor Notes 	sp-processor06.phx1.mozilla.com_12367:2008; MDSW emitted too many frames, triggering truncation; exploitablity tool: ERROR: unable to analyze dump
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x 116

Frame 	Module 	Signature 	Source
0 	XUL 	nsCOMPtr_base::assign_from_qi 	nsCOMPtr.cpp:14
1 	XUL 	imgRequestProxy::UnblockOnload 	nsCOMPtr.h:556
2 	XUL 	nsHttpDigestAuth::GenerateCredentials::hexChar 	
3 	XUL 	_ZZN7mozilla5imageL14get_header_strEPcS1_mE3hex 	
4 	XUL 	imgStatusTracker::SyncAndSyncNotifyDifference 	imgStatusTracker.cpp:1005
5 	XUL 	_ZZN7mozilla5imageL14get_header_strEPcS1_mE3hex 	
6 	XUL 	mozilla::image::nsPNGDecoder::~nsPNGDecoder 	nsPNGDecoder.cpp:131
7 	XUL 	mozilla::image::RasterImage::FinishedSomeDecoding 	RasterImage.cpp:3451
8 	XUL 	mozilla::image::RasterImage::DecodeWorker::DecodeABitOf 	RasterImage.cpp:3579
9 	XUL 	mozilla::image::RasterImage::RequestDecodeCore 	RasterImage.cpp:2826
10 	XUL 	nsMenuItemIconX::LoadIcon 	widget/cocoa/nsMenuItemIconX.mm:324
11 	XUL 	nsMenuItemIconX::SetupIcon 	widget/cocoa/nsMenuItemIconX.mm:110
12 	XUL 	nsMenuX::LoadMenuItem 	widget/cocoa/nsMenuX.mm:535
13 	XUL 	nsMenuX::GetMenuPopupContent 	obj-firefox/x86_64/dist/include/nsCOMPtr.h:410
14 	XUL 	nsMenuX::GetMenuPopupContent 	obj-firefox/x86_64/dist/include/nsCOMPtr.h:410
15 	XUL 	nsMenuX::MenuConstruct 	widget/cocoa/nsMenuX.mm:440
16 	XUL 	nsMenuX::RemoveAll 	widget/cocoa/nsMenuX.mm:322
17 	XUL 	nsMenuX::MenuOpened 	widget/cocoa/nsMenuX.mm:349
18 	libobjc.A.dylib 	lookUpMethod 	
19 	AppKit 	AppKit@0x551e27 	
20 	HIToolbox 	HIToolbox@0x35c83 	
21 	HIToolbox 	HIToolbox@0x37a62 	
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsCOMPtr_base%3A%3Aassign_from_qi%28nsQueryInterface%2C+nsID+const%26%29+|+imgRequestProxy%3A%3AUnblockOnload%28%29
https://crash-stats.mozilla.com/report/list?signature=imgRequestProxy%3A%3AUnblockOnload%28%29

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Ensure valid mListener in imgRequestProxy::(Unblock|Block)Onload.

Looks like nsMenuX::MenuOpened will sometimes decide a menu needs to be rebuilt. When it does that, it'll tear down the old menu with nsMenuX::RemoveAll, and then construct a new one with nsMenuX::MenuConstruct. The problem seems to be that if we constructed the original version of the menu quite recently, this can lead to a situation where an imgRequestProxy that has already been canceled (and hence, has nulled out its listener) is notified to unblock onload. Since we assume that mListener is non-null in imgRequestProxy::UnblockOnload, this crashes. The timing changes in bug 716140 exposed this bug.

To fix this, I just added a check that mListener is non-null in both imgRequestProxy::UnblockOnload and (since it suffered from the same problem) imgRequestProxy::BlockOnload. I think that's all that's needed here. AFAICT, the behavior is perfectly reasonable; we just need to handle it.

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 729352 [details] [diff] [review]
Ensure valid mListener in imgRequestProxy::(Unblock|Block)Onload.

I don't understand why this patch makes a difference. do_QueryInterface returns null if passed a null pointer; the only way we're calling UnblockOnload is if mListener is non-null.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Wasn't aware of that. Obviously I don't have a test to reproduce this issue. =)

If that's the case, I would have to assume that imgRequestProxy's this pointer is null here. That'd be a slightly different patch.

Comment 4

[Scoobidiver (away)]

There are no crashes after 22.0a1/20130322. The working range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0e9badd3cf39&tochange=3825fdbcec62