All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: dholbert)

Tracking

(Depends on: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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

Updated

6 years ago
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Reporter)

Comment 1

6 years ago
Created attachment 752232 [details] [diff] [review]
patch v1: Assert that placeholders are reflowed before their out-of-flows.
(Reporter)

Comment 2

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

Updated

6 years ago
Assignee: bzbarsky → nobody
(Reporter)

Updated

6 years ago
Status: ASSIGNED → NEW
(Assignee)

Comment 3

6 years ago
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]
 }
?
(Reporter)

Comment 4

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

Comment 5

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

Comment 6

6 years ago
Created attachment 752571 [details] [diff] [review]
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
(Assignee)

Comment 7

6 years ago
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.
Attachment #752571 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

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

Comment 9

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

r=me
Attachment #752571 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 10

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/affd9b74be00
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite-
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!"
(Assignee)

Comment 12

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

Thanks, philor.
(Assignee)

Comment 13

6 years ago
needinfo=me to investigate
Flags: needinfo?
(Assignee)

Updated

6 years ago
Flags: needinfo? → needinfo?(dholbert)
(Assignee)

Comment 14

6 years ago
(BTW, the two failing crashtests are actually identical (md5sum and all). I just opened bug  876194 to clean that up.)
Flags: needinfo?(dholbert)
(Assignee)

Comment 15

6 years ago
Created attachment 754228 [details]
breaktest 1

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

Comment 16

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

Comment 17

6 years ago
Created attachment 754229 [details] [diff] [review]
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.
Attachment #754229 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Attachment #754229 - Attachment description: add IB siblings to exemption → patch v3: add IB-split siblings to exemption
(Assignee)

Updated

6 years ago
Attachment #752571 - Attachment description: assert or warn, depending on whether we're in a continuation → patch v2: assert or warn, depending on whether we're in a continuation
Attachment #752571 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #752232 - Attachment description: Assert that placeholders are reflowed before their out-of-flows. → patch v1: Assert that placeholders are reflowed before their out-of-flows.
Attachment #752232 - Attachment is obsolete: true
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
Attachment #754229 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

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

Comment 20

6 years ago
>  // (This could eventually nsLayoutUtils::GetPrevContinuationOrSpecialSibling(),

(er, s/eventually/eventually call/)

Looks like inbound is hosed, so I'll land this tomorrow.
https://hg.mozilla.org/mozilla-central/rev/7698a95cd136
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

5 years ago
Depends on: 878538
You need to log in before you can comment on or make changes to this bug.