145147 - Crashes Sniffing Mime types M1RC2 Trunk [@ imgRequest::SniffMimeType]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[greer]

The topcrash list for M1RC2 has a number of crashes at this signature. Looking 
at past data, this problem showed up between M099 and M1RC1 (between March 11 
and April 17). Blame points to cbeisinger's patch for bug 104906 (March 23) in 
imgRequest.cpp.

imgRequest::SniffMimeType   
[d:\builds\seamonkey\mozilla\modules\libpr0n\src\imgRequest.cpp, line 788]         
sniff_mimetype_callback   
[d:\builds\seamonkey\mozilla\modules\libpr0n\src\imgRequest.cpp, line 779]         
nsPipe::nsPipeInputStream::ReadSegments   
[d:\builds\seamonkey\mozilla\xpcom\io\nsPipe2.cpp, line 420]         
imgRequest::OnDataAvailable   
[d:\builds\seamonkey\mozilla\modules\libpr0n\src\imgRequest.cpp, line 691]         
ProxyListener::OnDataAvailable   
[d:\builds\seamonkey\mozilla\modules\libpr0n\src\imgLoader.cpp, line 705]         
nsHttpChannel::OnDataAvailable   
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp, line 
2965]         nsOnDataAvailableEvent::HandleEvent   
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamListenerProxy.cpp, line 
203]         PL_HandleEvent   
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 597]         
PL_ProcessPendingEvents   [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, 
line 530]         nsEventQueueImpl::ProcessPendingEvents   
[d:\builds\seamonkey\mozilla\xpcom\threads\nsEventQueue.cpp, line 392]

The values at the time of the crash are:

Comment 1

[greer]

cc'ing cbiesinger who checked in the fix for bug 104906.

Comment 2

[greer]

Attachment: Stack values

Attachment with the values from the top three stack frames on the main thread
at the time of crash.

Comment 3

[greer]

Attachment: User comments

User comments for help with repro/testcase.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

I can neither reproduce this nor can see why this function can cause a crash...
I'd suspect that do_GetService fails, but don't know why that would fail.

The other possibility is that |this| is invalid, because it was deleted or
points to invalid memory...

It's hard to fix this when I don't know what causes it...

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Is it possible to find out where the nsCOMPtr<imgILoader> points to?

Comment 6

[greer]

Unfortunately, no, we can't see where nsCOMPtr<imgILoader> points to. Talkback 
does not resolve smart pointers.

Comment 7

[David :Bienvenu]

I hit this crash, and it was because I closed down the browser while it was
still loading (my.netscape.com) - the get service call failed, because I was
shutting down. A simple check for a null loader will fix this - I manually
continued on and shut down fine. I'll attach the trivial patch.

Comment 8

[David :Bienvenu]

Attachment: proposed fix

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 84855 [details] [diff] [review]
proposed fix

looks good, r=biesi

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 84855 [details] [diff] [review]
proposed fix

looks good, r=biesi

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 84855 [details] [diff] [review]
proposed fix

looks good, r=biesi

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 84855 [details] [diff] [review]
proposed fix

looks good, r=biesi

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 84855 [details] [diff] [review]
proposed fix

looks good, r=biesi

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 84855 [details] [diff] [review]
proposed fix

looks good, r=biesi

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 84855 [details] [diff] [review]
proposed fix

looks good, r=biesi

Comment 16

[David :Bienvenu]

I'd like to make sure this is OK with pav.

Comment 17

[David :Bienvenu]

Attachment: patch with assertion per pav's request

Comment 18

[Stuart Parmenter]

Comment on attachment 84940 [details] [diff] [review]
patch with assertion per pav's request

r=pavlov

Comment 19

[Stuart Parmenter]

Attachment: Don't use GetService at all

convert this to use a static method on imgLoader and avoid the GetService stuff
completly.

Comment 20

[David :Bienvenu]

Comment on attachment 84946 [details] [diff] [review]
Don't use GetService at all

r=bienvenu

Comment 21

[Scott MacGregor]

Comment on attachment 84946 [details] [diff] [review]
Don't use GetService at all

sr=mscott

Comment 22

[Stuart Parmenter]

fixed on trunk.  need a='s from drivers and adt

Comment 23

[Shiva Thirumazhusai]

->nsbeta1+ and added adt1.0.0

Comment 24

[Dawn Endico]

Comment on attachment 84946 [details] [diff] [review]
Don't use GetService at all

a=roc,scc,shaver

please check this in to
the mozilla 1.0 branch asap

Comment 25

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for approval to checkin to the 1.0 branch. After,
checking in, please add the fixed1.0 keyword.

Comment 26

[Stuart Parmenter]

fix on trunk and branch now

Comment 27

[Terri Preston]

Verified fix checked into branch and trunk, marking verified