874418 - Assert that placeholders are reflowed before their out-of-flows

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This came up in bug 851514.  We should assert when a placeholder gets its first reflow after the out-of-flow's first reflow.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: patch v1: Assert that placeholders are reflowed before their out-of-flows.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So with that patch, I hit asserts at least on the following (I say at least, because the fatal assert kills the test run):

Mochitest:

 layout/generic/test/test_bug394239.html

Reftest:

 layout/reftests/abs-pos/continuation-positioned-inline-1.html 

Crashtest:

 layout/base/crashtests/310638-2.html

All seem to involve rel-pos inlines with continuations and the like with abs-pos kids.  Which is not surprising: the placeholder might be in a later continuation while the abs-pos kid is parented to the first continuation and reflows when that reflows.

In other words, things like bug 489100, bug 489207, bug 490216, bug 507307, bug 629059, bug 667079 ...

Comment 3

[Daniel Holbert [:dholbert]]

Hmm, OK.  So in cases where the placeholder is in a continuation, then this is something that's allowed to happen.

Would it make sense to stick this in #ifdef DEBUG and do something like
 if (none of the frames in my ancestor chain have previous continuations) {
   [assertion goes here]
 }
?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well, it's allowed to happen now.  It's buggy if it happens....

We could do something like that; a bit worried about the overhead.

Comment 5

[Daniel Holbert [:dholbert]]

Yeah, the overhead would be debug-only but still a little sucky.

This would avoid the overhead, except in the cases where we're buggy):
#ifdef DEBUG
  if (placeholder is being reflowed after it's OOF) {
    // Buggy!
    if (any of my ancestors is a continuation) {
      // Just a warning because we have tests that trigger this, unfortunately:
      NS_WARNING("placeholder getting its first reflow after its OOF frame");
    } else {
      NS_ERROR("placeholder getting its first reflow after its OOF frame");
    }
  }
#endif

I'll spin up a patch to do that.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: patch v2: assert or warn, depending on whether we're in a continuation

Here's a patch along the lines of my previous comment.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=393e788f1ad5

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 752571 [details] [diff] [review]
patch v2: assert or warn, depending on whether we're in a continuation

Yay, Try likes this.  Requesting review.

Comment 8

[Daniel Holbert [:dholbert]]

I verified that this asserts on bug 851514's testcase if I back out its patch, too. (as we'd hope)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 752571 [details] [diff] [review]
patch v2: assert or warn, depending on whether we're in a continuation

r=me

Comment 10

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/affd9b74be00

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c234d32193 because layout/generic/crashtests/656130-2.html and layout/generic/crashtests/660451-1.html both say "dude, that's totally normal to us, we reflow our out-of-flows before our placeholders twice every single time we're run!"

Comment 12

[Daniel Holbert [:dholbert]]

d'oh. I forgot to include crashtests in the Try run.

Thanks, philor.

Comment 13

[Daniel Holbert [:dholbert]]

needinfo=me to investigate

Comment 14

[Daniel Holbert [:dholbert]]

(BTW, the two failing crashtests are actually identical (md5sum and all). I just opened bug  876194 to clean that up.)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: breaktest 1

Here's a reduced static testcase that triggers the NS_ERROR in the formerly-landed patch.

Comment 16

[Daniel Holbert [:dholbert]]

So we're failing the assertion in cases where there's an IB split, and the abspos frame falls in an IB sibling before its placeholder.

I think this is a situation like comment 2 (but with IB siblings instead of continuations).  We could probably just extend the patch's existing exemption for continuations to cover IB siblings, too.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: patch v3: add IB-split siblings to exemption

This changes the existing "isInContinuation" exemption to "isInContinuationOrIBSplit".

The point is to check for cases where the placeholder is in a later continuation or a later IB-split sibling than its out-of-flow, and only warn in those cases.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 754229 [details] [diff] [review]
patch v3: add IB-split siblings to exemption

Might be worth it to put a GetPrevContinuationOrSpecialSibling on nsLayoutUtils (similar to the GetNext... method already there) and use it here.  If we do that, we should check for the NS_FRAME_IS_SPECIAL flag.

r=me either way

Comment 19

[Daniel Holbert [:dholbert]]

I'll keep it simple & leave it as-is for now, especially since this check is just there as a hackaround for another bug.  But I'll add a comment saying something like: 
 // (This could eventually nsLayoutUtils::GetPrevContinuationOrSpecialSibling(),
 // if we ever add a function like that.)
so that if someone adds that function for other reasons, they might find & simplify this code.

Comment 20

[Daniel Holbert [:dholbert]]

>  // (This could eventually nsLayoutUtils::GetPrevContinuationOrSpecialSibling(),

(er, s/eventually/eventually call/)

Looks like inbound is hosed, so I'll land this tomorrow.

Comment 21

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7698a95cd136

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7698a95cd136