560441 - "ASSERTION: Ancestors of nodes with frames to be constructed lazily should have frames and not have NEEDS_FRAME bit set" with XBL, frameset

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Ancestors of nodes with frames to be constructed lazily should have frames and not have NEEDS_FRAME bit set: 'content->GetPrimaryFrame() && !content->HasFlag(NODE_NEEDS_FRAME)', file /Users/jruderman/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 6198

This assertion was added in rev f80de1a7c604:
user:        Timothy Nikkel
date:        Mon Jan 18 03:26:40 2010 -0600
summary:     Bug 502937. Part 3. Implement lazy frame construction. r=bzbarsky sr=roc

Comment 1

[Timothy Nikkel (:tnikkel)]

So we have

<frameset style="-moz-binding: url(#foo)">
  <frame id="y"></frame>
</frameset>

The foo binding has

    <content>
      <frame xmlns="http://www.w3.org/1999/xhtml">
        <children xmlns="http://www.mozilla.org/xbl"/>
      </frame>
    </content>

We end up with a flattened tree that looks like

<frameset style="-moz-binding: url(#foo)">
  <frame>
    <frame id="y"></frame>
  </frame>
</frameset>

but the frameset frame processes it's own children when constructing frames and it just looks at its principal child list (no ChildIterator), so it ignores the anonymous <frame> element. Then we insert a span as a child of the <frame id="y"> and we walk up through the anonymous <frame> in nsCSSFrameConstructor::MaybeConstructLazily, and it does not have a frame, so we assert.

We could add some conditions to the assert about the parent of the current content being a leaf frame with the reasoning that leaf frames construct their own children and may do something unusual. Or we could change nsFramesetFrame::Init to use ChildIterator. Or maybe there is a better solution. What do you think Boris?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Using ChildIterator in the frameset code seems like the good thing to do long-term in terms of general sanity, as long as there are no assumptions about parent chains made in frameset frames...

But changing the assert seems like the safe thing to do if we're worried about the ChildIterator approach.  How worried are we?

I'm not sure there's a better solution here, fwiw.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I think I'm just going to go for the quick, safe fix here and modify the assert.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 441136 [details] [diff] [review]
patch

r=bzbarsky

Comment 5

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/46d0398b5a61