Crash [@ nsCSSFrameConstructor::FindFrameWithContent] with tooltip, mathml:and and moving stuff in it

VERIFIED FIXED

Status

()

defect
--
critical
VERIFIED FIXED
13 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: roc)

Tracking

(Depends on 1 bug, Blocks 1 bug, {crash, testcase})

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
wanted1.8.1.x -
wanted1.8.0.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] deleted frame. post 1.8 branch - fix ready for review, crash signature)

Attachments

(2 attachments)

See upcoming testcase, which crashes Mozilla on load.
It also crashes Mozilla1.7, so no recent regression.

Talkback ID: TB24427770K
nsCSSFrameConstructor::FindFrameWithContent  [mozilla\layout\base\nscssframeconstructor.cpp, line 11060]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11197]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 408]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11167]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 408]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11167]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 408]
nsElementSH::PostCreate  [mozilla\dom\src\base\nsdomclassinfo.cpp, line 7252]
XPCWrappedNative::GetNewOrUsed  [mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 463]
XPCConvert::NativeInterface2JSObject  [mozilla\js\src\xpconnect\src\xpcconvert.cpp, line 1098]
XPCConvert::NativeData2JS  [mozilla\js\src\xpconnect\src\xpcconvert.cpp, line 474]
XPCWrappedNative::CallMethod  [mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2252]

Marking security sensitive, just to be sure.
Posted file testcase
CCing rbs in case this is a MathML bug.
In a debug build aParentFrame appears to be a deleted object (filled with 0xdddddddd).

FF1.5.0.9 and FF2.0.0.2pre do not crash. If you saw it in Mozilla1.7 it went away and came back? Did we fix it for FF1.5 on the 1.8 branch and forget to land the patch on the trunk?
Whiteboard: [sg:critical] deleted frame. Not 1.8 branch
Ah, sorry, I've tested it again, it doesn't crash a 1.7 build, nor a 1.8.1 build.
Critical security bugs must have owners. If you can't work on this bug please help us find another active owner for it.
Assignee: nobody → roc
Martijn,  should we close this based on your comment 4 ?
No, it still crashes trunk.
Basically what this testcase has is

<tooltip>
  <mathml:and/>
</tooltip>

and it inserts a <box> under the <mathml:and>.

That triggers nsCSSFrameConstructor::WipeContainingBlock which tries to find the nearest enclosing frame that's safe to recreate frames for. It ends up choosing the anonymous popupgroup that contains the tooltip popup! This falls down badly because the popups in that popupgroup have been gathered from content outside the popupgroup (namely, our <tooltip>).

The relevant frame hierarchy here is:
  nsPopupSetFrame (anonymous popupgroup)
    nsMenuPopupFrame (<tooltip>)
      nsInlineFrame (<mathml:and>)

It's really the <tooltip> that we want to recreate frames for here. But we don't because IsInlineFrame() returns true for it! And that's because IsInlineFrame specifically returns true for NS_STYLE_DISPLAY_DECK, POPUP and GROUPBOX. Indeed, removing those bogus-looking lines from IsInlineFrame fixes the crash.

So why would they need to be treated as inline? I don't know. They were there right from the beginning when XUL display types were created (bug 70704). But as far as I can tell every user of IsInlineFrame in nsCSSFrameConstructor, at least currently, should not be treating them as inline, or doesn't care:
-- one call from RecreateFramesForContent that's wondering about IB splits
-- one call from WrapFramesInFirstLetterFrame
-- one call from AppendFirstLineFrames
-- one call from AreAllKidsInline (called only by WipeContainingBlock itself); is it trying to avoid wiping the containing block when inserting a deck, groupbox or popup into an inline frame? That seems bogus
-- one call from ProcessInlineChildren which is computing allKidsInline, which is used to decide whether we do an IB split. Surely we should split if a deck, groupbox or popup is inserted?
-- the call from WipeContainingBlock discussed above.

I think we should remove DECK, POPUP and GROUPBOX from IsInlineFrame unless someone can think of a reason not to. I'll post a patch to do just that.
Posted patch fixSplinter Review
As described.

If this goes in I think we should follow up by removing IsInlineFrame and IsInlineFrame2, and also uses of nsStyleDisplay::IsBlockLevel, in favour of nsStyleDisplay::Is(Block/Inline)(Inside/Outside). We'd want to apply this change to IsInlineOutside (which currently has no callers).

How should XUL boxes be treated by those methods? Neither block or inline inside or outside?
Attachment #257790 - Flags: superreview?(dbaron)
Attachment #257790 - Flags: review?(dbaron)
Whiteboard: [sg:critical] deleted frame. Not 1.8 branch → [sg:critical] deleted frame. Not 1.8 branch - fix ready for review
Do you know if this patch changes the behavior of these display types when included within a line?  (I'm worried mainly about deck, and perhaps groupbox, although I don't really even know what that is.)
Yeah, it does change behaviour. Groupbox is the XUL equivalent of fieldset.

Actually with this patch things are a bit broken because nsLineLayout::TreatFrameAsBlock still treats decks as inline, so we can still put decks on their own line when they're direct children of blocks, but if you put a deck inside an inline element, nsCSSFrameConstructor treats it as non-inline so we do an IB split.

If you like I can restrict this patch to just removing POPUP from IsInline.

We can leave decks and groupboxes as "inline outside", but I think we should do what I suggested in comment #9 and also alter TreatFrameAsBlock:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsLineLayout.cpp#2692
Shouldn't it just be
  return !aFrame->GetStyleDisplay()->IsInlineOutside() &&
         aFrame->GetType() != nsGkAtoms::placeholderFrame;
?
Or hmm, is the style system hacking the display type of placeholders to make them inline? Otherwise I don't see how nsCSSFrameConstructor::IsInlineFrame and related callers work properly today.
Placeholder frames have the same type of style context as text frames -- one that has no rules atching it.  That means they're always inline.
Cool, then we could just get rid of TreatFrameAsBlock and call IsInlineOutside instead.

So do you want me to restrict this patch to just remove POPUP from IsInline?
Flags: wanted1.8.1.x-
Flags: wanted1.8.0.x-
Whiteboard: [sg:critical] deleted frame. Not 1.8 branch - fix ready for review → [sg:critical] deleted frame. post 1.8 branch - fix ready for review
Any progress here? Sounds like the patch needs a respin... dbaron, what needs to be changed so we can get a new patch here?
Flags: blocking1.9+
Comment on attachment 257790 [details] [diff] [review]
fix

I really don't remember wanting anything changed here (other than the cleanup to follow), so r+sr=dbaron on this.
Attachment #257790 - Flags: superreview?(dbaron)
Attachment #257790 - Flags: superreview+
Attachment #257790 - Flags: review?(dbaron)
Attachment #257790 - Flags: review+
checked in the patch.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Cleanup followup filed as bug 383551 (with patch!)
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070607 Minefield/3.0a6pre
Status: RESOLVED → VERIFIED
Group: security
Flags: in-testsuite?
crash test landed
http://hg.mozilla.org/mozilla-central/rev/757f02fc2849
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsCSSFrameConstructor::FindFrameWithContent]
You need to log in before you can comment on or make changes to this bug.