Crash [@ nsContentUtils::ContentIsDescendantOf] with counters, MathML

RESOLVED FIXED

Status

()

P4
critical
RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: jruderman, Assigned: roc)

Tracking

(Blocks: 2 bugs, {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:nse] null deref?, crash signature)

Attachments

(3 attachments)

(Reporter)

Description

11 years ago
Created attachment 290075 [details]
testcase (crashes Firefox when loaded)

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.
This should depend on the bug about mathml doing style system stuff at unsafe times.  Don't have the bug# offhand.
Flags: blocking1.9?
Depends on: 355548
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
(Reporter)

Comment 2

11 years ago
Still crashes now even though bug 355548 is fixed.
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.
Assignee: nobody → roc
Created attachment 296889 [details] [diff] [review]
fix MathML constrained-heights

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.
Attachment #296889 - Flags: superreview?(bzbarsky)
Attachment #296889 - Flags: review?(bzbarsky)
Created attachment 296896 [details] [diff] [review]
fix

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.)
Attachment #296896 - Flags: superreview?(bzbarsky)
Attachment #296896 - Flags: review?(bzbarsky)
Whiteboard: [need review]
Whiteboard: [need review] → [needs review]
Comment on attachment 296896 [details] [diff] [review]
fix

Much better!
Attachment #296896 - Flags: superreview?(bzbarsky)
Attachment #296896 - Flags: superreview+
Attachment #296896 - Flags: review?(bzbarsky)
Attachment #296896 - Flags: review+
Attachment #296889 - Flags: superreview?(bzbarsky)
Attachment #296889 - Flags: superreview+
Attachment #296889 - Flags: review?(bzbarsky)
Attachment #296889 - Flags: review+
(Reporter)

Comment 7

11 years ago
Vlad Sukhoy has a different change to the "dangling frame without a content node" assertion condition in bug 400475.
His is better, I'll take mine out before checking in.
checked in with crashtest.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
(Reporter)

Comment 10

11 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.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.14?
Does this patch work for the 1.8 branch?
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Whiteboard: [sg:nse] null deref?
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?
Whiteboard: [sg:nse] null deref? → [sg:nse] null deref? need roc reply to comment 12
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.
It doesn't.

The other patch is riskier and nontrivial to backport to branch.
Roc: is there someone else we can offload the backport to, or should we punt on this until a later release?
Flags: blocking1.8.1.15+ → blocking1.8.1.16+
I actually think we should not be backporting fixes like this at all.
Not blocking but wanted.
Flags: blocking1.8.1.17+
Whiteboard: [sg:nse] null deref? need roc reply to comment 12 → [sg:nse] null deref?
Crash Signature: [@ nsContentUtils::ContentIsDescendantOf]
Group: core-security
You need to log in before you can comment on or make changes to this bug.