467481 - "ASSERTION: What's going on?" with xul:listbox, iframe, ordinal

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: What's going on?: 'mInnerView', file /Users/jruderman/central/layout/generic/nsFrameFrame.cpp, line 918

###!!! ASSERTION: Shouldn't happen: 'aPresContext->GetPresShell()->GetPrimaryFrameFor(mContent) == this', file /Users/jruderman/central/layout/generic/nsFrameFrame.cpp, line 533

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is XUL bug.  We're constructing two nsGridRowLeaf frames for the same listitem and for all its kids, which gives us two subdocument frames for the same content node.

Generally, that last situation is exploitable.

Comment 2

[Boris Zbarsky [:bzbarsky]]

That extra frame is created by nsListBoxBodyFrame::CreateRows

This is sorta like bug 433296 but not quite, since all the grid row leaves are inside the listbox body in this case in the frame tree.  So I have no idea why CreateRows is trying to create stuff.

Perhaps we should disallow subdocument frames inside listboxes, until listboxes get unhorked?

Comment 3

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

Anything with ordinals makes me wonder if it's fixed by bug 431705.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Doesn't seem to be.

Comment 5

[Daniel Holbert [:dholbert]]

I can reproduce both assertions on a linux debug build; Platform --> All/All

Comment 6

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

I think creating two frame subtrees for the same element is guaranteed to be scary even if we disallow subdocument frames in there. So I think we should try to tackle the root cause.

Comment 7

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

I think we should just disable ordinals in listboxes, reordering of rows with ordinals isn't really compatible with the dynamic creation and destruction of row frames.

Comment 8

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

Attachment: fix

Voila

Comment 9

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

Oh, I really meant to attach this patch to the other bug. But it doesn't matter.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Let's do it.

Comment 11

[[e-mail]]

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

> NS_IMETHODIMP
> nsBoxFrame::RelayoutChildAtOrdinal(nsBoxLayoutState& aState, nsIBox* aChild)
> {
>+  if (!SupportsOrdinalsInChildren())
>+    return NS_OK;
As this is already virtual, it would be minusculely neater to override this in nsListBoxBodyFrame instead.

Comment 12

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

I considered that, but CheckBoxOrder still needs to be skipped, and it seemed better to me to skip both of them (and future/other code) with a single overridden function in subclasses.

But I suppose I could override SetInitialChildList *and* RelayoutChildAtOrdinal in nsListBoxBodyFrame. Boris, what do you think?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'd be ok with that, sure.  But then we have to make sure no one ever adds calls to the reordering stuff elsewhere...

Comment 14

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

The way this is currently written also makes it very easy for us to disable ordinal-reordering for children of other kinds of XUL frames. So I think overall I'll just leave it as-is.

Comment 15

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

Pushed 1da6da73561c

Comment 17

[Daniel Holbert [:dholbert]]

Patch pushed to 1.9.1 branch on behalf of roc.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/254bb9eafd97