269566 - [FIX]Crash after 2 or 3 reloads [@ nsCachedStyleData::GetStyleData]

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Jean-Marc]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041111 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041111 Firefox/0.9.1+

Talkback was unfortunately not available (why ?) at installation time.

Reproducible: Always
Steps to Reproduce:
1. Load http://www.tiscali.be/FR/home/home_center.asp .
2. Reload 2 or 3 times.

Actual Results:  
Crash.

Expected Results:  
No crash.

Firefox 1.0 displays the page without problem.

Comment 1

[Jean-Marc]

Firefox does not show anything from the page (blank). This page is hosted by my
ISP. I truly hope that they don't restrict its access to their IP range.
Otherwise I'm out of luck and I'll have to hope that someone else with the same
problem will provide a good testcase in another bug report.

Comment 2

[timeless]

reporter: you can try selecting custom install, in order to avoid flooding the
talkback servers, windows installer builds randomly decide not to install
talkback. that said, here's a stack:

>	gklayout.dll!nsCachedStyleData::GetStyleData(const nsStyleStructID &
aSID=eStyleStruct_Visibility)  Line 210 + 0x3	C++
 	gklayout.dll!nsStyleContext::GetStyleData(nsStyleStructID
aSID=eStyleStruct_Visibility)  Line 247 + 0xf	C++
 	gklayout.dll!nsIFrame::GetStyleData(nsStyleStructID
aSID=eStyleStruct_Visibility)  Line 616	C++
 	gklayout.dll!nsIFrame::GetStyleVisibility()  Line 98 + 0x11	C++
 	gklayout.dll!nsImageFrame::FrameChanged(imgIContainer * aContainer=0x035d2260,
gfxIImageFrame * aNewFrame=0x035f8480, nsRect * aDirtyRect=0x0012fc40)  Line
702 + 0x8	C++
 	gklayout.dll!nsImageListener::FrameChanged(imgIContainer *
aContainer=0x035d2260, gfxIImageFrame * newframe=0x035f8480, nsRect *
dirtyRect=0x0012fc40)  Line 2139	C++
 	gklayout.dll!nsImageLoadingContent::FrameChanged(imgIContainer *
aContainer=0x035d2260, gfxIImageFrame * aFrame=0x035f8480, nsRect *
aDirtyRect=0x0012fc40)  Line 160 + 0x4f	C++
 	imglib2.dll!imgRequestProxy::FrameChanged(imgIContainer *
container=0x035d2260, gfxIImageFrame * newframe=0x035f8480, nsRect *
dirtyRect=0x0012fc40)  Line 366	C++
 	imglib2.dll!imgRequest::FrameChanged(imgIContainer * container=0x035d2260,
gfxIImageFrame * newframe=0x035f8480, nsRect * dirtyRect=0x0012fc40)  Line 375	C++
 	imglib2.dll!imgContainerGIF::Notify(nsITimer * timer=0x0373b878)  Line 456	C++
 	xpcom_core.dll!nsTimerImpl::Fire()  Line 387	C++
 	xpcom_core.dll!nsTimerManager::FireNextIdleTimer()  Line 617	C++
 	gkwidget.dll!nsAppShell::Run()  Line 142	C++
 	appcomps.dll!nsAppStartup::Run()  Line 221	C++
 	mozilla.exe!main1(int argc=0x00000001, char * * argv=0x00347b80, nsISupports *
nativeApp=0x0109ef38)  Line 1321 + 0x20	C++
 	mozilla.exe!main(int argc=0x00000001, char * * argv=0x00347b80)  Line 1813 +
0x25	C++
 	mozilla.exe!mainCRTStartup()  Line 400 + 0x11	C
 	kernel32.dll!TermsrvAppInstallMode()  + 0x269	

+	info	{mCachedStyleDataOffset=0x00000000 mInheritResetOffset=0x00000010
mIsReset=0x00000000 }	const nsCachedStyleData::StyleStructInfo &
	info.mCachedStyleDataOffset	0x00000000	int
+	resetOrInherit	0x037499c0 "WôPT"	char *
+	resetOrInheritSlot	0xddddddf9 <Bad Ptr>	char *
+	this	0xddddddf9 {gInfo=0x01fec970 struct nsCachedStyleData::StyleStructInfo *
nsCachedStyleData::gInfo mInheritedData=??? mResetData=??? }	nsCachedStyleData *
const

Unhandled exception at 0x01b30e28 (gklayout.dll) in mozilla.exe: 0xC0000005:
Access violation reading location 0xddddddf9.

problem starts here:
>	gklayout.dll!nsImageListener::FrameChanged(imgIContainer *
aContainer=0x035d2260, gfxIImageFrame * newframe=0x035f8480, nsRect *
dirtyRect=0x0012fc40)  Line 2139	C++
+	aContainer	0x035d2260 {mRefCnt={mValue=0x00000002 }
_mOwningThread={mThread=0x00345278 } mObserver={mRawPtr=0x03622668
{mRefCount=0x00000001 mReferent=0x035b7d1c {mRefCnt={...} _mOwningThread={...}
mChannel={...} ...} } } ...}	imgIContainer *
+	dirtyRect	0x0012fc40 {x=0x00000000 y=0x00000000 width=0x000001d4 ...}	nsRect *
+	mFrame	0x0375a19c {mImageMap=0xdddddddd {mRefCnt={mValue=??? }
_mOwningThread={mThread=??? } mPresShell=??? ...} mListener={mRawPtr=0xdddddddd
} mComputedSize={width=0xdddddddd height=0xdddddddd } ...}	nsImageFrame *
+	newframe	0x035f8480 {mRefCnt={mValue=0x00000001 }
_mOwningThread={mThread=0x00345278 } mSize={width=0x000001d4 height=0x0000003c
} ...}	gfxIImageFrame *
+	this	0x0374ac08 {mRefCnt={mValue=0x00000002 }
_mOwningThread={mThread=0x00345278 } mFrame=0x0375a19c {mImageMap=0xdddddddd
{mRefCnt={mValue=??? } _mOwningThread={mThread=??? } mPresShell=??? ...}
mListener={mRawPtr=0xdddddddd } mComputedSize={width=0xdddddddd
height=0xdddddddd } ...} }	nsImageListener * const

Comment 3

[timeless]

fwiw there were no asserts of interest. debug build is trunk from probably about
the middle of last week. i can check later if people care.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Image for testcase

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

The basic problem is that ConstructHTMLFrame doesn't get a pseudo-parent, I
think.  I'll do more debugging tomorrow...

Comment 7

[timeless]

not that it really matters, but loading the testcase (by clicking edit from this
bug) and then clicking back in Camino crashed my poor Camino [Mozilla/5.0
(Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a5) Gecko/20041110 Camino/0.8+]

Comment 8

[Ryan VanderMeulen [:RyanVM]]

The testcase kills my 1.0 branch build as well.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041108
Firefox/1.0 (MOOX M2)

Comment 9

[Worcester12345]

(In reply to comment #8)
> The testcase kills my 1.0 branch build as well.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041108
> Firefox/1.0 (MOOX M2)

Ran reload about 10-12 times here with no problems. I didn't allow the popup
window to load.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Comment 10

[Boris Zbarsky [:bzbarsky]]

Guys, we know where the crash happens and why it happens.  It's nondeterministic
in release builds (so it'll only happen sometimes).  In a debug build it will
happen with 100% certainty when the testcase page is closed.

Please stop spamming the bug long enough for us to work on a fix, ok?  ;)

Comment 11

[Martijn Wargers (dead)]

Attachment: Testcase2

Some time ago, I also made a testcase of that ur, without knowing the existence
of this bug.
It's almost the same, but this one uses an iframe.
It crashes always for me on closing the tab.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So I've been thinking about this, and I don't see a simple solution offhand,
short of bailing out of ConstructHTMLFrame immediately if the parent is a table
frame... That would not show content on the testcase in question, but that's
better than crashing.

Here are the problems I see:

1)  We need to decide whether we have a pseudo-parent or not before constructing
    frames.
2)  If we create a pseudo-parent in ConstructHTMLFrame but we're not looking at
    an HTML frame, what happens?
3)  <img style="display: table-row"> -- what should this do?

#2 is what worries me as far as just hacking this goes...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

This fixes this bug, bug 278266, and bug 275625.

Basic changes:

1)  Hoist the pseudo-frame construction out of ConstructFrameByDisplayType and
    into ConstructFrameInternal so we do it for all frame types as needed
(fixes
    this bug and bug 278266).
2)  Make sure to construct pseudo-frames even for HTML frames that claim to be
    of a table display type if we happen to know that they won't get a
    table-internal frame (fixes bug 275625).

The rest is just propagating the damage around.  I added a |aHasPseudoParent|
arg to a few functions where it's currently unused but _should_ be used. I'd be
ok with removing those args for now, pending a better way of doing
pseudo-frames altogether.

I've run the regression tests on this, and verified that the fixes for bug
2479, bug 3037, etc. did not regress.

Comment 14

[Bernd]

Comment on attachment 171234 [details] [diff] [review]
Proposed patch

>the sate... 
state?

if (IsTableRelated(aParentFrame->GetType(), PR_FALSE) &&
+      (!IsTableRelated(aChildDisplay->mDisplay, PR_TRUE) ||

please explain this if statement as it is the heart of the patch and the
PR_FALSE and then PR_TRUE argument is not obvious (while correct I believe)

>// XXXbz somewhere here we should process pseudo frames if !aHasPseudoParent

does that mean we will crash if hit this code?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> state?

Yes.

> please explain this if statement

Will do.  I just lifted it from ConstructFrameByDisplayType, of course... ;)

> does that mean we will crash if hit this code?

I'm not actually sure. We may just end up with frames in the wrong places in the
frame tree.  But yes, we may lose frames altogether and leak and/or crash as a
result.

Comment 16

[Bernd]

Boris: could you check that we don't need something like
https://bugzilla.mozilla.org/attachment.cgi?id=170350 from bug 277035 at the if
statement that I asked you to better comment. In other words what happens if
these  html frames have display:none specified.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Bernd, there's a check for that right before the place where I put the
AdjustParentFrame call.  We never get to that if in the case when the display is
"none".

Comment 18

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Followup patch

I got the ordering of args to nsINodeInfo::Equals wrong, and this still
compiled because one of the args was a macro for 0.  I just checked this in,
with r=sicking.

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED; both testcase 1:
https://bugzilla.mozilla.org/attachment.cgi?id=165961 and testcase 2:
https://bugzilla.mozilla.org/attachment.cgi?id=169367 using build 2005-01-26-06
on Windows XP Seamonkey trunk builds.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Would be nice to figure out a way to crashtest this.

Comment 22

[Bob Clary [:bc] (inactive)]

layout/base/crashtests/269566-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3