Closed
Bug 874418
Opened 12 years ago
Closed 12 years ago
Assert that placeholders are reflowed before their out-of-flows
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bzbarsky, Assigned: dholbert)
References
(Depends on 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 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•12 years ago
|
Assignee: bzbarsky → nobody
Reporter | ||
Updated•12 years ago
|
Status: ASSIGNED → NEW
Assignee | ||
Comment 3•12 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•12 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•12 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•12 years ago
|
||
Here's a patch along the lines of my previous comment.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=393e788f1ad5
Assignee | ||
Comment 7•12 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•12 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•12 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•12 years ago
|
||
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Flags: in-testsuite-
Comment 11•12 years ago
|
||
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•12 years ago
|
||
d'oh. I forgot to include crashtests in the Try run.
Thanks, philor.
Assignee | ||
Updated•12 years ago
|
Flags: needinfo? → needinfo?(dholbert)
Assignee | ||
Comment 14•12 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•12 years ago
|
||
Here's a reduced static testcase that triggers the NS_ERROR in the formerly-landed patch.
Assignee | ||
Comment 16•12 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•12 years ago
|
||
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•12 years ago
|
Attachment #754229 -
Attachment description: add IB siblings to exemption → patch v3: add IB-split siblings to exemption
Assignee | ||
Updated•12 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•12 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
Reporter | ||
Comment 18•12 years ago
|
||
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•12 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•12 years ago
|
||
> // (This could eventually nsLayoutUtils::GetPrevContinuationOrSpecialSibling(),
(er, s/eventually/eventually call/)
Looks like inbound is hosed, so I'll land this tomorrow.
Assignee | ||
Comment 21•12 years ago
|
||
Comment 22•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•