758695 - placeholder assertion when removing whitespace from 382745-1.html crashtest

Status: REOPENED

(Core :: Layout, defect)

Description

[Neil Deakin]

Attachment: testcase

Steps:

The testcase is a modified version of layout/generic/crashtests/382745-1.xhtml with the binding inline and the whitespace within the tree removed.

The following assertion occurs:

###!!! ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.: '!placeholder || nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, placeholder)', file /builds/moz2/focus/layout/generic/nsFrame.cpp, line 605

Some stacks and frame tree dumps from a build are at http://pastebin.mozilla.org/1612548 (generated with the patch in bug 653881 applied)

The assertion occurs while the menupopup inside the tree's treecolpicker is being destroyed. I'm guessing that the popupset frame is being destroyed before the fixed items list, so the real menupopup frame is being destroyed before the placeholder which is a descendant within the fixed items list.

Removing the whitespace from the test is important as, since its an xhtml file where whitespace is significant, some xbl issue occurs that causes the <treecols> not to generate its anonymous content, so the menupopup never even gets generated. With the patch in bug 653881, this xbl issue is gone, so this assertion occurs.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  I can't seem to reproduce with that testcase: I get the various text, but no asserts.....

Is that because I'm on Mac, or something else?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I'm guessing that the popupset frame is being destroyed before the fixed items list, so
> the real menupopup frame is being destroyed before the placeholder which is a descendant
> within the fixed items list.

Hmm.  Yeah, that seems pretty likely.

So this is a weird case where something looks "in-flow" (the popupset and its kids) while it's actually out-of-flow content.

Could we have destruction of the DocElementBox make sure to destroy the PopupSet, if any, last?  That would certainly fix the issue...  Maybe in DocElementBox::DestroyFrom, remove the popupgroup, if any, from the child list, then destroy the other stuff, then destroy the popupgroup?  Or just move the popupgroup to the end of the child list before calling into the superclass?

Comment 3

[Neil Deakin]

Attachment: Patch, destroy popupgroup last

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 630954 [details] [diff] [review]
Patch, destroy popupgroup last

r=me.  Can you add a crashtest?

Comment 5

[Neil Deakin]

Attachment: right testcase

This is the right testcase.

The patch doesn't work on this testcase. This is because there is a block frame in between the nsDocElementBoxFrame and the popupset:

Viewport(-1)@0xeffe58 [view=0x236804b0] {0,0,59280,53400} [state=0000062000043020] [content=0x0] [sc=0xe4f810] pst=:-moz-viewport<
  RootBox(window)(-1)@0xf002a0 {0,0,59280,53400} [state=0000060081c41000] [content=0x236ac9f0] [sc=0xefffd8] pst=:-moz-canvas<
    DocElementBox(window)(-1)@0xf00408 {0,0,59280,53400} [state=00001600a1a41000] [content=0x236ac9f0] [sc=0xf00330]<
      Block(window)(-1)@0xf37aa0 [content=0x236ac9f0] {0,0,59280,11052} [state=0000000000141020] [vis-overflow=-60,-240,59340,11532] [scr-overflow=0,0,59280,11052] sc=0xf74740(i=1,b=0) pst=:-moz-xul-anonymous-block<
        line 0xf37b00: count=6 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x141] {0,0,19562,11052} vis-overflow={-60,-240,19622,11532} scr-overflow={0,0,19562,11052} <
          PopupSet(popupgroup)(-1)@0xf00b30 next=0xf00cd8 {0,876,0,0} [state=0000160080c40000] [content=0x24b6d070] [sc=0xf00da8]<
            PopupList for PopupSet(popupgroup)(-1)@0xf00b30 <
              MenuPopup(menupopup)(-1)@0xfcc448 [view=0x24b832c0] next=0xf00ba8 {0,0,0,0} [state=0000060080802502] [content=0x236b44c0] [sc=0xfcc108]<>

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  Can the popupset actually appear anywhere in the frame tree??

Comment 7

[Neil Deakin]

Attachment: popupgroup with block testcase

It shouldn't do. The popupgroup should always be right in the document element.

The attached testcase is a minimal testcase that shows this problem. The element in another namespace causes the anonymous block to be added.

Comment 8

[Neil Deakin]

The popupgroup shouldn't be placed in an anonymous block. bz, any idea where this is being done?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Boxes wrap all their kids in an anonymous block if they have an inline kid.  Is that what's happening here?

Comment 10

[Neil Deakin]

In the testcase, <element> is an inline element.

Viewport(-1)@0x70c51f8 [view=0x6c59f00] {0,0,5520,3600} [state=0000066400042028] [content=0x0] [sc=0x70c4ff0] pst=:-moz-viewport<
  RootBox(window)(-1)@0x70c5690 {0,0,5520,3600} [state=0000064081c40000] [content=0x6c851a0] [sc=0x70c53c8] pst=:-moz-canvas<
    DocElementBox(window)(-1)@0x70c57f8 {0,0,5520,3600} [state=00001640a1a40010] [content=0x6c851a0] [sc=0x70c5720]<
      Block(window)(-1)@0x710c068 [content=0x6c851a0] {0,0,5520,1680} [state=0000000000140020] [vis-overflow=0,0,5520,1740] [scr-overflow=0,0,5520,1680] sc=0x710be70(i=1,b=0) pst=:-moz-xul-anonymous-block<
        line 0x710c0c8: count=4 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x140] {0,0,5460,1680} vis-overflow={0,0,5460,1740} scr-overflow={0,0,5460,1680} <
          PopupSet(popupgroup)(-1)@0x70c6338 next=0x70c6488 {0,1080,0,0} [state=0000160080c40000] [content=0x6cc2c10] [sc=0x70c58e8]<
            PopupList for PopupSet(popupgroup)(-1)@0x70c6338 <
              MenuPopup(tooltip)(-1)@0x70c63b0 [view=0x6cdb960] {0,0,0,0} [state=0000060080802502] [content=0x6cc2ec0] [sc=0x70c5c90]<>
            >
          >
          Placeholder(tooltip)(-1)@0x70c6488 {0,1080,0,0} [state=0000000000800000] [content=0x6cc2ec0] [sc=0x70c5f70] outOfFlowFrame=MenuPopup(tooltip)(-1)@0x70c63b0
          Inline(element)(0)@0x70c64c0 next=0x70c6508 {0,420,0,840} [state=0000100000000000] [content=0x6c855b0] [sc=0x70c5ad0]<>
          ButtonBoxFrame(button)(1)@0x70c6508 {360,300,4740,1200} [state=2000164080c00010] [content=0x6c85650] [vis-overflow=-240,-240,5220,1680] [scr-overflow=0,0,4740,1200] [sc=0x70c6120]<
            Box(hbox)(-1)@0x70c65f8 {420,60,3900,960} [state=0000160080400000] [content=0x6cd1a20] [sc=0x70c65a0]<
              ImageBox(image)(-1)@0x710bd20 {1359,480,0,0} [state=0000060000000000] [content=0x6cd8830] [sc=0x70c6700]
              TextBox(label)(-1)[value=OK]@0x710bda0 {1539,60,942,840} [state=0000064000000010] [content=0x6cd88a0] [vis-overflow=0,0,1002,840] [scr-overflow=0,0,942,840] [sc=0x70c6898]
            >
          >
        >
      >
    >
  >
>

I tried simply removing the popupset from the frame list that gets reparented at the end of nsCSSFrameConstructor::ProcessChildren and adding it instead to the docframe, but that causes frame list assertions, and puts the placeholder for the tree element inside the popupset instead. I'm not really sure what to change here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

What assertions did you hit?  What did your code look like?

Alternately, we could change the pulling-out code in DocElementBox to look inside its one anonymous block if that's what its child is...

Comment 12

[Neil Deakin]

This is what I experimented with:

http://pastebin.mozilla.org/1684055

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

As a note, bz gave me r=him on IRC to bump the assertion count on the crashtest to allow landing bug 653881.

Comment 15

[Boris Zbarsky [:bzbarsky]]

This seems to work for me now; I don't get assertions...

Comment 16

[Brian Grinstead [:bgrins]]

I'm seeing this in Bug 1487568, where I'm ignoring whitespace text nodes for the XBL unmatched children compatibility hack which drops content. More info at https://bugzilla.mozilla.org/show_bug.cgi?id=1487568#c16:

> So, what's happens here is that we have a frame tree that looks as follows:
> 
> Viewport
>  - fixed block for the <tree style="position: fixed">
>  - nsRootBoxFrame
>    - nsDocElementBoxFrame
>      - nsPopupSetFrame for the menupopup inside of the treecolpicker.
>        - All the stuff in the popup.
>      - nsPlaceholderFrame for the <tree>
> 
> And we're reframing the root, so we start destroying at nsRootBoxFrame.
> 
> Then the nsPopupSetFrame contains the <menupopup> itself, but its
> placeholder is inside the fixed block under the viewport. Note, crucially,
> how the nsPopupSetFrame has been inserted _before_ the placeholder for the
> <tree>, so when we destroy the <menupopup> under it, we still haven't
> destroyed the fixed block for it.
> 
> The good thing is that we will destroy it once we get around to destroy the
> placeholder, but this all sounds very fishy... I'm not sure how all this
> popupset stuff is supposed to be sound, but given this is behavior that has
> been here since forever, and it's not a correctness issue per se, since
> we'll actually get to destroy the stuff, it may be worth filing a bug and
> move on marking the test-case as asserts-if.