Jesse Ruderman Reporter
	Description • 17 years ago

Loading the testcase in a Mac trunk debug build triggers several assertions and a crash.  The last two assertions are:

###!!! ASSERTION: null check on startContent should be sufficient to null check nodeContent as well, since if nodeContent is for the root, startContent (which is before it) must be too: 'nodeContent || !startContent', file /Users/jruderman/trunk/mozilla/layout/base/nsCounterManager.cpp, line 145

###!!! ASSERTION: The possible descendant is null!: 'aPossibleDescendant', file /Users/jruderman/trunk/mozilla/content/base/src/nsContentUtils.cpp, line 1175

Thread 0 Crashed:
0   nsINode::GetNodeParent (nsINode.h:494)
1   nsContentUtils::ContentIsDescendantOf (nsContentUtils.cpp:1181)
2   nsCounterList::SetScope (nsCounterManager.cpp:148)
3   nsCounterList::RecalcAll (nsCounterManager.cpp:176)
4   RecalcDirtyLists (nsCounterManager.cpp:275)
...

The crash is a null-deref for me, but I'm filing this bug as security-sensitive because of bug 383129 comment 10.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 17 years ago

This should depend on the bug about mathml doing style system stuff at unsafe times.  Don't have the bug# offhand.

	Jesse Ruderman Reporter
	Comment 2 • 17 years ago

Still crashes now even though bug 355548 is fixed.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 17 years ago

We have a frame in the frame tree for a node that's not in the tree: the <munder>.  It's stuck on an overflow list:

                  Frame(mo)(2)@0xb022b84c next=0xb022b628 {0,-1080,0,2760} [state=00011008] [content=0xb0228d00] [overflow=0,0,5820,2760] [sc=0xb022b51c]<
                    Block(mo)(2)@0xb022bd10 {0,0,0,2760} [state=02d01008] [overflow=0,0,5820,1380] sc=0xb022bdb8(i=1,b=0) pst=:-moz-mathml-anonymous-block<
                      line 0xb022bd68: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x4120] {0,0,5820,1380} <
                        Inline(malignmark)(0)@0xb022b97c next-continuation=0x888d34c {0,0,5820,1380} [content=0xb0228d48] [sc=0xb022bc64]<
                          Frame(msubsup)(0)@0xb022ba04 {0,0,5820,1380} [state=00010000] [content=0xb0228d70] [sc=0xb027918c]<>
                        >
                      >
                      Overflow-list<
                        Inline(malignmark)(0)@0x888d34c next=0xb022a5e0 prev-continuation=0xb022b97c next-continuation=0xb022a5e0 {0,1380,5820,1380} [state=00000404] [content=0xb0228d48] [sc=0xb022bc64]<
                          Frame(munderover)(1)@0xb022ba7c {0,0,5820,1380} [state=00010000] [content=0xb0228db8] [sc=0xb027918c]<>
                        >
                        Overflow-list<
                          Frame(munder)(-1)@0xb022bb44 {0,0,5820,1380} [state=00010000] [content=0xb0228de0] [sc=0xb022b9b4]<>
                        >
                        Inline(malignmark)(0)@0xb022a5e0 prev-continuation=0x888d34c {0,0,0,0} [state=00000406] [content=0xb0228d48] [sc=0xb022bc64]<>
                      >
                    >
                  >

I'm not sure why the frame didn't get removed when the content was removed from the tree here, but that's bad.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 4 • 17 years ago

The first thing that's wrong here is that we shouldn't be doing vertical breaking. It's happening because MathML likes to set availableHeight to ComputedHeight() sometimes, totally bogus. I already fixed one instance of this in nsMathMLContainerFrame, here are the others that I found.

This alone shouldn't cause crashes though, we should be able to find and delete frames in overflow lists when their content goes away. I'll keep looking for that bug.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 5 • 17 years ago

nsContainerFrame::RemoveFrame just doesn't look in the overflow list. It should.

This patch also silences an assertion when malformed MathML content is looking for a MathML ancestor and discovers that the viewport has no associated content. (Which is bogus but I don't want to fix that now.)

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 17 years ago

Comment on attachment 296896 [details] [diff] [review]
fix

Much better!

	Jesse Ruderman Reporter
	Comment 7 • 17 years ago

Vlad Sukhoy has a different change to the "dangling frame without a content node" assertion condition in bug 400475.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 8 • 17 years ago

His is better, I'll take mine out before checking in.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 9 • 17 years ago

checked in with crashtest.

	Jesse Ruderman Reporter
	Comment 10 • 17 years ago

Crashes my 1.8 branch debug build [@ nsIFrame::GetStateBits].  It's a null deref for me, but see the last paragraph of comment 0.

	Daniel Veditz [:dveditz]
	Comment 11 • 16 years ago

Does this patch work for the 1.8 branch?

	Daniel Veditz [:dveditz]
	Comment 12 • 16 years ago

roc: are we going to fix this one on the branch "to be safe" or should we go ahead and unhide the bug as a non-exploitable problem?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 13 • 16 years ago

I don't think we should open this bug while it's an issue on branch.

The patch in comment #4 could be taken on branch, it's very safe. Dunno if that fixes the bug though.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 16 years ago

It doesn't.

The other patch is riskier and nontrivial to backport to branch.

	Daniel Veditz [:dveditz]
	Comment 15 • 16 years ago

Roc: is there someone else we can offload the backport to, or should we punt on this until a later release?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 16 years ago

I actually think we should not be backporting fixes like this at all.

	Samuel Sidler (old account; do not CC)
	Comment 17 • 16 years ago

Not blocking but wanted.