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

Status: VERIFIED FIXED

(Core :: Layout, defect, P4)

Description

[Martijn Wargers (dead)]

Attachment: image needed for testcase

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

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

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.

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Mats Palmgren (inactive)]

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 5

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

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?)

Comment 6

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

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

Comment 7

[Jesse Ruderman]

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

Comment 8

[Mats Palmgren (inactive)]

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).

Comment 9

[Mats Palmgren (inactive)]

Attachment: crashtest.diff

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (wip)

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.)

Comment 11

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

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)?

Comment 12

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

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

Comment 13

[Mats Palmgren (inactive)]

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.

Comment 14

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

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

Comment 15

[Mats Palmgren (inactive)]

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.

Comment 16

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

FWIW, I landed the relevant patch in bug 322475.

Comment 17

[Mats Palmgren (inactive)]

Resolving as fixed by bug 322475.
Holding the crashtest since 3.1b1 is affected.

Filed bug 459623 on the possible optimization.

-> FIXED

Comment 18

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

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 19

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

Comment on attachment 342688 [details] [diff] [review]
Patch rev. 2 (wip)

r+sr=dbaron (for the nsPresContext.cpp parts)

Comment 20

[Mats Palmgren (inactive)]

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

Comment 21

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

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

Comment 22

[Mats Palmgren (inactive)]

Attachment: crashtest2.diff

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.

Comment 23

[Tony Chung [:tchung]]

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

Comment 24

[Mats Palmgren (inactive)]

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

Comment 25

[Mats Palmgren (inactive)]

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