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

RESOLVED WORKSFORME

Status

()

Core
Layout
RESOLVED WORKSFORME
6 years ago
7 months ago

People

(Reporter: Neil Deakin, Unassigned)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

1.38 KB, patch
Details | Diff | Splinter Review
480 bytes, application/xhtml+xml
Details
131 bytes, application/vnd.mozilla.xul+xml
Details
(Reporter)

Description

6 years ago
Created attachment 627289 [details]
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.
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?
> 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?
(Reporter)

Comment 3

6 years ago
Created attachment 630954 [details] [diff] [review]
Patch, destroy popupgroup last
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #630954 - Flags: review?(bzbarsky)
Comment on attachment 630954 [details] [diff] [review]
Patch, destroy popupgroup last

r=me.  Can you add a crashtest?
Attachment #630954 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 5

6 years ago
Created attachment 631906 [details]
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]<>
Attachment #627289 - Attachment is obsolete: true
Hmm.  Can the popupset actually appear anywhere in the frame tree??
(Reporter)

Comment 7

6 years ago
Created attachment 631918 [details]
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.
(Reporter)

Comment 8

6 years ago
The popupgroup shouldn't be placed in an anonymous block. bz, any idea where this is being done?
Boxes wrap all their kids in an anonymous block if they have an inline kid.  Is that what's happening here?
(Reporter)

Comment 10

6 years ago
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.
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...
(Reporter)

Comment 12

6 years ago
This is what I experimented with:

http://pastebin.mozilla.org/1684055

Updated

5 years ago
Duplicate of this bug: 838700
As a note, bz gave me r=him on IRC to bump the assertion count on the crashtest to allow landing bug 653881.
(Reporter)

Updated

3 years ago
Assignee: enndeakin → nobody
Status: ASSIGNED → NEW
This seems to work for me now; I don't get assertions...
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Flags: in-testsuite?
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.