Crash [@ nsTextFrame::Destroy] with <xul:arrowscrollbox>

VERIFIED FIXED

Status

()

Core
Layout: Text
--
critical
VERIFIED FIXED
11 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: roc)

Tracking

(Blocks: 1 bug, {assertion, crash, testcase})

Trunk
x86
Mac OS X
assertion, crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
Loading the testcase (Mac trunk debug Firefox) triggers an assertion:

###!!! ASSERTION: Gap or overlap in textframes mapping content?!: 'endOfLastContent == contentStart', file /Users/jruderman/trunk/mozilla/layout/generic/nsTextFrameThebes.cpp, line 1590

Leaving the testcase triggers a crash.  The stack trace depends on how I leave:


1) Quit the browser (Cmd+Q):

Thread 0 Crashed:
0   libthebes.dylib          	0x059f052f nsAutoRefCnt::operator unsigned() const + 9 (nsISupportsImpl.h:214)
1   libthebes.dylib          	0x059f2976 gfxTextRunFactory::Release() + 26 (gfxFont.h:419)
...


2) Reload after a while:

###!!! ASSERTION: Object is lying about its index: 'generation.Length() > index && generation[index] == aObj', file ../../dist/include/xpcom/nsExpirationTracker.h, line 153

Thread 0 Crashed:
0   libgklayout.dylib        	0x17b17117 nsTextFrame::ClearTextRun() + 95 (nsTextFrameThebes.cpp:3561)
1   libgklayout.dylib        	0x17b1781f nsTextFrame::Destroy() + 17 (nsTextFrameThebes.cpp:3249)
...


3) Reload immediately:

Thread 0 Crashed:
0   <<00000000>> 	0x00000000 0 + 0
1   libgklayout.dylib        	0x17b1781f nsTextFrame::Destroy() + 17 (nsTextFrameThebes.cpp:3249)
2   libgklayout.dylib        	0x17abe0f8 nsFrameList::DestroyFrames() + 48 (nsFrameList.cpp:68)
3   libgklayout.dylib        	0x17aa7649 nsContainerFrame::Destroy() + 73 (nsContainerFrame.cpp:255)
...
Flags: blocking1.9?
(Reporter)

Comment 1

11 years ago
Created attachment 280015 [details]
testcase (crashes Firefox when closed)
Assignee: nobody → roc
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical?]
The problem is that when we remove the text node from the DOM, the frame isn't removed because we can't find primary frame for the node. Here's the relevant fragment of the frame tree:

              Box(arrowscrollbox)(1)@0x24cae38 next=0x24cb44c {0,-60,0,0} [state=80c40000] [content=0x3dfe44c0] [sc=0x246a15c]<
                ButtonBoxFrame(autorepeatbutton)(-1)@0x24ab724 next=0x24cb114 {120,60,0,0} [state=80400000] [content=0x3dfe45d0] [sc=0x246a3e8]<
                  ImageBox(image)(-1)@0x24cafc0 {0,0,0,0} [content=0x3e41bcf0]
                >
                XULScroll(scrollbox)(-1)@0x24cb114 [view=0x3e407b70] next=0x24cb540 {0,0,0,0} [state=80c42000] [content=0x3dfe4740] [sc=0x24cb050]<
                  Box(scrollbox)(-1)@0x24cb0ac [view=0x3e407c10] {0,0,0,0} [state=80c02000] [content=0x3dfe4740] [sc=0x24cb208] pst=:-moz-scrolled-content<
                    Box(box)(-1)@0x24cb350 {0,0,0,0} [state=80c00000] [content=0x3e42de00] [sc=0x24cb2f4]<
                      Text(-1)@0x246a300[0,1,T]  {0,0,0,0} [content=0x3dfe4510] sc=0x24cb3fc pst=:-moz-non-element<
                        "\n"
                      >
                    >
                  >
                >
                ButtonBoxFrame(autorepeatbutton)(-1)@0x24cb540 {120,60,0,0} [state=80400000] [content=0x3dfe4770] [sc=0x24cb4dc]<
                  ImageBox(image)(-1)@0x24cb650 {0,0,0,0} [content=0x3e41bed0]
                >
              >

Relevant part of the call stack:

#0  nsCSSFrameConstructor::FindPrimaryFrameFor (this=0x3e424ca0, aFrameManager=0x239c41c, aContent=0x3dfe4510, aFrame=0xbfffa938, aHint=0x0) at /Users/roc/mozilla-checkin/mozilla/layout/base/nsCSSFrameConstructor.cpp:10796

But the relevant locals are:

(gdb) p parentFrame
$37 = (nsIFrame *) 0x24cae38
(gdb) p aContent
$38 = (nsIContent *) 0x3dfe4510
(gdb) p aContent->GetParent()
$39 = (nsIContent *) 0x3dfe44c0

So the text node's parent is the arrowscrollbox. But the FindPrimaryFrameFor code is expecting the parent to be the inner box (0x3e42de00). I can't remember whether nsIContent::GetParent() should return the parent in the real DOM or the shadow DOM... if the real DOM, then how is FindPrimaryFrameFor supposed to work for children of XBL insertion points?
For non-anonymous nodes, nsIContent::GetParent() returns the real DOM parent.

> how is FindPrimaryFrameFor supposed to work

parentContent ends up being the arrowscrollbox, so parentFrame is its primary frame.  Then we call FindFrameWithContent, and that steps down through kids that have parentContent as the GetBindingParent().  See patch for bug 54524.  So I'd expect this to work, yes...
OK, FindFrameWithContent is not finding the text node because it refuses to descend from the scrollbox into the box. The reason is this check:

          if (aParentContent == kidContent ||
              (aParentContent && (aParentContent == kidContent->GetBindingParent()))) 

Here, when kidContent is the scrollbox's child (0x24cb350), its binding parent is the scrollbox because of the nested binding. But aParentContent is the arrowscrollbox, so this check fails and we don't descend.

I think we could probably check whether aParentContent is a binding *ancestor* of kidContent. I'll make a patch to that effect.
Created attachment 281293 [details] [diff] [review]
fix

This fixes it and I think it makes sense.
Attachment #281293 - Flags: superreview?(bzbarsky)
Attachment #281293 - Flags: review?(bzbarsky)
Comment on attachment 281293 [details] [diff] [review]
fix

>Index: layout/base/nsCSSFrameConstructor.cpp
>+IsBindingAncestor(nsIContent* aContent, nsIContent* aBindingRoot)

>+    nsIContent* bindingParent = aContent->GetBindingParent();
>+    if (!bindingParent)
>+      return PR_FALSE;
>+    if (bindingParent == aBindingRoot)
>+      return PR_TRUE;
>+    aContent = bindingParent;

I think you want another clause there after the == check:

  if (bindingParent == aContent)
    return PR_FALSE;

Otherwise passing in a native-anonymous aContent will send you into an infinite loop, no?

r+sr=bzbarsky with that change.
Attachment #281293 - Flags: superreview?(bzbarsky)
Attachment #281293 - Flags: superreview+
Attachment #281293 - Flags: review?(bzbarsky)
Attachment #281293 - Flags: review+

Updated

11 years ago
Assignee: roc → nobody
Component: GFX: Thebes → Layout: Fonts and Text
QA Contact: thebes → layout.fonts-and-text

Updated

11 years ago
Assignee: nobody → roc
Flags: blocking1.9? → blocking1.9+
checked in with that change.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011009 Firefox/3.0b3pre ID:2008011009 while using the testcase. No Assertion or crash when loading the testcase -> Verified fixed
Status: RESOLVED → VERIFIED
(Reporter)

Comment 9

10 years ago
I don't see any assertions/crashes on branch with this testcase.
Group: security
Flags: wanted1.8.1.x-
Crash Signature: [@ nsTextFrame::Destroy]
You need to log in before you can comment on or make changes to this bug.