See upcoming testcase which crashes current trunk build after 100ms. http://crash-stats.mozilla.com/report/index/7650f69c-56a0-11dd-89d5-001a4bd43ef6?p=1 0 xul.dll nsImageLoader::RedrawDirtyFrame layout/base/nsImageLoader.cpp:215 1 xul.dll nsImageLoader::OnStopFrame layout/base/nsImageLoader.cpp:181 2 xul.dll nsGIFDecoder2::EndImageFrame modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp:413 3 xul.dll nsGIFDecoder2::GifWrite modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp:1160 4 xul.dll nsGIFDecoder2::ProcessData modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp:243 5 xul.dll ReadDataOut modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp:190 6 xul.dll nsInputStreamTee::WriteSegmentFun xpcom/io/nsInputStreamTee.cpp:102 7 xul.dll nsPipeInputStream::ReadSegments xpcom/io/nsPipe3.cpp:799 8 xul.dll nsInputStreamTee::ReadSegments xpcom/io/nsInputStreamTee.cpp:156 9 xul.dll nsGIFDecoder2::WriteFrom modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp:262 10 xul.dll imgRequest::OnDataAvailable modules/libpr0n/src/imgRequest.cpp:861 11 xul.dll nsBaseAppShell::NativeEventCallback widget/src/xpwidgets/nsBaseAppShell.cpp:131

The pres shell is destroyed and it called SetShell(nsnull) on the pres context, which will be destroyed soon too at which point the it will destroy all the nsImageLoaders (it owns). A null-check is needed to close the gap.

I wasn't able to make a testcase that crashes when run as -reftest from a file: URL (a.k.a. crashtest).

Actually, maybe it can be more than a null-pointer crash. If 'mReflowOnLoad' is false, we could be using a destroyed frame (mFrame) in the code that follows? http://hg.mozilla.org/mozilla-central/index.cgi/annotate/5d6b41375fd2/layout/base/nsImageLoader.cpp#l233

Comment on attachment 330562 [details] [diff] [review] Patch rev. 1 r+sr=dbaron, although I'm a little unsure if the mFrame = nsnull is needed. Might it be better to skip that? (Can the frame still exist at this point, and still get to its image loader via the pres context?)

Note that nsFrame::Destroy calls presContext->StopImagesFor(this), which should destroy the nsImageLoader in question.

Mats, can you address dbaron's concerns (if needed) and check this in?

This is worse that I first realized (I'm hiding the bug in case the fix does not make it into 3.1b1). The pres context can be destroyed at that point so 'shell' has an arbitrary value (the shell and frame is destroyed too).

There's several errors in nsPresContext in handling 'mBorderImageLoaders'. The primary cause of this crash is that when nsPresContext::DoLoadImage() creates image loaders for border images it adds them to wrong list (and "leaks" an existing image loader): http://hg.mozilla.org/mozilla-central/annotate/0a7fa17fe202/layout/base/nsPresContext.cpp#l1170 It uses 'aTable' for lookup but always adds to 'mImageLoaders', so when aTable==mBorderImageLoaders we will create a new loader and forget about an existing one. (I think there's also an opportunity to optimize DoStopImageFor() when destroying the whole shell - avoid doing it for each frame and just enumerate and clear the whole lists up front. Should save some lookups.)

Are any of these problems still present with the multiple image loaders patch in bug 322475 (which I'm planning to land in the next few days)?

(And I pointed out this problem in bug 322475 comment 39.)

Yes, bug 322475 fixes this crash. I have a couple of improvements locally - adding MOZ_COUNT_CTOR/DTOR in nsImageLoader (which I think would have flagged the leak) and a mPresContext->DestroyImageLoaders() call from PresShell::Destroy() which is an optimization but also allows us to assert if someone adds an image loader while the shell is destroyed (which seems like a bad idea). I'll wait for your landing and then suggest a new patch here.

nsImageLoader uses NS_IMPL_ISUPPORTS, so it shouldn't have MOZ_COUNT_CTOR/DTOR.

Ok, I was just a little surprised this problem wasn't flagged as a leak early on, but I haven't looked at the ownership of the loader in detail.

FWIW, I landed the relevant patch in bug 322475.

Resolving as fixed by bug 322475. Holding the crashtest since 3.1b1 is affected. Filed bug 459623 on the possible optimization. -> FIXED

I think I'm going to back out bug 322475 to fix bug 460796; when I do that I'll probably land this patch as well (under my review). Any objections to that plan?

Comment on attachment 342688 [details] [diff] [review] Patch rev. 2 (wip) r+sr=dbaron (for the nsPresContext.cpp parts)

No objections, please land as you see fit, but hold the crash test until after 3.1b2 is released.

Landed: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6fc233a8263d&tochange=16710698149f

Previous crashtest refers to the b.m.o image. This is an updated crashtest that uses a local image and wraps the original test in an <iframe> that reloads every 20ms for 700ms. It crashes reliably for me on x86-64 linux in a pre-fix build.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090122 Shiretoko/3.1b3pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090122 Minefield/3.2a1pre

Landed the crashtest: http://hg.mozilla.org/mozilla-central/rev/167990d03429 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e91c63699039

Making public as 3.1b2 has been released with the fix (bug was trunk/1.9.1 only)