Closed Bug 446328 Opened 12 years ago Closed 11 years ago

Crash [@ nsImageLoader::RedrawDirtyFrame] with document that has -moz-border-image and gets display: none

Categories

(Core :: Layout, defect, P4, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: martijn.martijn, Assigned: dbaron)

References

Details

(Keywords: crash, testcase, verified1.9.1, Whiteboard: [sg:critical] virtual call on random address)

Crash Data

Attachments

(3 files, 3 obsolete files)

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
Attached file testcase
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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.
Assignee: nobody → mats.palmgren
Status: NEW → ASSIGNED
Attachment #330562 - Flags: superreview?(dbaron)
Attachment #330562 - Flags: review?(dbaron)
I wasn't able to make a testcase that crashes when run as -reftest
from a file: URL (a.k.a. crashtest).
Flags: in-testsuite?
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:nse] null-pointer access
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
Whiteboard: [sg:nse] null-pointer access
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?)
Attachment #330562 - Flags: superreview?(dbaron)
Attachment #330562 - Flags: superreview+
Attachment #330562 - Flags: review?(dbaron)
Attachment #330562 - Flags: review+
Note that nsFrame::Destroy calls presContext->StopImagesFor(this), which should destroy the nsImageLoader in question.
Flags: blocking1.9.1?
Mats, can you address dbaron's concerns (if needed) and check this in?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P4
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).
Group: core-security
Whiteboard: [sg:critical] virtual call on random address
Attached patch crashtest.diff (obsolete) — Splinter Review
Attached patch Patch rev. 2 (wip) (obsolete) — Splinter Review
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.)
Attachment #330562 - Attachment is obsolete: true
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.
Depends on: 322475
Attachment #342688 - Attachment is obsolete: true
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
Assignee: mats.palmgren → dbaron
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
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)
Attachment #342688 - Flags: superreview+
Attachment #342688 - Flags: review+
No objections, please land as you see fit, but hold the crash test until
after 3.1b2 is released.
Attached patch crashtest2.diffSplinter Review
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.
Attachment #342687 - Attachment is obsolete: true
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
Status: RESOLVED → VERIFIED
Making public as 3.1b2 has been released with the fix (bug was trunk/1.9.1 only)
Group: core-security
Flags: wanted1.9.0.x-
Crash Signature: [@ nsImageLoader::RedrawDirtyFrame]
You need to log in before you can comment on or make changes to this bug.