Open Bug 1340966 Opened 7 years ago Updated 2 years ago

Add assertion to nsReflowStatus::SetNextInFlowNeedsReflow()

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

People

(Reporter: TYLin, Unassigned)

References

Details

Attachments

(3 files, 2 obsolete files)

Followup of bug 775624 comment 44. We should add assertion to SetNextInFlowNeedsReflow() to ensure the IsIncomplete() is true.
MozReview-Commit-ID: 2yrL79jOV8A
The invariant "SetNextInFlowNeedsReflow() shouldn't be called when IsComplete() is true." is already violated per try results here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=45ce9d66fc603f2b6fde4f12067b55a12573a553

Perhaps we should instead update the documentation.
Attachment #8841529 - Flags: review?(dholbert)
Assignee: nobody → tlin
Comment on attachment 8841529 [details] [diff] [review]
Update document for SetNextInFlowNeedsReflow().

Review of attachment 8841529 [details] [diff] [review]:
-----------------------------------------------------------------

I think we need to understand the failures better, before we make any changes here.  Please either:

(1) investigate so we better understand what changed & why, with respect to this supposed invariant (so we can be sure that it's OK that we're violating it).

...OR, if you don't have time for that at this point:
(2) Add a comment like the following:
 // XXX We might not live up to this expectation anymore; see bug NNN
...where "bug NNN" is a followup that includes your info from comment 1 and 2 here.  (It could just be this bug, maybe, as long as we don't let this ^^ comment tweak cause this bug to be closed.)
Attachment #8841529 - Flags: review?(dholbert) → review-
Priority: -- → P3
Attachment #8841529 - Attachment is obsolete: true
Attachment #8841482 - Attachment is obsolete: true
Comment on attachment 8905021 [details]
Bug 1340966 Part 3 - Add assertion in SetNextInFlowNeedsReflow(), to verify that it's never called on fully-complete frames

https://reviewboard.mozilla.org/r/176338/#review181924

::: commit-message-c9593:3
(Diff revision 1)
> +Bug 1340966 - Asserting SetNextInFlowNeedsReflow() should be fully complete when called.
> +
> +The comment "you wouldn't set mNextInFlowNeedsReflow IsComplete() is true"

This quote isn't quite right -- you're missing the word "when".

The correct quote is:
"you wouldn't set mNextInFlowNeedsReflow when IsComplete() is true"

::: commit-message-c9593:4
(Diff revision 1)
> +was written before OverflowIncomplete was introduced in bug 379349.
> +Nowadays, nsGridContainerFrame sets this bit with SetOverflowIncomplete [1].
> +

I'm not clear why you're bringing up nsGridContainerFrame here -- this patch/bug doesn't involve grid at all.  And grid only one of several callers of this API.  So it doesn't seem particularly relevant.

I think you could probably just drop this "Nowadays" sentence -- the rest of the paragraph makes sense and successfully explains the discrepancy between the old code-comment vs. current reality without that last sentence.

::: commit-message-c9593:7
(Diff revision 1)
> +So I believe we should assert IsFullyComplete() in
> +SetNextInFlowNeedsReflow() rather than assert IsComplete().

Note that we don't assert *anything* right now, so the "rather than ..." in this comment is kind of confusing.

How about:

"rather than assert IsComplete() (as the old comment implies we could)."

::: commit-message-c9593:10
(Diff revision 1)
> +Other modifications are setting (or merging) incomplete status for callers.
> +

These "other modifications" are functional changes and probably deserve their own patch (with a clearer explanation of what is changing & why), rather than being folded into an "add commit message" patch.  Could you spin them out (with the reftest.list tweak as well, I imagine) and explain why you're making those changes?

::: commit-message-c9593:12
(Diff revision 1)
> +472587-1.xhtml will have assertions due to the aState.mReflowStatus was set
> +to Incomplete() in nsBlockFrame::DoReflowInlineFrames(). Some of the
> +crashtests already have this assertion (bug 847368), so it should be fine.

I'm not so sure that this new instance of the assertion failure has the same underlying cause as the old instance. Nor am I sure that "it should be fine" is true.  Just because we can already make an assertion fail via one set of circumstances, that doesn't mean it's necessarily fine to introduce a new way for it to fail via some previously-OK other set of circumstances.  It might mean it's non-scary from a security perspective, but it still suggests that we're breaking something...

(If this were a case where stuff's already broken and we were just papering over the assertion by accident and now we won't be papering over it, then that'd make sense to me. But it's not clear to me that this is that sort of change.)

::: commit-message-c9593:16
(Diff revision 1)
> +[1] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/layout/generic/nsGridContainerFrame.cpp#5549-5550
> +[2] http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/layout/generic/nsBlockFrame.cpp#1693-1694

Probably drop these footnotes, because:
 - Per my note above, I don't think it makes sense to bother referencing the nsGridContainerFrame code ([1]).
 - You don't actually reference the nsBlockFrame footnote anywhere ([2]) -- this footnote is orphaned.

Also: if you *do* keep these links (or different ones) in some form, I'd strongly prefer dxr.mozilla.org links (and note that the hashes in the URL will be different, because DXR uses hg hashes, whereas searchfox uses git hashes, I think).  I'm uneasy about encoding searchfox.org links into our version control system -- searchfox.org *is* awesome, but it's also just a side project basically run by one guy, so I don't have strong confidence that these links will still work in, say, 2 years. And if does disappear, it'd be hard for a human to reconstruct what this URL means (with its hash) via some other service, in part because it's using a git hash, whereas all our other web hosted services use mercurial hashes.  If a future VCS archeologist could just manually swap in s/searchfox.org/dxr.mozilla.org/ and get a working URL, then I'd be less concerned -- but it doesn't work that easily.

(I have stronger confidence that dxr.mozilla.org will still be around in some form, in 2+ years -- or at least that its links will still work & redirect nicely -- because we'll surely require Mozilla IT to keep it working, for use cases just like this one.)

::: layout/generic/crashtests/crashtests.list:334
(Diff revision 1)
>  load 467914-1.html
>  load 468207-1.html
>  load 468771-1.xhtml
>  load 468771-2.xhtml
>  load 469859-1.xhtml # bug 1323665
> -load 472587-1.xhtml
> +asserts(0-1) load 472587-1.xhtml # bug 847368

You should probably file a new bug for this assertion-failure with this testcase.  (And that bug should probably be marked as a regression from this bug.)

The bug you're currently using in the annotation (bug 847368) is about a different specific testcase, and has this in the title:

"... with :first-letter float and page-break-inside:avoid"

But this testcase you're annotating here (472587-1.xhtml) has none of those three features (:first-letter, float, page-break-inside).  So it's possible & perhaps likely that the underlying problem is different (though it leads to failing the same assertion.  Hence, worth using a new bug for the new failure.

(We can always dupe after the fact, if needed.)
Attachment #8905021 - Flags: review?(dholbert) → review-
Comment on attachment 8905021 [details]
Bug 1340966 Part 3 - Add assertion in SetNextInFlowNeedsReflow(), to verify that it's never called on fully-complete frames

https://reviewboard.mozilla.org/r/176338/#review181946

::: commit-message-c9593:10
(Diff revision 1)
> +Other modifications are setting (or merging) incomplete status for callers.
> +

> rather than being folded into an "add commit message" patch.

Sorry -- I meant |"add an assertion" patch| there.

i.e. We shouldn't lump functional changes into a patch whose commit message claims to just be about adding an assertion.
Comment on attachment 8905021 [details]
Bug 1340966 Part 3 - Add assertion in SetNextInFlowNeedsReflow(), to verify that it's never called on fully-complete frames

https://reviewboard.mozilla.org/r/176338/#review181924

> I'm not clear why you're bringing up nsGridContainerFrame here -- this patch/bug doesn't involve grid at all.  And grid only one of several callers of this API.  So it doesn't seem particularly relevant.
> 
> I think you could probably just drop this "Nowadays" sentence -- the rest of the paragraph makes sense and successfully explains the discrepancy between the old code-comment vs. current reality without that last sentence.

Right. Grid is just one of the callers. I'll drop the "Nowadays ..."

> Note that we don't assert *anything* right now, so the "rather than ..." in this comment is kind of confusing.
> 
> How about:
> 
> "rather than assert IsComplete() (as the old comment implies we could)."

This is better!

> These "other modifications" are functional changes and probably deserve their own patch (with a clearer explanation of what is changing & why), rather than being folded into an "add commit message" patch.  Could you spin them out (with the reftest.list tweak as well, I imagine) and explain why you're making those changes?

Yes. I can spilt functional changes to another patch.

> Probably drop these footnotes, because:
>  - Per my note above, I don't think it makes sense to bother referencing the nsGridContainerFrame code ([1]).
>  - You don't actually reference the nsBlockFrame footnote anywhere ([2]) -- this footnote is orphaned.
> 
> Also: if you *do* keep these links (or different ones) in some form, I'd strongly prefer dxr.mozilla.org links (and note that the hashes in the URL will be different, because DXR uses hg hashes, whereas searchfox uses git hashes, I think).  I'm uneasy about encoding searchfox.org links into our version control system -- searchfox.org *is* awesome, but it's also just a side project basically run by one guy, so I don't have strong confidence that these links will still work in, say, 2 years. And if does disappear, it'd be hard for a human to reconstruct what this URL means (with its hash) via some other service, in part because it's using a git hash, whereas all our other web hosted services use mercurial hashes.  If a future VCS archeologist could just manually swap in s/searchfox.org/dxr.mozilla.org/ and get a working URL, then I'd be less concerned -- but it doesn't work that easily.
> 
> (I have stronger confidence that dxr.mozilla.org will still be around in some form, in 2+ years -- or at least that its links will still work & redirect nicely -- because we'll surely require Mozilla IT to keep it working, for use cases just like this one.)

OK. I'll use dxr.mozilla.org if needed. Thanks for explaining the difference between searchfox.org and dxr to me.
Comment on attachment 8905021 [details]
Bug 1340966 Part 3 - Add assertion in SetNextInFlowNeedsReflow(), to verify that it's never called on fully-complete frames

https://reviewboard.mozilla.org/r/176338/#review182710

::: layout/generic/nsBlockFrame.cpp:4172
(Diff revision 1)
> +        aState.mReflowStatus.SetIncomplete();
>          aState.mReflowStatus.SetNextInFlowNeedsReflow();

Instead of adding `SetIncomplete()` that leads to a regression, I'd like to remove this `SetNextInFlowNeedsReflow()`. Because these lines were added in bug 523468, and the tests added are no longer assert locally. Let's see whether try agrees with this.
Comment on attachment 8905021 [details]
Bug 1340966 Part 3 - Add assertion in SetNextInFlowNeedsReflow(), to verify that it's never called on fully-complete frames

https://reviewboard.mozilla.org/r/176338/#review182948

This patch should be ordered last, rather than first, so that we aren't creating an intermediate (slightly) broken state.

(The brokenness isn't huge here, but it's trivial to avoid just by ordering things nicer, so we might as well.)

::: commit-message-c9593:1
(Diff revision 2)
> +Bug 1340966 Part 1 - Asserting SetNextInFlowNeedsReflow() should be fully complete when called.

This needs rewording for several reasons.

Firstly, it sounds like it's saying "SetNextInFlowNeedsReflow()" *is the thing* that should be complete (whatever that would mean).
Secondly, the logic here is backwards -- it's missing a "not".  (Really, the patch is asserting that something is NOT fully complete when this function is called.)

Perhaps replace the commit message with the following:
"Add assertion in SetNextInFlowNeedsReflow(), to verify that it's never called on fully-complete frames"

::: commit-message-c9593:3
(Diff revision 2)
> +The comment "you wouldn't set mNextInFlowNeedsReflow when IsComplete() is
> +true" was written before OverflowIncomplete was introduced in bug 379349.

This isn't quite true. Please adjust like so:
s/The comment/The original version of the comment/

(The exact phrase that you're quoting here is actually fairly recent, from your bug 775624. :) But I assume you're referring to its original form, about NS_FRAME_COMPLETE and NS_FRAME_REFLOW_NEXTINFLOW, and that version does indeed date to 1999 it seems.  So, let's make it clearer that you're talking about the *original* version of this comment.)

::: commit-message-c9593:6
(Diff revision 2)
> +So I believe we should assert IsFullyComplete() in
> +SetNextInFlowNeedsReflow() rather than assert IsComplete() (as the old
> +comment implies we could).

I just realized, this is missing some "!" negation operators here.

It should say "!IsFullyComplete()... !IsComplete()..."
Attachment #8905021 - Flags: review?(dholbert) → review-
Comment on attachment 8905021 [details]
Bug 1340966 Part 3 - Add assertion in SetNextInFlowNeedsReflow(), to verify that it's never called on fully-complete frames

https://reviewboard.mozilla.org/r/176338/#review182948

I've updated my patches to address comment 14 :)
Comment on attachment 8905838 [details]
Bug 1340966 Part 1 - Fix assertion in nsBlockFrame::ReflowFloat().

https://reviewboard.mozilla.org/r/177642/#review185562

::: commit-message-38e92:6
(Diff revision 2)
> +Bug 1340966 Part 1 - Fix assertion in nsBlockFrame::ReflowFloat().
> +
> +After adding Part 3, aState.mReflowStatus.SetNextInFlowNeedsReflow() will
> +assert, so fix it beforehand.
> +
> +This is fixed by using MergeCompletionStatusFrom() to properly merging the

s/merging/merge/

::: commit-message-38e92:7
(Diff revision 2)
> +mCompletion status from aReflowStatus to aState.mReflowStatus. Because
> +aReflowStatus is either OverflowIncomplete or Incomplete,
> +aState.mReflowStatus won't violate the assertion that will be added in
> +nsReflowStatus::SetNextInFlowNeedsReflow().

This commit message successfully explains why this change will save us from violating the assertion that you'll be adding in part 3.

But it doesn't explain why the change is correct/valid.  If we're taking this change, I want to better understand why it's correct, layout-wise (and won't break implicit assumptions in the surrounding code, and ideally won't change behavior except for the better, etc.)

::: layout/generic/nsBlockFrame.cpp:6407
(Diff revision 2)
>    if (aReflowStatus.NextInFlowNeedsReflow()) {
> -    aState.mReflowStatus.SetNextInFlowNeedsReflow();
> +    aState.mReflowStatus.MergeCompletionStatusFrom(aReflowStatus);
>    }
>  
>    if (aFloat->IsLetterFrame()) {
>      // We never split floating first letters; an incomplete state for
>      // such frames simply means that there is more content to be
>      // reflowed on the line.
>      if (aReflowStatus.IsIncomplete())
>        aReflowStatus.Reset();

As noted in the commit message, it's not clear to me whether this "MergeCompletionStatusFrom() upgrade" is a behavior-change or not.  (Is this changing the value that ends up in aState.mReflowStatus in a way that impacts what we do / whether we try to split / etc?)


For example / in particular: I'm slightly worried about the IsLetterFrame special case right after this change (which I'm including in the MozReview code-highlight here).  In current m-c, that special case makes us throw away any incompleteness, to avoid splitting first letter frames -- but now with your change, we'll be *capturing* that incompleteness in aState.mReflowStatus, which seems like it might make us try to split (when we wouldn't have split before), depending on how aState.mReflowStatus is used...
Attachment #8905838 - Flags: review?(dholbert) → review-
Comment on attachment 8905839 [details]
Bug 1340966 Part 2 - Fix assertion in nsBlockFrame::DoReflowInlineFrames().

https://reviewboard.mozilla.org/r/177644/#review185564

::: commit-message-5cb3d:2
(Diff revision 2)
> +
> +SetNextInFlowNeedsReflow() will also assert on this call site after adding
> +Part 3. These lines removed here were added in bug 523468, but the test
> +case (523468-1.html) added in bug 523468 is no longer assert after applying
> +this patch, so these lines do not seem relevant anymore.

s/is no longer assert/will no longer assert/

Though: as with part 1, this commit message doesn't give me a lot of confidence about whether this change is good/correct.  Not violating assertions is good and all, but it doesn't mean it's correct. :)


Specifically: the change here seems to be about a case where we're pushing some lines to a container that is not ourselves (with that container presumably being a next-in-flow).  In that scenario, SetNextInFlowNeedsReflow() seems like an entirely appropriate call for us to make.  Perhaps we really should be setting ourselves as incomplete here? [and maybe we even *already* do that eventually, but just not until later...?
I wonder if the places where we violate this assertion (the code that you're touching in part 1 and 2) are simply cases where we *eventually* satisfy the invariant, but we just don't satisfy it yet when we call SetNextInFlowNeedsReflow...? 

i.e. maybe the original documentation about "you wouldn't set both NS_FRAME_COMPLETE and NS_FRAME_REFLOW_NEXTINFLOW" was intended to describe the end state, rather than mandating that NS_FRAME_REFLOW_NEXTINFLOW can only be added after we've already set some incomplete status...?

If that was the intention, then maybe this assertion doesn't make sense after all -- or maybe it should go in some later piece of code (like ~nsReflowStatus and nsReflowStatus::Reset, for example).

For now, I'm uneasy with the logic tweaks in Part 1 and Part 2, unless we've got enough analysis to convincingly explain why the changes are sensible...
The two violations SetNextInFlowNeedsReflow() that I am tweaking (in Part 1 and Part 2) are both related to BlockReflowInput::mReflowStatus. Interestingly, none of the direct caller are calling mReflowStatus.NextInFlowNeedsReflow() by searching all the reference [1]. The bit could be transfer to other reflow statuses via MergeCompletionStatusFrom or via copy assignment.

Since BlockReflowInput is live in nsBlockFrame::Reflow(), BlockReflowInput::mReflowStatus::mNextInFlowNeedsReflow bit should be set somewhere during reflowing that block frame's children, and be propagated to the parent of the block frame. I wonder if this makes sense. I guess I'll have to find a test case that really calls NextInFlowNeedsReflow() call after SetNextInFlowNeedsReflow() is called on BlockReflowInput::mReflowStatus.

Also, BlockReflowInput::mReflowStatus has a interesting comment "// XXX get rid of this." Perhaps we should do investigate whether this comment is still valid.

[1] http://searchfox.org/mozilla-central/search?q=symbol:F_%3CT_mozilla%3A%3ABlockReflowInput%3E_5&redirect=false
Comment on attachment 8905839 [details]
Bug 1340966 Part 2 - Fix assertion in nsBlockFrame::DoReflowInlineFrames().

https://reviewboard.mozilla.org/r/177644/#review187694

(Setting review to r- for now, since it's not clear yet that these changes are correct)
Attachment #8905839 - Flags: review?(dholbert) → review-
Comment on attachment 8905021 [details]
Bug 1340966 Part 3 - Add assertion in SetNextInFlowNeedsReflow(), to verify that it's never called on fully-complete frames

https://reviewboard.mozilla.org/r/176338/#review187696
Attachment #8905021 - Flags: review?(dholbert) → review-
Assignee: aethanyc → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: