1229437 - Assertion failure: "float should be in this block unless it was marked as pushed float" with ruby

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: aFloat->GetParent() == mBlock || (aFloat->GetStateBits() & NS_FRAME_IS_PUSHED_FLOAT) (float should be in this block unless it was marked as pushed float), at layout/generic/nsBlockReflowState.cpp:584

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1229437 part 1 - Add a helper function to get the float containing block of a given frame. r=dbaron

Review commit: https://reviewboard.mozilla.org/r/28715/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28715/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1229437 part 2 - Reparent floats inside pulled ruby segment. r=dbaron

Review commit: https://reviewboard.mozilla.org/r/28717/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28717/

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1229437 part 3 - Support iterating frames of RubyColumn. r=dholbert

Review commit: https://reviewboard.mozilla.org/r/28719/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28719/

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1229437 part 4 - Reparent floats inside pulled ruby column.

Review commit: https://reviewboard.mozilla.org/r/28721/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28721/

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1229437 part 5 - Add crashtests for this bug. r=dbaron

Review commit: https://reviewboard.mozilla.org/r/28723/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28723/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

FWIW the first crashtest is effectively equivalent to the testcase Jesse uploaded here. Part 2 fixes the first crashtest, and part 4 fixes the other one.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

All versions support ruby are affected.

Comment 9

[Daniel Holbert [:dholbert]]

I should probably punt this review to someone with a more recent understanding of our float-handling code.  dbaron would probably be ideal, since he's reviewed ruby-related stuff and also did some css-float refactoring recently.

xidorn, mind tagging dbaron to review? (and/or: dbaron, mind stealing review?) (Not sure how complex it is to change the reviewer in mozreview; hopefully not too bad.)

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700452 [details]
MozReview Request: Bug 1229437 part 1 - Add a helper function to get the float containing block of a given frame. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28715/diff/1-2/

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700453 [details]
MozReview Request: Bug 1229437 part 2 - Reparent floats inside pulled ruby segment. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28717/diff/1-2/

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700455 [details]
MozReview Request: Bug 1229437 part 4 - Reparent floats inside pulled ruby column.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28721/diff/1-2/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700456 [details]
MozReview Request: Bug 1229437 part 5 - Add crashtests for this bug. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28723/diff/1-2/

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8700454 [details]
MozReview Request: Bug 1229437 part 3 - Support iterating frames of RubyColumn. r=dholbert

https://reviewboard.mozilla.org/r/28719/#review25653

Looks good! Nits below.

::: layout/generic/RubyUtils.h:129
(Diff revision 1)
> +  class MOZ_STACK_CLASS Iterator

Please add a brief comment to document the purpose of this class (& what it iterates over). E.g.:
  // Helper class to support iteration across the frames within
  // a single RubyColumn (the column's ruby base & its annotations).

This is particularly worth documenting since "RubyColumnEnumerator" lives right below us, and it's easy to mix up "RubyColumnEnumerator" and "RubyColumn::Iterator".  Nice to have clear documentation saying what each one does.

::: layout/generic/RubyUtils.h:132
(Diff revision 1)
> +    Iterator(const RubyColumn& aColumn, int32_t aIndex)

Are you sure you want to make this Iterator constructor public? (and allow any arbitrary aIndex to be passed in)

I expect we only want to be instantiated via |begin| and |end|, right?  (via range-based for loops)

If possible, consider making this constructor private (and tagging RubyColumn as a friend, if necessary for its begin/end methods.  Though IIRC it might get access to inner-class guts automatically).

::: layout/generic/RubyUtils.h:136
(Diff revision 1)
> +      MOZ_ASSERT(aIndex == -1 || aIndex <= aColumn.mTextFrames.Length());

The condition after || here should also check that aIndex is >=0.

(We expect that it's either -1, or it's a valid array-index (or the length of the array).)

::: layout/generic/RubyUtils.h:161
(Diff revision 1)
> +    // non-negitive means the index of ruby text frame

Typo: s/git/gat/ (should be "negative").

Also, might be worth calling out mTextFrames.Length() as a possibility here -- since that's a value we expect here, & it's a non-negative number, but it's not "the index of ruby text frame". Maybe add:
 // A value of mTextFrames.Length() means we're done iterating.

::: layout/generic/RubyUtils.cpp:61
(Diff revision 1)
> +RubyColumn::Iterator::operator*() const

Can you assert here that the return value of this function (operator*) is non-null?  (This may require restructuring it slightly, to e.g. capture the return value in a single local variable so you can assert about it before you return.)

I think it's your intent that this only return non-null things -- but it's non-obvious from looking at operator* that this must be the case.  It falls out of your SkipToFirstExisting() impl, and the fact that we call it everywhere that mIndex is set/modified.

::: layout/generic/RubyUtils.cpp:63
(Diff revision 1)
> +  if (mIndex < 0) {

This should probably be "mIndex == -1".  (That's what you check everywhere else, at least. The <0 check here suggests that we expect other negative numbers.)

::: layout/generic/RubyUtils.cpp:70
(Diff revision 1)
> +RubyColumn::Iterator::SkipToFirstExisting()

"First" is misleading in this function name. Really, there is only one "First existing frame", and it doesn't depend on where your mIndex is currently at. (I think you mean "first starting at current position".)

I was going to suggest s/First/Next/, but I don't like that either because "next" implies we don't consider the frame we're on right now (but this function does consider that frame).

SO: Consider renaming to something like "SkipUntilExistingFrame" or "AdvanceToExistingFrame", or something else that better-captures what this does.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Comment on attachment 8700454 [details]
> MozReview Request: Bug 1229437 part 3 - Support iterating frames of
> RubyColumn.
> 
> https://reviewboard.mozilla.org/r/28719/#review25653
> 
> ::: layout/generic/RubyUtils.h:132
> (Diff revision 1)
> > +    Iterator(const RubyColumn& aColumn, int32_t aIndex)
> 
> Are you sure you want to make this Iterator constructor public? (and allow
> any arbitrary aIndex to be passed in)
> 
> I expect we only want to be instantiated via |begin| and |end|, right?  (via
> range-based for loops)
> 
> If possible, consider making this constructor private (and tagging
> RubyColumn as a friend, if necessary for its begin/end methods.  Though IIRC
> it might get access to inner-class guts automatically).

FYI, no, it doesn't get access to inner-class private automatically. IIRC, Java does so, not C++.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8700452 [details]
MozReview Request: Bug 1229437 part 1 - Add a helper function to get the float containing block of a given frame. r=dbaron

https://reviewboard.mozilla.org/r/28715/#review28875

::: layout/generic/nsInlineFrame.cpp:320
(Diff revision 1)
> -  do {
> +  if (frameBlock == aOurLineContainer)

To preserve what the old code is doing, shouldn't this be:

if (!frameBlock || frameBlock == aOurLineContainer) {
  return;
}

(also please {} the return)

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8700453 [details]
MozReview Request: Bug 1229437 part 2 - Reparent floats inside pulled ruby segment. r=dbaron

https://reviewboard.mozilla.org/r/28717/#review29091

::: layout/generic/nsRubyFrame.cpp:364
(Diff revision 1)
> +  nsBlockFrame* frameBlock =
> +    nsLayoutUtils::GetFloatContainingBlock(baseFrame);

Please rename frameBlock to something more meaningful like oldFloatCB

::: layout/generic/nsRubyFrame.cpp:375
(Diff revision 1)
> +  if (nsBlockFrame* ourFrameBlock =

maybe rename to newFloatCB?

::: layout/generic/nsRubyFrame.cpp:378
(Diff revision 1)
> +      ourFrameBlock->ReparentFloats(baseFrame, frameBlock, true);

I think (although I'm not particularly confident) that you want to pass false here rather than true.  (That's what nsInlineFrame::PullOneFrame does.)  I think we generally want to use false when pulling a frame from a next-continuation, and true when pushing overflow to a next-continuation.

In particular, I think passing true is likely to cause misbehavior if there's a later float not in the same ruby segment that shouldn't get reparented.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8700455 [details]
MozReview Request: Bug 1229437 part 4 - Reparent floats inside pulled ruby column.

https://reviewboard.mozilla.org/r/28721/#review29097

::: layout/generic/nsRubyBaseContainerFrame.cpp:761
(Diff revision 1)
> +    // We are not pulling an intra-level whitespace, which means all
> +    // elements we are going to pull can have non-whitespace content,
> +    // which may contain float which we need to reparent.

Are you confident that something with a float placeholder can't count as whitespace?  (In many contexts it can.)

::: layout/generic/nsRubyBaseContainerFrame.cpp:764
(Diff revision 1)
> +    nsBlockFrame* frameBlock = nullptr;

again, please call this oldFloatCB or similar

::: layout/generic/nsRubyBaseContainerFrame.cpp:769
(Diff revision 1)
> +#ifdef DEBUG
> +    bool frameFound = false;
> +    for (nsIFrame* frame : aColumn) {
> +      frameFound = true;
> +      MOZ_ASSERT(nsLayoutUtils::GetFloatContainingBlock(frame) == frameBlock,
> +                 "All frames in the same ruby column should share "
> +                 "the same float containing block");
> +    }
> +    MOZ_ASSERT(!frameFound || frameBlock,
> +               "Must have float containing block if there is any frame");
> +#endif

Please put {} around the entire contents of the #ifdef DEBUG section (inside the ifdef) and indent everything in them 2 spaces, to avoid the frameFound variable leaking outside the #ifdef.

::: layout/generic/nsRubyBaseContainerFrame.cpp:781
(Diff revision 1)
> +      if (nsBlockFrame* ourFrameBlock =
> +          nsLayoutUtils::GetAsBlock(aLineLayout->LineContainerFrame())) {
> +        if (frameBlock != ourFrameBlock) {

Please rename ourFrameBlock to newFloatCB or similar.

And maybe do that outside the if, and just have a single condition
  if (newFloatCB && oldFloatCB != newFloatCB) {

Also, what does it mean for this to be null?  Can you assert that either both or neither of old and new are null?  (Or, maybe even better... could you just assert that neither is null?  Can ruby ever occur without a block frame ancestor?)

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8700456 [details]
MozReview Request: Bug 1229437 part 5 - Add crashtests for this bug. r=dbaron

https://reviewboard.mozilla.org/r/28723/#review29099

Did you test that they crash without the patches?  (If so, maybe worth saying so in the commit message.)

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #17)
> ::: layout/generic/nsRubyFrame.cpp:378
> (Diff revision 1)
> > +      ourFrameBlock->ReparentFloats(baseFrame, frameBlock, true);
> 
> I think (although I'm not particularly confident) that you want to pass
> false here rather than true.  (That's what nsInlineFrame::PullOneFrame
> does.)  I think we generally want to use false when pulling a frame from a
> next-continuation, and true when pushing overflow to a next-continuation.

I think that depends. I use true here simply because there may be more than one frames need to be reparented (the base container and all its text containers).

I think nsInlineFrame::PullOneFrame passes false because it pull one frame, while we are pulling one ruby segment, which can contain several frames.

> In particular, I think passing true is likely to cause misbehavior if
> there's a later float not in the same ruby segment that shouldn't get
> reparented.

I feel confusing from this comment. It seems to me that, with true passed, it would follow the next sibling pointer to reparent floats in all silbings after. And IIRC, the next sibling pointer doesn't point to a frame in its containers' next continuation, does it?

If it doesn't as what I expect, then we would only reparent floats in the ruby segment we just pull.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #18)
> Comment on attachment 8700455 [details]
> MozReview Request: Bug 1229437 part 4 - Reparent floats inside pulled ruby
> column.
> 
> https://reviewboard.mozilla.org/r/28721/#review29097
> 
> ::: layout/generic/nsRubyBaseContainerFrame.cpp:761
> (Diff revision 1)
> > +    // We are not pulling an intra-level whitespace, which means all
> > +    // elements we are going to pull can have non-whitespace content,
> > +    // which may contain float which we need to reparent.
> 
> Are you confident that something with a float placeholder can't count as
> whitespace?  (In many contexts it can.)

A frame would be intra-level whitespace only if it only has one child, and the content of the child is whitespace only, which is checked via TextIsOnlyWhitespace(). AFAICS, that function returns false for all non-data nodes.

> ::: layout/generic/nsRubyBaseContainerFrame.cpp:781
> (Diff revision 1)
> > +      if (nsBlockFrame* ourFrameBlock =
> > +          nsLayoutUtils::GetAsBlock(aLineLayout->LineContainerFrame())) {
> > +        if (frameBlock != ourFrameBlock) {
> 
> Please rename ourFrameBlock to newFloatCB or similar.
> 
> And maybe do that outside the if, and just have a single condition
>   if (newFloatCB && oldFloatCB != newFloatCB) {
> 
> Also, what does it mean for this to be null?  Can you assert that either
> both or neither of old and new are null?  (Or, maybe even better... could
> you just assert that neither is null?  Can ruby ever occur without a block
> frame ancestor?)

Hmmm. I think this part can be revised to be more clear.

> Comment on attachment 8700456 [details]
> MozReview Request: Bug 1229437 part 5 - Add crashtests for this bug.
> 
> https://reviewboard.mozilla.org/r/28723/#review29099
> 
> Did you test that they crash without the patches?  (If so, maybe worth
> saying so in the commit message.)

Yes. I've noted that in comment 7.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700452 [details]
MozReview Request: Bug 1229437 part 1 - Add a helper function to get the float containing block of a given frame. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28715/diff/1-2/

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700453 [details]
MozReview Request: Bug 1229437 part 2 - Reparent floats inside pulled ruby segment. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28717/diff/1-2/

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700454 [details]
MozReview Request: Bug 1229437 part 3 - Support iterating frames of RubyColumn. r=dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28719/diff/1-2/

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700455 [details]
MozReview Request: Bug 1229437 part 4 - Reparent floats inside pulled ruby column.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28721/diff/1-2/

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700456 [details]
MozReview Request: Bug 1229437 part 5 - Add crashtests for this bug. r=dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28723/diff/1-2/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8700455 [details]
MozReview Request: Bug 1229437 part 4 - Reparent floats inside pulled ruby column.

Note that there is an issue on the interdiff of this commit that some changed lines are not properly highlighted. I've filed bug 1242873 for this issue.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #20)
> > In particular, I think passing true is likely to cause misbehavior if
> > there's a later float not in the same ruby segment that shouldn't get
> > reparented.
> 
> I feel confusing from this comment. It seems to me that, with true passed,
> it would follow the next sibling pointer to reparent floats in all silbings
> after. And IIRC, the next sibling pointer doesn't point to a frame in its
> containers' next continuation, does it?
> 
> If it doesn't as what I expect, then we would only reparent floats in the
> ruby segment we just pull.

Ah, you're right.  As long as the ruby segment has already been pulled (and disconnected from its later siblings), this should be ok.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8700455 [details]
MozReview Request: Bug 1229437 part 4 - Reparent floats inside pulled ruby column.

I can't figure out how to get the raw diff out of MozReview for the *previous* reviewed version, which means I can't generate a real interdiff.  Therefore, review denied.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Please attach a non-MozReview patch for both the previous version that I reviewed and the current version for which you're requesting review.)

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, never mind, I found the well-hidden "Download Diff" link in the top bar, which is actually correct.  (As opposed to the ones that are broken as described in bug 1234279.)

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8700455 [details]
MozReview Request: Bug 1229437 part 4 - Reparent floats inside pulled ruby column.

https://reviewboard.mozilla.org/r/28721/#review29211

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04c819d845d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd7ad84ece9

Comment 34

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36db8bd303f1f440f6bd210b89fe9191c10f5c95
Backed out changeset 4dd7ad84ece9 (bug 1229437)

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1d72ffd535a25dd29454d4503ccfef80cb78fb
Backed out changeset 04c819d845d5 (bug 1229437)

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3f1250cfc8fd18d9815b8314524b08caabe8bc
Bug 1229437 part 1 - Add a helper function to get the float containing block of a given frame. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/d26720c9d6a07ba9ca1a8d3b5d1d3aa6c9073220
Bug 1229437 part 2 - Reparent floats inside pulled ruby segment. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/daa693feccc504cc84960971678b84f221681c8c
Bug 1229437 part 3 - Support iterating frames of RubyColumn. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/96478aae9d0ea1495806dc49683e356aec73e8e5
Bug 1229437 part 4 - Reparent floats inside pulled ruby column. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c8ee9d178dbe40dfc348638a9e19d36f65bae4
Bug 1229437 part 5 - Add crashtests for this bug. r=dbaron

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/650a0b9d45398d7bcfa772f05862730d2b88795e
Bug 1229437 followup - Fix sign-compare error in RubyColumn::Iterator on CLOSED TREE.

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3624090bdcf26024a075f9357f76537a01e7f838
Bug 1229437 followup 2 - Fix another sign-compare error in RubyColumn::Iterator on CLOSED TREE.

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/695f43695c1756cddedd26c272976af756dc741c
Bug 1229437 followup 3 - Fix a mistake in RubyColumn::Iterator::SkipUntilExistingFrame(). a=me

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f3f1250cfc8
https://hg.mozilla.org/mozilla-central/rev/d26720c9d6a0
https://hg.mozilla.org/mozilla-central/rev/daa693feccc5
https://hg.mozilla.org/mozilla-central/rev/96478aae9d0e
https://hg.mozilla.org/mozilla-central/rev/f9c8ee9d178d
https://hg.mozilla.org/mozilla-central/rev/650a0b9d4539
https://hg.mozilla.org/mozilla-central/rev/3624090bdcf2
https://hg.mozilla.org/mozilla-central/rev/695f43695c17

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f3f1250cfc8
https://hg.mozilla.org/mozilla-central/rev/d26720c9d6a0
https://hg.mozilla.org/mozilla-central/rev/daa693feccc5
https://hg.mozilla.org/mozilla-central/rev/96478aae9d0e
https://hg.mozilla.org/mozilla-central/rev/f9c8ee9d178d
https://hg.mozilla.org/mozilla-central/rev/650a0b9d4539
https://hg.mozilla.org/mozilla-central/rev/3624090bdcf2
https://hg.mozilla.org/mozilla-central/rev/695f43695c17