271151 - [FIX]###!!! ASSERTION: RemovedAsPrimaryFrame called after PreDestroy: 'PR_FALSE', file r:/mozilla/layout/html/forms/src/nsTextControlFrame.cpp, line 1436

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[timeless]

@see bug 244454 

steps:
edit mozilla.org/
select all
delete

###!!! ASSERTION: bad action nesting!: 'mActionNesting>0', file
r:/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 388
Break: at file r:/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 388
###!!! ASSERTION: bad action nesting!: 'mActionNesting>0', file
r:/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 388
Break: at file r:/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 388
###!!! ASSERTION: bad action nesting!: 'mActionNesting>0', file
r:/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 388
Break: at file r:/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp, line 388
CSS Error (chrome://editor/content/editor.xul :0.22): Expected color but found
'mixed'.  Error in parsing value for property 'background-color'.  Declaration
dropped.
###!!! ASSERTION: RemovedAsPrimaryFrame called after PreDestroy: 'PR_FALSE',
file r:/mozilla/layout/html/forms/src/nsTextControlFrame.cpp, line 1436
Break: at file r:/mozilla/layout/html/forms/src/nsTextControlFrame.cpp, line 1436

 	xpcom_core.dll!nsDebug::Assertion(const char * aStr=0x02116f40, const char *
aExpr=0x02116f34, const char * aFile=0x02116efc, int aLine=0x0000059c)  Line 109	C++
>	gklayout.dll!nsTextControlFrame::RemovedAsPrimaryFrame(nsPresContext *
aPresContext=0x03aa34d0)  Line 1436 + 0x25	C++
 	gklayout.dll!DoDeletingFrameSubtree(nsPresContext * aPresContext=0x03aa34d0,
nsIPresShell * aPresShell=0x042d1150, nsFrameManager * aFrameManager=0x042d116c,
nsVoidArray & aDestroyQueue={...}, nsIFrame * aRemovedFrame=0x0434fbb8, nsIFrame
* aFrame=0x03532814)  Line 9353	C++
 	gklayout.dll!DoDeletingFrameSubtree(nsPresContext * aPresContext=0x03aa34d0,
nsIPresShell * aPresShell=0x042d1150, nsFrameManager * aFrameManager=0x042d116c,
nsVoidArray & aDestroyQueue={...}, nsIFrame * aRemovedFrame=0x0434fbb8, nsIFrame
* aFrame=0x034a767c)  Line 9396 + 0x1d	C++
 	gklayout.dll!DoDeletingFrameSubtree(nsPresContext * aPresContext=0x03aa34d0,
nsIPresShell * aPresShell=0x042d1150, nsFrameManager * aFrameManager=0x042d116c,
nsVoidArray & aDestroyQueue={...}, nsIFrame * aRemovedFrame=0x0434fbb8, nsIFrame
* aFrame=0x034a74a0)  Line 9396 + 0x1d	C++
 	gklayout.dll!DoDeletingFrameSubtree(nsPresContext * aPresContext=0x03aa34d0,
nsIPresShell * aPresShell=0x042d1150, nsFrameManager * aFrameManager=0x042d116c,
nsVoidArray & aDestroyQueue={...}, nsIFrame * aRemovedFrame=0x0434fbb8, nsIFrame
* aFrame=0x0349c358)  Line 9396 + 0x1d	C++
 	gklayout.dll!DoDeletingFrameSubtree(nsPresContext * aPresContext=0x03aa34d0,
nsIPresShell * aPresShell=0x042d1150, nsFrameManager * aFrameManager=0x042d116c,
nsVoidArray & aDestroyQueue={...}, nsIFrame * aRemovedFrame=0x0434fbb8, nsIFrame
* aFrame=0x0434fbb8)  Line 9396 + 0x1d	C++
 	gklayout.dll!DeletingFrameSubtree(nsPresContext * aPresContext=0x03aa34d0,
nsIPresShell * aPresShell=0x042d1150, nsFrameManager * aFrameManager=0x042d116c,
nsIFrame * aFrame=0x0434fbb8)  Line 9438 + 0x1d	C++
 	gklayout.dll!nsCSSFrameConstructor::ContentRemoved(nsPresContext *
aPresContext=0x03aa34d0, nsIContent * aContainer=0x0432e830, nsIContent *
aChild=0x0433fb28, int aIndexInContainer=0x00000000, int
aInReinsertContent=0x00000000)  Line 9669 + 0x15	C++
 	gklayout.dll!PresShell::ContentRemoved(nsIDocument * aDocument=0x03a755a0,
nsIContent * aContainer=0x0432e830, nsIContent * aChild=0x0433fb28, int
aIndexInContainer=0x00000000)  Line 5164	C++
 	gklayout.dll!nsDocument::ContentRemoved(nsIContent * aContainer=0x0432e830,
nsIContent * aChild=0x0433fb28, int aIndexInContainer=0x00000000)  Line 2114	C++
 	gklayout.dll!nsHTMLDocument::ContentRemoved(nsIContent *
aContainer=0x0432e830, nsIContent * aContent=0x0433fb28, int
aIndexInContainer=0x00000000)  Line 1161	C++
 	gklayout.dll!nsGenericElement::RemoveChildAt(unsigned int aIndex=0x00000000,
int aNotify=0x00000001)  Line 2643	C++
 	gklayout.dll!nsGenericElement::doRemoveChild(nsIContent * aElement=0x0432e830,
nsIDOMNode * aOldChild=0x0433fb44, nsIDOMNode * * aReturn=0x0012e8f4)  Line
3112 + 0x11	C++
 	gklayout.dll!nsGenericElement::RemoveChild(nsIDOMNode * aOldChild=0x0433fb44,
nsIDOMNode * * aReturn=0x0012e8f4)  Line 503 + 0x11	C++
 	gklayout.dll!nsHTMLBodyElement::RemoveChild(nsIDOMNode * aOldChild=0x0433fb44,
nsIDOMNode * * aReturn=0x0012e8f4)  Line 99 + 0x14	C++
 	editor.dll!DeleteElementTxn::DoTransaction()  Line 120 + 0x4d	C++
 	editor.dll!EditAggregateTxn::DoTransaction()  Line 65 + 0x17	C++
 	editor.dll!DeleteRangeTxn::DoTransaction()  Line 169 + 0x9	C++
 	editor.dll!EditAggregateTxn::DoTransaction()  Line 65 + 0x17	C++
 	txmgr.dll!nsTransactionItem::DoTransaction()  Line 180 + 0x12	C++
 	txmgr.dll!nsTransactionManager::BeginTransaction(nsITransaction *
aTransaction=0x044e68c0)  Line 1071 + 0xb	C++
 	txmgr.dll!nsTransactionManager::DoTransaction(nsITransaction *
aTransaction=0x044e68c0)  Line 132 + 0x12	C++
 	editor.dll!nsEditor::DoTransaction(nsITransaction * aTxn=0x044e68c0)  Line
544 + 0x1e	C++
 	editor.dll!nsEditor::DeleteSelectionImpl(short aAction=0x0001)  Line 4443 +
0x10	C++
 	editor.dll!nsHTMLEditRules::WillDeleteSelection(nsISelection *
aSelection=0x034547d8, short aAction=0x0001, int * aCancel=0x0012efac, int *
aHandled=0x0012ef6c)  Line 2253 + 0x20	C++
 	editor.dll!nsHTMLEditRules::WillDoAction(nsISelection * aSelection=0x034547d8,
nsRulesInfo * aInfo=0x0012ef70, int * aCancel=0x0012efac, int *
aHandled=0x0012ef6c)  Line 592 + 0x1f	C++
 	editor.dll!nsPlaintextEditor::DeleteSelection(short aAction=0x0001)  Line
872 + 0x3b	C++
 	editor.dll!nsTextEditorKeyListener::KeyPress(nsIDOMEvent *
aKeyEvent=0x044e6768)  Line 217	C++
 	gklayout.dll!DispatchToInterface(nsIDOMEvent * aEvent=0x044e6768,
nsIDOMEventListener * aListener=0x0378da80, unsigned int (nsIDOMEvent *)*
aMethod=0x01d5b0d0, const nsID & aIID={...}, int * aHasInterface=0x0012f0a4) 
Line 128 + 0xb	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext *
aPresContext=0x03aa34d0, nsEvent * aEvent=0x0012f65c, nsIDOMEvent * *
aDOMEvent=0x0012f2c8, nsIDOMEventTarget * aCurrentTarget=0x03a75648, unsigned
int aFlags=0x00000202, nsEventStatus * aEventStatus=0x0012f43c)  Line 1609 +
0x23	C++
 	gklayout.dll!nsDocument::HandleDOMEvent(nsPresContext *
aPresContext=0x03aa34d0, nsEvent * aEvent=0x0012f65c, nsIDOMEvent * *
aDOMEvent=0x0012f2c8, unsigned int aFlags=0x00000202, nsEventStatus *
aEventStatus=0x0012f43c)  Line 3828	C++
 	gklayout.dll!nsGenericElement::HandleDOMEvent(nsPresContext *
aPresContext=0x03aa34d0, nsEvent * aEvent=0x0012f65c, nsIDOMEvent * *
aDOMEvent=0x0012f2c8, unsigned int aFlags=0x00000207, nsEventStatus *
aEventStatus=0x0012f43c)  Line 2036 + 0x2e	C++
 	gklayout.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0012f65c,
nsIView * aView=0x039f8248, unsigned int aFlags=0x00000001, nsEventStatus *
aStatus=0x0012f43c)  Line 5984 + 0x3a	C++
 	gklayout.dll!PresShell::HandleEvent(nsIView * aView=0x039f8248, nsGUIEvent *
aEvent=0x0012f65c, nsEventStatus * aEventStatus=0x0012f43c, int
aForceHandle=0x00000001, int & aHandled=0x00000001)  Line 5814 + 0x19	C++
 	gklayout.dll!nsViewManager::HandleEvent(nsView * aView=0x039f8248, nsGUIEvent
* aEvent=0x0012f65c, int aCaptured=0x00000000)  Line 2348	C++
 	gklayout.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0012f65c,
nsEventStatus * aStatus=0x0012f5a4)  Line 2121 + 0x14	C++
 	gklayout.dll!HandleEvent(nsGUIEvent * aEvent=0x0012f65c)  Line 166	C++
 	gkwidget.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0012f65c,
nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 1074 + 0xa	C++
 	gkwidget.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0012f65c) 
Line 1095	C++
 	gkwidget.dll!nsWindow::DispatchKeyEvent(unsigned int aEventType=0x00000083,
unsigned short aCharCode=0x0000, unsigned int aVirtualCharCode=0x0000002e, long
aKeyData=0x01530001)  Line 3003 + 0xf	C++
 	gkwidget.dll!nsWindow::OnKeyDown(unsigned int aVirtualKeyCode=0x0000002e,
unsigned int aScanCode=0x00000153, long aKeyData=0x01530001)  Line 3129	C++
 	gkwidget.dll!nsWindow::ProcessMessage(unsigned int msg=0x00000100, unsigned
int wParam=0x0000002e, long lParam=0x01530001, long * aRetValue=0x0012fbb8) 
Line 3967 + 0x1d	C++
 	gkwidget.dll!nsWindow::WindowProc(HWND__ * hWnd=0x11c61e3e, unsigned int
msg=0x00000100, unsigned int wParam=0x0000002e, long lParam=0x01530001)  Line
1355 + 0x1b	C++
 	user32.dll!77d43a50() 	
 	user32.dll!77d43b1f() 	
 	user32.dll!GetMessageW()  + 0x125	
 	user32.dll!DispatchMessageW()  + 0xb	
 	appcomps.dll!nsAppStartup::Run()  Line 216	C++
 	mozilla.exe!main1(int argc=0x00000001, char * * argv=0x00347b88, nsISupports *
nativeApp=0x01106140)  Line 1321 + 0x20	C++
 	mozilla.exe!main(int argc=0x00000001, char * * argv=0x00347b88)  Line 1813 +
0x25	C++
 	mozilla.exe!mainCRTStartup()  Line 400 + 0x11	C
 	kernel32.dll!TermsrvAppInstallMode()  + 0x269	

fwiw this is non fatal

Comment 1

[Boris Zbarsky [:bzbarsky]]

Minimal testcase needed.

Comment 2

[timeless]

1. load http://www.lauffen.de/portal
ASSERT

Comment 3

[Boris Zbarsky [:bzbarsky]]

Minimal testcase would be nice... all I can tell from the stack is we're moving
stuff about in the DOM.

Comment 4

[Martijn Wargers (dead)]

Attachment: Testcase

This still asserts for me in my debug build.

Comment 5

[Martijn Wargers (dead)]

Attachment: Testcase

The previous one had a js bug in it.

Comment 6

[Martijn Wargers (dead)]

Oh, the document.body.appendChild(x); part is not necessary to trigger the assert.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Martijn, thank you!

What happens here is that we're removing the containing block of an absolutely
positioned frame.  As a result of the subtree munging we do for out-of-flows,
we were calling DoDeletingFrameSubtree() twice on the subtree rooted at the
child abs pos frame (once when processing the placeholder, and once when
processing the abs pos kid itself).

This change makes us not do that, but perhaps we should keep part of the old
behavior and skip calling DoDeletingFrameSubtree() altogether in this case
(instead of calling it on the placeholder like this patch makes us do).

Comment 8

[rbs]

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

Registering my interest to review this. It might take me a few days to get to
it however. As it is not a harmful assertion, that shouldn't be a problem.

Comment 9

[rbs]

The outOfFlowFrame frame @0328C96C is to be deleted, thus two things happen:
                         ^^^^^^^^^
1/ Its primary child list is walked: @0328CAF0, leading to @0328CA9C
                                     ---------             =========

2/ Its absolute child list is walked: @0328CA9C is walked again...
                                      =========


ContentRemoved: childFrame=Area(div)(-1)@0328C96C
                                        ^^^^^^^^^
Area(html)(-1)@0328C1C4 <
  line 0328C3A4: <
    Block(body)(1)@0328C33C <
      line 032988D8: <
        Text(0)@0328C85C <
          "\n\n"
        >
        Placeholder(div)(-1)@0328C9C0 outOfFlowFrame=Area(div)(-1)@0328C96C
      >                                                           ^^^^^^^^^
    >
  >
  Absolute-list<
    Area(div)(-1)@0328C96C <
                 ^^^^^^^^^
      line 032988A8: <
        Placeholder(div)(0)@0328CAF0 outOfFlowFrame=Area(div)(0)@0328CA9C
                           ---------                            =========
      >
      Absolute-list<
        Area(div)(0)@0328CA9C <
                    =========
          line 03298878: <
            nsTextControlFrame@0328CC9C <
              XULScroll(div)(-1)@032984C8 <
                ScrollBox(div)(-1)@0329858C <
                  Area(div)(-1)@0329874C <
                    line 0329883C: <
                      Frame(br)(0)@03298810
                    >
                  >
                >
              >
            >
          >
        >
      >
    >
  >
>

==============================
So bz's patch does precisely what it says:

"This change makes us not do that, but perhaps we should keep part of the old
behavior and skip calling DoDeletingFrameSubtree() altogether in this case
(instead of calling it on the placeholder like this patch makes us do)."

With the caveat that it goes into the placeholder, as bz pointed out. I am not
much concerned about this. But you can resolve it if you feel that it makes the
code easier to understand.

As I was tracing, my suprise came instead from the fact that it was the
outOfFlowFrame frame @0328C96C that was initially passed at the beginning. It
seemed to me that one should pass the original placeholder @0328C9C0 because the
purpose of DeleteFrameSubtree is precisely to delete these outOfFlow frames
(nevermind the somewhat misleading name |DeleteFrameSubtree|...).

If the code was passing the placeholder @0328C9C0, we wouldn't have this issue
at all. I am under the impression that's what people have assumed so far, too.

Comment 10

[rbs]

I should have started my previous comment by stating that I was curious about
this bug, and dumped the frame tree (see it). And what I saw corroborated bz's
analysis, then my comments...

Comment 11

[rbs]

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

r=rbs, safest patch.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> It seemed to me that one should pass the original placeholder @0328C9C0 because
> the purpose of DeleteFrameSubtree is precisely to delete these outOfFlow frames

The purpose is to destroy the frame subtree rooted at the frame passed in,
whatever that is...  and if this subtree contains placeholders to make sure that
the corresponding out-of-flows are also destroyed.

Comment 13

[rbs]

bz, sure, I understand that. But to be precise again, it does not destroy the
subtree, it instead deletes/cleans-up indirect references/things that the
subtree may hold onto (which include the out-of-flow-frames). (And then, then
subtree gets subsequently destroyed on return by the callers at their respective
call-sites.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed.