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

RESOLVED FIXED

Status

()

defect
P2
normal
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: jruderman, Assigned: roc)

Tracking

(Blocks 1 bug, {assertion, fixed1.9.1, testcase})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments)

Reporter

Description

11 years ago
Posted file 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
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.
Group: core-security
Flags: blocking1.9.1?
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?
Whiteboard: [sg:critical?]
Anything with ordinals makes me wonder if it's fixed by bug 431705.
Doesn't seem to be.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
I can reproduce both assertions on a linux debug build; Platform --> All/All
OS: Mac OS X → All
Hardware: x86 → All
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.
Assignee: nobody → roc
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.
Depends on: 433296
Whiteboard: [sg:critical?] → [sg:critical?][depends on 433296]
Posted patch fixSplinter Review
Voila
Attachment #355942 - Flags: superreview?(bzbarsky)
Attachment #355942 - Flags: review?(bzbarsky)
Oh, I really meant to attach this patch to the other bug. But it doesn't matter.
Blocks: 433296
No longer depends on: 433296
Whiteboard: [sg:critical?][depends on 433296] → [sg:critical?][needs review]
Comment on attachment 355942 [details] [diff] [review]
fix

Let's do it.
Attachment #355942 - Flags: superreview?(bzbarsky)
Attachment #355942 - Flags: superreview+
Attachment #355942 - Flags: review?(bzbarsky)
Attachment #355942 - Flags: review+
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.
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?
I'd be ok with that, sure.  But then we have to make sure no one ever adds calls to the reordering stuff elsewhere...
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.
Pushed 1da6da73561c
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs 191 landing]
Patch pushed to 1.9.1 branch on behalf of roc.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/254bb9eafd97
Keywords: fixed1.9.1
Whiteboard: [sg:critical?][needs 191 landing] → [sg:critical?]
Group: core-security
You need to log in before you can comment on or make changes to this bug.