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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Jesse Ruderman]

CCing rbs in case this is a MathML bug.

Comment 3

[Daniel Veditz [:dveditz]]

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?

Comment 4

[Martijn Wargers (dead)]

Ah, sorry, I've tested it again, it doesn't crash a 1.7 build, nor a 1.8.1 build.

Comment 5

[Daniel Veditz [:dveditz]]

Critical security bugs must have owners. If you can't work on this bug please help us find another active owner for it.

Comment 6

[chris hofmann]

Martijn,  should we close this based on your comment 4 ?

Comment 7

[Martijn Wargers (dead)]

No, it still crashes trunk.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

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?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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;
?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 15

[Samuel Sidler (old account; do not CC)]

Any progress here? Sounds like the patch needs a respin... dbaron, what needs to be changed so we can get a new patch here?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in the patch.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Cleanup followup filed as bug 383551 (with patch!)

Comment 19

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070607 Minefield/3.0a6pre

Comment 20

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/757f02fc2849