396613 - Crash [@gklayout!nsTableFrame::GetFrameAtOrBefore]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Paul Nickerson]

Attachment: testcase

I couldn't reduce the testcase anymore so I have no idea why this crashes :(

Firefox version:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070812 BonEcho/2.0.0.6

Details:
eax=03de601c ebx=7ffdf000 ecx=03de601c edx=00000000 esi=00a07920 edi=00011970
eip=01b282a0 esp=0012ded8 ebp=0012dee4 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000202
*** WARNING: Unable to verify checksum for C:\mozilla\mozilla\firefox-debug\dist\bin\components\gklayout.dll
gklayout!nsTableFrame::GetFrameAtOrBefore+0x20:
01b282a0 ff92e8000000    call    dword ptr [edx+0E8h] ds:0023:000000e8=????????


Disassembly:
gklayout!nsTableFrame::GetFrameAtOrBefore+0x20:
01b282a0 ff92e8000000    call    dword ptr [edx+0E8h]
01b282a6 394510          cmp     dword ptr [ebp+10h],eax
01b282a9 7505            jne     gklayout!nsTableFrame::GetFrameAtOrBefore+0x30 (01b282b0)
01b282ab 8b450c          mov     eax,dword ptr [ebp+0Ch]
01b282ae eb4e            jmp     gklayout!nsTableFrame::GetFrameAtOrBefore+0x7e (01b282fe)
01b282b0 c745f400000000  mov     dword ptr [ebp-0Ch],0
01b282b7 6a00            push    0
01b282b9 8b4508          mov     eax,dword ptr [ebp+8]

Stack trace:
gklayout!nsTableFrame::GetFrameAtOrBefore(
                        class nsIFrame * aParentFrame = 0x039cf86c, 
                        class nsIFrame * aPriorChildFrame = 0x03de601c, 
                        class nsIAtom * aChildType = 0x0167ccc8)
gklayout!nsTableRowFrame::InsertFrames(
                        class nsIAtom * aListName = 0x00000000, 
                        class nsIFrame * aPrevFrame = 0x03de601c, 
                        class nsIFrame * aFrameList = 0x039cf8c8)
gklayout!nsFrameManager::InsertFrames(
                        class nsIFrame * aParentFrame = 0x039cf86c, 
                        class nsIAtom * aListName = 0x00000000, 
                        class nsIFrame * aPrevFrame = 0x03de601c, 
                        class nsIFrame * aFrameList = 0x039cf8c8)
gklayout!nsCSSFrameConstructor::ContentInserted(
                        class nsIContent * aContainer = 0x03dd4c30, 
                        class nsIFrame * aContainerFrame = 0x00000000, 
                        class nsIContent * aChild = 0x03dd3ca8, 
                        int aIndexInContainer = 1, 
                        class nsILayoutHistoryState * aFrameState = 0x03acdc48, 
                        int aInReinsertContent = 0)
gklayout!nsCSSFrameConstructor::RecreateFramesForContent(
                        class nsIContent * aContent = 0x03dd3ca8)
gklayout!nsCSSFrameConstructor::ProcessRestyledFrames(
                        class nsStyleChangeList * aChangeList = 0x0012e284)
gklayout!nsIPresShell::ReconstructStyleDataInternal(void)
gklayout!nsIPresShell::ReconstructStyleData(void)
gklayout!PresShell::EndUpdate(
                        class nsIDocument * aDocument = 0x035f0fb0, 
                        unsigned int aUpdateType = 2)
gklayout!nsDocument::EndUpdate(
                        unsigned int aUpdateType = 2)
gklayout!nsStyleLinkElement::UpdateStyleSheet(
                        class nsIDocument * aOldDocument = 0x035f0fb0, 
                        class nsICSSLoaderObserver * aObserver = 0x00000000)
gklayout!nsHTMLStyleElement::UnbindFromTree(
                        int aDeep = 1, 
                        int aNullParent = 1)
gklayout!doRemoveChildAt(
                        unsigned int aIndex = 7, 
                        int aNotify = 1, 
                        class nsIContent * aKid = 0x033d9a40, 
                        class nsIContent * aParent = 0x0256e2d0, 
                        class nsIDocument * aDocument = 0x035f0fb0, 
                        class nsAttrAndChildArray * aChildArray = 0x0256e2e8)
gklayout!nsGenericElement::RemoveChildAt(
                        unsigned int aIndex = 7, 
                        int aNotify = 1)
gklayout!nsGenericElement::RemoveChild(
                        class nsIDOMNode * aOldChild = 0x033d9a5c, 
                        class nsIDOMNode ** aReturn = 0x0012ea68)
gklayout!nsHTMLHeadElement::RemoveChild(
                        class nsIDOMNode * aOldChild = 0x033d9a5c, 
                        class nsIDOMNode ** aReturn = 0x0012ea68)
xpcom_core!XPTC_InvokeByIndex(
                        class nsISupports * that = 0x0256e2ec, 
                        unsigned int methodIndex = 0x11, 
                        unsigned int paramCount = 2, 
                        struct nsXPTCVariant * params = 0x0012ea58)
xpc3250!XPCWrappedNative::CallMethod(
                        class XPCCallContext * ccx = 0x0012ebd4, 
                        XPCWrappedNative::CallMode mode = CALL_METHOD (0))
xpc3250!XPC_WN_CallMethod(
                        struct JSContext * cx = 0x033008c0, 
                        struct JSObject * obj = 0x02c39ce8, 
                        unsigned int argc = 1, 
                        long * argv = 0x03dc9cdc, 
                        long * vp = 0x0012ed34)
js3250!js_Invoke(
                        struct JSContext * cx = 0x033008c0, 
                        unsigned int argc = 1, 
                        unsigned int flags = 0)

Comment 1

[Martijn Wargers (dead)]

This was fixed somehow between 2005-11-05 and 2005-11-08±
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-11-05+06&maxdate=2005-11-08+08&cvsroot=%2Fcvsroot
I guess somehow fixed by bug 314776, perhaps?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Could be, yes...  Maybe we should take that on branch; it didn't seem to cause much in the way of hard-to-fix and serious regressions....

Comment 3

[Daniel Veditz [:dveditz]]

aPriorChildFrame appears to be a deleted object, crashes here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/tables/nsTableFrame.cpp&rev=3.616.6.12&mark=4360#4352

Comment 4

[Martijn Wargers (dead)]

I just tried the patch for bug 314776, it indeed seems to fix this crash.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I won't be able to get to this until I get back to Chicago in January; I don't have a branch tree here (nor disk space to set one up)...  What does the 1.8.1.12 schedule look like?

Comment 6

[Martijn Wargers (dead)]

Perhaps I can help? The problem with the patch in bug 314776 is that it's changing uuid's, which isn't allowed on branch, afaik.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, indeed.

I think we can avoid that if we avoid adding the debug verification function on branch; we're not changing the vtable in that case.  We'd want the External method to still be sync, I think.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch fix

I hunted down some disk space.

This patch fixes this bug, bug 394337, and of course bug 314776 (it's just a backport of the patch for bug 314776 to the branch).

The fix is a little different from trunk.  Specifically, it doesn't change the behavior of ReconstructStyleDataExternal, which did change on trunk.  It _does_ change the behavior of ReconstructStyleDataInternal.  I think it's reasonable to do that without changing the nsIPresShell IID or adding a new interface, since it won't change nsIPreShell consumers outside libgklayout.  But I can do the whole thing with adding an interface if desired.

Comment 9

[Martijn Wargers (dead)]

Boris, sorry for not having done this and thanks for taking this.

Comment 10

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

Comment on attachment 294993 [details] [diff] [review]
Branch fix

r+sr=dbaron.  Looks like all the ReconstructStyleDataInternal callers are in nsPresShell itself, so this is only changing what we do for adding removing sheets or rules.  So that doesn't seem too scary to me (although I'm sure a bunch of sites actually do do it).

r+sr=dbaron

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 294993 [details] [diff] [review]
Branch fix

Yeah, exactly.  This just affects sheet/rule removal+addition, and if the site asks for the style data we'll do the restyle at that point.  So this is pretty safe.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 294993 [details] [diff] [review]
Branch fix

approved for 1.8.1.12, a=dveditz for release-drivers

Comment 13

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Verified for Branch in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre.

Comment 15

[Alexander Sack]

Comment on attachment 294993 [details] [diff] [review]
Branch fix

a=asac for 1.8.0.15

(unmodified distro patch)

Comment 16

[Alexander Sack]

Note: bug #279505 fixes a regression from this.

Comment 17

[Reed Loden [:reed]]

Attachment: 1.8.0 backport

Comment 18

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in layout/base/nsIPresShell.h;
/cvsroot/mozilla/layout/base/nsIPresShell.h,v  <--  nsIPresShell.h
new revision: 3.158.4.3.4.4; previous revision: 3.158.4.3.4.3
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.852.2.11.2.13; previous revision: 3.852.2.11.2.12
done

Comment 19

[Alex Vincent [:WeirdAl]]

I've positively identified this bug (1.8 branch) as responsible for a regression in our Skyfire product, using our custom "Smart Fit" capability.  I don't yet understand how, precisely, but the regression manifested when we followed a few steps in order:

* Turn on Smart Fit.
* Go to money.cnn.com and let it load.
* Visit another page and let that load.
* Hit the Back button.

The result was a seriously fragged webpage (large grey boxes covering most of the content).

I'm wondering if anyone can help me in the analysis of this, to help me figure out how this regression (plus our modifications to layout to support Smart Fit) could have occured.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Well, the first place to look is whether your Smart Fit thing or some other layout change you have calls the functions involved in this patch.

Comment 21

[Alex Vincent [:WeirdAl]]

As far as I can tell, no - but these functions do get called for every CSS style sheet inserted.  The code which applies our Smart Fit XBL bindings does so by inserting a stylesheet tied to our custom CSS selectors and a XBL binding...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ah.  You might want to bring this up in .style so we can talk about it without cluttering this bug?