253597 - Trunk crash blocking ads/images [@ nsImgManager::TestPermission]

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Jay Patel [:jay]]

There aren't a lot of these yet, but enough across all platforms to show up on
the topcrash radar for the latest Trunk builds.  I also see a few crashes in
Firefox 0.9.x. 

Here's some Talkback data:
Rank    StackSignature    Count  

6   nsImgManager::TestPermission   9 

 
 	Source File :
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/extensions/cookie/nsImgManager.cpp
line : 302
 
====================================================================================================
     Count   Offset    Real Signature
[ 4   nsImgManager::TestPermission 63be5c50 - nsImgManager::TestPermission ]
 
     Crash date range: 19-JUL-04 to 28-JUL-04
     Min/Max Seconds since last crash: 6 - 89723
     Min/Max Runtime: 334 - 89723
 
     Count   Platform List 
     4   Windows XP [Windows NT 5.1 build 2600] 
 
     Count   Build Id List 
     2   2004071908
     1   2004072307
     1   2004072209
 
     No of Unique Users         3
 
 Stack trace(Frame) 

	 nsImgManager::TestPermission
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/extensions/cookie/nsImgManager.cpp
 line 302] 
	 nsImgManager::ShouldLoad
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/extensions/cookie/nsImgManager.cpp
 line 215] 
	 nsImgManager::ShouldProcess
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/extensions/cookie/nsImgManager.cpp
 line 249] 
	 nsContentPolicy::CheckPolicy
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsContentPolicy.cpp
 line 188] 
	 nsContentPolicy::ShouldProcess
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/base/src/nsContentPolicy.cpp
 line 268] 
	 NS_CheckContentProcessPolicy
[../../../../dist/include/content\nsContentPolicyUtils.h  line 182] 
	 ImageListener::OnStartRequest
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/html/document/src/nsImageDocument.cpp
 line 186] 
	 nsDocumentOpenInfo::OnStartRequest
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/uriloader/base/nsURILoader.cpp
 line 330] 
	 nsHttpChannel::CallOnStartRequest
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp
 line 669] 
	 nsHttpChannel::ProcessNormal
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp
 line 837] 
	 nsHttpChannel::ProcessResponse
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp
 line 774] 
	 nsHttpChannel::OnStartRequest
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp
 line 3551] 
	 nsInputStreamPump::OnStateStart
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/netwerk/base/src/nsInputStreamPump.cpp
 line 383] 
	 nsInputStreamPump::OnInputStreamReady
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/netwerk/base/src/nsInputStreamPump.cpp
 line 345] 
	 nsInputStreamReadyEvent::EventHandler
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/io/nsStreamUtils.cpp
 line 215] 
	 PL_HandleEvent
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c
 line 693] 
	 PL_ProcessPendingEvents
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c
 line 631] 
	 _md_EventReceiverProc
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c
 line 1434] 
	 USER32.dll + 0x3d79 (0x77d43d79)  
	 USER32.dll + 0x3ddf (0x77d43ddf)  
	 nsAppShellService::Run
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/appshell/src/nsAppShellService.cpp
 line 489] 
	 main1
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 1331] 
	 main
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 1801] 
	 WinMain
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp
 line 1827] 
	 WinMainCRTStartup()  
	 kernel32.dll + 0x214c7 (0x77e814c7)   
 
     (402328)	URL: http://sourceforge.net/projects/feedreader/
     (402328)	Comments: Attempted to download feedreader  .exe  when terminated
     (366389)	URL: www.jibjab.com
 
====================================================================================================
     Count   Offset    Real Signature
[ 1   nsImgManager::TestPermission() 3c4ed5d1 - nsImgManager::TestPermission() ]
 
     Crash date range: 25-JUL-04 to 25-JUL-04
     Min/Max Seconds since last crash: 14412 - 14412
     Min/Max Runtime: 14412 - 14412
 
     Count   Platform List 
     1   [Darwin 7.4.0]      
 
     Count   Build Id List 
     1   2004072408
 
     No of Unique Users         1
 
 Stack trace(Frame) 

	 nsImgManager::TestPermission()  
	 nsImgManager::TestPermission()  
	 nsImgManager::ShouldProcess()  
	 nsContentPolicy::CheckPolicy(unsigned (nsIContentPolicy::*)()  
	 nsContentPolicy::ShouldProcess()  
	 ImageListener::OnStartRequest()  
	 nsDocumentOpenInfo::OnStartRequest()  
	 nsHttpChannel::CallOnStartRequest()  
	 nsInputStreamPump::OnStateStart()  
	 nsInputStreamPump::OnInputStreamReady()  
	 nsInputStreamReadyEvent::EventHandler()  
	 PL_HandleEvent()  
	 PL_ProcessPendingEvents()  
	 _md_EventReceiverProc()  
	 HIToolbox.145.0.0 + 0x2330 (0x927d2330)  
	 HIToolbox.145.0.0 + 0x25a4 (0x927d25a4)  
	 HIToolbox.145.0.0 + 0x6a0c (0x927d6a0c)  
	 HIToolbox.145.0.0 + 0x130a4 (0x927e30a4)  
	 HIToolbox.145.0.0 + 0x23ec (0x927d23ec)  
	 HIToolbox.145.0.0 + 0x25a4 (0x927d25a4)  
	 HIToolbox.145.0.0 + 0x14a34 (0x927e4a34)  
	 HIToolbox.145.0.0 + 0x1892c (0x927e892c)  
	 HIToolbox.145.0.0 + 0x28b90 (0x927f8b90)  
	 HIToolbox.145.0.0 + 0x9100 (0x927d9100)  
	 HIToolbox.145.0.0 + 0x92b4 (0x927d92b4)  
	 HIToolbox.145.0.0 + 0x1ce68 (0x927ece68)  
	 HIToolbox.145.0.0 + 0x2db80 (0x927fdb80)  
	 nsMacMessagePump::GetEvent()  
	 nsMacMessagePump::DoMessagePump()  
	 nsAppShell::Run()  
	 mozilla-bin + 0xb674 (0x0000b674)  
	 mozilla-bin + 0xbbb8 (0x0000bbb8)  
	 mozilla-bin + 0x8724 (0x00008724)  
	 mozilla-bin + 0x85a4 (0x000085a4)   
 
     (408319)	URL: http://bootsnall.com
     (408319)	Comments: opening a gae to register


And a link to other crashes currently in the Talkback db:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsImgManager%3A%3ATestPermission

Comment 1

[[e-mail]]

http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsImgManager.cpp#301

probably aFirstURI is null. we should probably require aCurrentURI to be !null
on penalty of permission denied, and the same for aFirstURI but only when using
the foreign check.

mvl, want to patch? :)

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

I'd prefer allowing by default. All that can happen is seeing an image you
wanted to block. There won't be much harm in that. If block is default, you
might not be able a see a image you do want, and be not able to do anything
about it.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: nullcheck

Patch add a nullcheck for aFirstURI. aCurrentURI is already checked in
ShouldLoad. I don't think an additional check is needed, because the method is
internal. (I should remove nsIImgManager.idl one day. no need for it)

Comment 4

[Darin Fisher]

Comment on attachment 154749 [details] [diff] [review]
nullcheck

right, because aRequestOrigin in nsIContentPolicy::shouldLoad may be null.

Comment 5

[[e-mail]]

fixed-on-trunk

Comment 6

[[e-mail]]

Comment on attachment 154749 [details] [diff] [review]
nullcheck

easy topcrasher fix for aviary.

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 154749 [details] [diff] [review]
nullcheck

a=asa for aviary checkin

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think this needs to be checked into 1.7 branch to keep Aviary and 1.7 Gecko in
sync. Can you please make it so?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Uh... did anyone bother to check _why_ the requesting URI is null?  You just
rebroke bug 200433 with this patch, no?  The requesting URI there _is_ null, and
always will be; see
<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsImageDocument.cpp&rev=1.125&mark=180#178>.

Perhaps the image manager (or some other content policy code, possibly even in
nsContentPolicy?) should be smart enough to get a URI off the context in cases
when the requesting URI is null?  Putting code to do that in every caller seems
silly.

Comment 10

[Frank Wein [:mcsmurf]]

*** Bug 254038 has been marked as a duplicate of this bug. ***

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Whoever landed this on the aviary branch did so with different punctuation than
the trunk and the 1.7 branch.  This causes pain when synchronizing branches. 
Please use cvs to apply patches to branches, not hand merging or application of
potentially old patches.

Comment 12

[Worcester12345]

(In reply to comment #11)
> Whoever landed this on the aviary branch did so with different punctuation than
> the trunk and the 1.7 branch.  This causes pain when synchronizing branches. 
> Please use cvs to apply patches to branches, not hand merging or application of
> potentially old patches.

So, was this taken care of, or should it still be open?