1062963 - Make nsFloatManager use logical coordinates

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

For vertical text support, we need to convert nsFloatManager to use logical coordinates.

Comment 1

[Simon Montagu :smontagu]

Attachment: Bad w-i-p patch

This patch is buggy, attaching for reference.

Comment 2

[Simon Montagu :smontagu]

Try push showing the test failures from attachment 8484289 [details] [diff] [review]: https://tbpl.mozilla.org/?tree=Try&rev=7b6c7af6db3b

Comment 3

[Simon Montagu :smontagu]

Attachment: w-i-p patch v.2

This fixes the assertions from the previous version, but there are still test failures https://tbpl.mozilla.org/?tree=Try&rev=12154ad669d5

Comment 4

[Simon Montagu :smontagu]

Attachment: Patch 1: nsLineLayout::ReflowFrame and CanPlaceFrame

I split the w-i-p patch into smaller chunks in order to find the bugs more easily -- I assume that reviewing them that way is easier too, but let me know if you prefer a roll-up patch. The earlier ones are only name and comment changes.

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch 2: name and comment changes in nsFloatManager itself

Comment 6

[Simon Montagu :smontagu]

Attachment: Patch 3: additions to the WritingModes API

Comment 7

[Simon Montagu :smontagu]

Attachment: Patch 4: make nsFloatManager's origin a LogicalPoint, and adapt GetFlowAreas, AddFloats, ClearFloats, etc. to use it

Comment 8

[Simon Montagu :smontagu]

Attachment: patch 5: make nsFloatManager region functions work with logical regions

I have never been quite sure how much of the float manager's internals to logicalize. What I've ended up with is using logical coordinates for all the calculations, but storing the frame regions as physical margins.

There is apparently no guarantee that a float manager's caller has the same writing mode as the original caller that added floats. Apart from cases where we are reflowing after a change of direction, it can also happen that the same float manager is called by a block frame and by its containing block which has a different writing mode.

Comment 9

[Simon Montagu :smontagu]

I think those patches are a sufficiently adult-sized portion for this bug. The other stuff from the original w-i-p will go into bug 1079139.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500895 [details] [diff] [review]
Patch 3: additions to the WritingModes API

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

::: layout/generic/WritingModes.h
@@ +509,5 @@
>  
> +  bool operator==(LogicalPoint aOther) const
> +  {
> +    return mWritingMode == aOther.mWritingMode &&
> +           B() == aOther.B() && I() == aOther.I();

Uh-oh ... this is wrong: the mWritingMode member is for DEBUG use only. It should have been #ifdef'd out of non-DEBUG builds altogether; the fact that this compiles in an opt build is a bug!

And on looking, I see that we've got the same problem in the LogicalSize == and != operators. Filed bug 1079314.

So this will need to be changed to do a standard CHECK_WRITING_MODE instead, and callers will need to be audited to make sure they're using it correctly (never passing LogicalPoints with different writing modes to the comparison operator).

@@ +1326,5 @@
>  
> +  bool IsEqualEdges(const LogicalRect aOther) const
> +  {
> +    return mWritingMode == aOther.mWritingMode &&
> +      mRect.IsEqualEdges(aOther.mRect);

As above, the mWritingMode comparison here is invalid.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500890 [details] [diff] [review]
Patch 2: name and comment changes in nsFloatManager itself

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

I started commenting on some disturbing mixtures of coordinate systems here, but on looking ahead, I see that the next patch is going to fix a bunch of this. So it may be OK to ignore these remarks ... I'll come back to this after working through more of the patches.

::: layout/generic/nsFloatManager.cpp
@@ +115,3 @@
>                              SavedState* aState) const
>  {
> +  NS_ASSERTION(aBSize >= 0, "unexpected max height");

s/height/block-size/

@@ +139,5 @@
>    if (floatCount == 0 ||
> +      (mFloats[floatCount-1].mLeftBEnd <= blockStart &&
> +       mFloats[floatCount-1].mRightBEnd <= blockStart)) {
> +    return nsFlowAreaRect(aContentArea.x, aBOffset, aContentArea.width,
> +                          aBSize, false);

This doesn't look right - it's passing physical coordinates from aContentArea to nsFlowAreaRect's iCoord and iSize, which won't be right for vertical writing modes.

@@ +158,4 @@
>      }
>    }
> +  nscoord inlineStart = mICoord + aContentArea.x;
> +  nscoord inlineEnd = mICoord + aContentArea.XMost();

Here, too, the mix of logical and physical looks broken.

@@ +178,5 @@
>        // disagrees with the spec.  (We might want to fix this in the
>        // future, though.)
>        continue;
>      }
> +    nscoord floatBStart = fi.mRect.y, floatBEnd = fi.mRect.YMost();

FloatInfo's mRect is still physical, I presume; in which case this is mixing physical and logical.

To do the right thing here, I think the FloatManager needs to know what writing mode it's working with -- i.e. what its mICoord etc actually mean.

@@ +252,5 @@
>    NS_ASSERTION(floatStyle == NS_STYLE_FLOAT_LEFT ||
>                 floatStyle == NS_STYLE_FLOAT_RIGHT, "unexpected float");
> +  nscoord& sideBEnd = (floatStyle == NS_STYLE_FLOAT_LEFT) ? info.mLeftBEnd
> +                                                           : info.mRightBEnd;
> +  nscoord thisBEnd = info.mRect.YMost();

logical/physical?

::: layout/generic/nsFloatManager.h
@@ +38,2 @@
>                   bool aHasFloats)
> +    : mRect(aICoord, aBCoord, aISize, aBSize), mHasFloats(aHasFloats) {}

I wonder - should we add a (DEBUG-only) mWritingMode field to nsFlowAreaRect, and make callers pass the mode that they're working with (like with the logical-geometry classes)? ISTM that might help us to guard against inadvertently mixing modes. It's a bit of extra clutter in the source, I realize, but may be worth it for the added validation.

@@ +245,4 @@
>     *
>     * The result is relative to the current translation.
>     */
>    nscoord GetLowestFloatTop() const;

maybe s/GetLowestFloatTop/GetLastFloatBStart/?

@@ +306,5 @@
>  #endif
>    };
>  
> +  nscoord mICoord, mBCoord;  // translation from local to global
> +                             // coordinate space

I suspect we want to add an mWritingMode field to nsFloatManager, so that it knows what these fields mean. And it may need an mContainerWidth field, if it's going to need to convert things?

Comment 12

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #11)

> but on looking ahead, I see that the next patch is going to fix a bunch of

s/the next patch/patch 4/, of course.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500902 [details] [diff] [review]
patch 5: make nsFloatManager region functions work with logical regions

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

::: layout/generic/nsFloatManager.h
@@ +75,5 @@
>     * Store the float region on the frame. The region is stored
>     * as a delta against the mRect, so repositioning the frame will
>     * also reposition the float region.
>     */
> +  static void StoreRegionFor(nsIFrame* aFloat, const nsRect& aRegion);

It seems odd that Get/CalculateRegionFor() return logical rects, but StoreRegionFor still takes a physical rect. Is there a strong reason to have this API mismatch, or could we make this logical as well?

Comment 14

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> 
> It seems odd that Get/CalculateRegionFor() return logical rects, but
> StoreRegionFor still takes a physical rect. Is there a strong reason to have
> this API mismatch, or could we make this logical as well?

I originally had StoreRegion taking a logical rect as well, but doing so adds a lot of bulk which made it seem like overkill: aFloat->GetLogicalRect instead of GetRect, and having to add an operator- to LogicalRect which returns a LogicalMargin. Also I was worried about what would happen if the writing mode changed so that IsEqualEdges returned false even the physical coordinates were the same.

I suppose one could avoid all that by passing in a logical rect and doing the logical->physical conversion right at the beginning. Would you prefer that for API consistency?

Comment 15

[Simon Montagu :smontagu]

Attachment: Patch 3 v.2: additions to the WritingModes API

Updated to bug 1079314

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500890 [details] [diff] [review]
Patch 2: name and comment changes in nsFloatManager itself

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

OK, I think the concerns I had when I started reading this are temporary issues that get resolved by later patches here. I'm essentially ready to r+ all these (with a few comments/suggestions), but I suggest we hold off on landing them until after next week's merge, so as to give us the maximum testing time on Nightly to flush out any issues.

Actually, what I'd suggest here is that you fold patches 2, 4 and 5 together, to eliminate some of the more jarring "partly-converted" stages. Patches 1 and 3 (which would then become patch 2, as it needs to precede the main work here) are pretty much standalone, but these three look like they'd make more sense merged. If you agree with that, could you maybe post a refreshed version of the resulting patch for one more read-through?

Comment 17

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500899 [details] [diff] [review]
Patch 4: make nsFloatManager's origin a LogicalPoint, and adapt GetFlowAreas, AddFloats, ClearFloats, etc. to use it

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

As noted above, I suggest merging this with patches 2 and 5.

In any case, the patch numbers in the commit messages here look out-of-date; if not merging the patches, please remember to refresh them to avoid confusion.

::: layout/generic/nsBlockFrame.cpp
@@ +1841,2 @@
>      // with aLine's floats
>      nscoord lineYA = aLine->BStart() + aDeltaY;

Hmm, is aDeltaY going to become aDeltaB at some point in this collection of patches? Seems like we're still mixing physical and logical here...

@@ +1844,5 @@
>      // Scrollable overflow should be sufficient for things that affect
>      // layout.
>      nsRect overflow = aLine->GetOverflowArea(eScrollableOverflow);
>      nscoord lineYCombinedA = overflow.y + aDeltaY;
>      nscoord lineYCombinedB = lineYCombinedA + overflow.height;

...while these are purely physical computations...

@@ +1847,5 @@
>      nscoord lineYCombinedA = overflow.y + aDeltaY;
>      nscoord lineYCombinedB = lineYCombinedA + overflow.height;
> +    WritingMode wm = aState.mReflowState.GetWritingMode();
> +    if (floatManager->IntersectsDamage(wm, lineYA, lineYB) ||
> +        floatManager->IntersectsDamage(wm, lineYCombinedA, lineYCombinedB)) {

...but here they're being passed as logical values.

::: layout/generic/nsBlockReflowState.cpp
@@ +291,5 @@
> +  if (wWM == mFloatManagerWM) {
> +    NS_ASSERTION(wPt == mFloatManagerOrigin, "bad coord system");
> +  } else {
> +    //XXX if the writing modes are different we can't easily assert that
> +    //    the origin is the same. 

Can't we do something like
  NS_ASSERTION(wPt == mFloatManagerOrigin.ConvertTo(wWM, ...), ...)
here? Or is that not valid?

::: layout/generic/nsContainerFrame.cpp
@@ +1173,5 @@
>      }
>      else {
>        tracker.Skip(frame, aStatus);
>        if (aReflowState.mFloatManager)
> +        nsBlockFrame::RecoverFloatsFor(frame, *aReflowState.mFloatManager,

nit: please add the missing braces for the "if" while you're here.

::: layout/generic/nsFloatManager.cpp
@@ +399,5 @@
>    // reflow mentioned above) and then from B to C during the subsequent
>    // reflow. In the typical case A and C will be the same, but not always.
>    // Allowing mFloatDamage to accumulate the damage incurred during both
>    // reflows ensures that nothing gets missed.
> +  aState->mOrigin = mOrigin;

Should PushState/PopState be saving and restoring the writing mode along with the origin?

Or if the WM will never change, should callers pass the mode they're working with and PushState/PopState assert that it matches the mode for which the SavedState was constructed?

::: layout/generic/nsFloatManager.h
@@ +75,5 @@
>  
>    // Structure that stores the current state of a frame manager for
>    // Save/Restore purposes.
>    struct SavedState;
>    friend struct SavedState;

While you're here, how about removing these two redundant declarations?

@@ +299,5 @@
>    bool ClearContinues(uint8_t aBreakType) const;
>  
>    void AssertStateMatches(SavedState *aState) const
>    {
> +    NS_ASSERTION(aState->mOrigin == mOrigin &&

Include mWritingMode in the assertion tests?

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500902 [details] [diff] [review]
patch 5: make nsFloatManager region functions work with logical regions

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

::: layout/generic/nsBlockReflowState.cpp
@@ +908,4 @@
>    NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "bad float placement");
>    // store region
> +  nsFloatManager::StoreRegionFor(aFloat,
> +                                 region.GetPhysicalRect(wm, mContainerWidth));

Could StoreRegionFor be made to take a LogicalRect? (As per earlier comment.)

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #18)

> Could StoreRegionFor be made to take a LogicalRect? (As per earlier comment.)

Oops, I see you replied to that in comment 14 - sorry. OK. I'm inclined to think the API consistency you suggest there would be nice; it won't really make any difference to the overall complexity, but it'll make the class less confusing to use, I think.

Comment 20

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #17)

Maybe I also need to recombine here the patches that I split off from this bug and intended to put in bug 1079139 in order to answer your questions about aDeltaY, lineYCombinedA/B etc.

> > +    //XXX if the writing modes are different we can't easily assert that
> > +    //    the origin is the same. 
> 
> Can't we do something like
>   NS_ASSERTION(wPt == mFloatManagerOrigin.ConvertTo(wWM, ...), ...)
> here? Or is that not valid?

Not just like that: to convert the origin of an object you can't just convert a point from one writing mode to another -- the origin in the new writing mode might be a different physical point, e.g. the top right corner instead of the top left corner. With a little more work we could still make the conversion, but I wasn't confident that the origin would be constant anyway, say if there has been a resize and a direction change or something like that.

> > +  aState->mOrigin = mOrigin;
> 
> Should PushState/PopState be saving and restoring the writing mode along
> with the origin?

Yes indeed, I thought I'd done that, and ditto with your comment on AssertStateMatches below ;-)

Comment 21

[Simon Montagu :smontagu]

Attachment: Patch 2,4 and 5 combined

For now, just patches 2 (which will become 3), 4 and 5 combined.

Comment 22

[Jonathan Kew [:jfkthame]]

Comment on attachment 8503984 [details] [diff] [review]
Patch 2,4 and 5 combined

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

This mostly seems fine, except for one place in nsBlockReflowState.cpp where it looks like modes are mixed up - please double-check this and tell me if I'm misunderstanding altogether...

::: layout/generic/nsBlockFrame.cpp
@@ +6030,5 @@
>      // If the element is relatively positioned, then adjust x and y
>      // accordingly so that we consider relatively positioned frames
>      // at their original position.
> +    LogicalPoint pos = block->GetLogicalNormalPosition(aWM, aContainerWidth);
> +    WritingMode oldWM =  aFloatManager.Translate(aWM, pos, aContainerWidth);

nit: there's an extra space after =

::: layout/generic/nsBlockReflowState.cpp
@@ +560,5 @@
> +  WritingMode oldWM;
> +  LogicalPoint oPt(wm);
> +  mFloatManager->GetTranslation(oldWM, oPt);
> +  LogicalPoint dPt = mFloatManagerOrigin.ConvertTo(wm, mFloatManagerWM,
> +                                                   mContainerWidth) - oPt;

This looks a bit broken. AIUI, |oPt| is a LogicalPoint with writing mode |oldWM| (which we just got from mFloatManager), but we're subtracting it from a point which has just been converted to |wm|. So is oldWM always equal to wm? If not, LogicalPoint's operator- will assert here, and the result may be nonsense.

Comment 23

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #21)
> Created attachment 8503984 [details] [diff] [review]
> Patch 2,4 and 5 combined
> 
> For now, just patches 2 (which will become 3), 4 and 5 combined.

BTW, it looks like the old patch 2 (names and comments in nsFloatManager) didn't make it into this combined patch yet. No big deal, I'm sure you'll have a coherent patch queue altogether when it comes to landing! Just noting that this didn't quite match its claimed description.

Comment 24

[Simon Montagu :smontagu]

(In reply to Jonathan Kew (:jfkthame) from comment #22)
> This looks a bit broken. AIUI, |oPt| is a LogicalPoint with writing mode
> |oldWM| (which we just got from mFloatManager), but we're subtracting it
> from a point which has just been converted to |wm|.

Looking at the original code again, I think it is itself if not broken then at least rather hacky: it's calling nsFloatManager::Translate with the difference between the current translation just returned by GetTranslation and the original result of GetTranslation saved by the constructor in mFloatManagerX/Y. Translate itself adds its input to the current values, so in effect this is equivalent to a hypothetical SetTranslate call with mFloatManagerX/Y.

As you say, doing maths on two points not necessarily in the same writing mode is nonsense, but I think the overloading of Translate is not good in the first place and it's better to just add a SetTranslate method. Does that make sense?

Comment 25

[Simon Montagu :smontagu]

Attachment: Patch 2,4 and 5 combined, v2

Including patch 2 this time, and with the changes from the last round of review comments

Comment 26

[Simon Montagu :smontagu]

Attachment: Interdiff of the changes from comment 22 and comment 24

Comment 27

[Jonathan Kew [:jfkthame]]

(In reply to Simon Montagu :smontagu from comment #25)
> Created attachment 8505499 [details] [diff] [review]
> Patch 2,4 and 5 combined, v2
> 
> Including patch 2 this time, and with the changes from the last round of
> review comments

Looks like the wrong file got attached here -- oops!

Comment 28

[Simon Montagu :smontagu]

Attachment: Patch 2,4 and 5 combined, v2, correct patch

Comment 29

[Jonathan Kew [:jfkthame]]

Comment on attachment 8506305 [details] [diff] [review]
Patch 2,4 and 5 combined, v2, correct patch

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

Looking good; let's get this landed!

Comment 30

[Jonathan Kew [:jfkthame]]

Comment on attachment 8500899 [details] [diff] [review]
Patch 4: make nsFloatManager's origin a LogicalPoint, and adapt GetFlowAreas, AddFloats, ClearFloats, etc. to use it

Clearing r? on older patches that are now merged into the final reviewed version.

Comment 32

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71211c4a4acc
https://hg.mozilla.org/integration/mozilla-inbound/rev/90172cc0b012
https://hg.mozilla.org/integration/mozilla-inbound/rev/241c23570a62

Comment 33

[Phil Ringnalda (:philor)]

Backed out both this and bug 1079139 in https://hg.mozilla.org/integration/mozilla-inbound/rev/2c490d1c97b0 because I didn't know which caused the failures in 427129-table-caption.html (permaorange in b2g reftest-6, intermittent in Android 2.3 reftest-5).

Comment 34

[Simon Montagu :smontagu]

I've narrowed the failure down to what was patch 4 (attachment 8500899 [details] [diff] [review]), but the test is impenetrable to say the least and I can't tell how serious the failure is. I'm tempted just to add
|fails-if(B2G) random-if(Android)|. WDYT?

Comment 35

[Jonathan Kew [:jfkthame]]

It's tempting, but I'm a bit reluctant until we have some clue as to why things have changed. And why it's apparently platform-specific; that's weird. Maybe we can extract a single failing line from the reftest and look into it more?

Comment 36

[Jonathan Kew [:jfkthame]]

I've found a way to reproduce the problem here in a desktop build on OS X, and tracked down what I think it probably the cause.

STR, using a build that includes the three patches here:

* Load layout/reftests/bugs/427129-table-caption.html.
* Then resize the browser window slightly.

Note that although the testcase initially renders correctly (as confirmed by comparing the reference), when you resize the window (even though keeping it plenty big enough for the whole testcase to display), the lower half of the lines suddenly "snap" to the incorrect rendering, like that seen in the b2g-emulator reftest failures.

This doesn't happen, of course, with a current trunk build; it's a result of the patches here. It only affects the RTL elements in the testcase.

In a debug build, this triggers a bunch of assertions like this:

[78605] ###!!! ASSERTION: writing-mode mismatch: 'aPoint.GetWritingMode() == GetWritingMode()', file /Users/jkew/mozdev/mc-vert-layout/layout/generic/WritingModes.h, line 1388

Program received signal SIGTRAP, Trace/breakpoint trap.
0x00007fff90a6782a in __kill ()
(gdb) bt 20
#0  0x00007fff90a6782a in __kill ()
#1  0x000000010160b772 in RealBreak () at /Users/jkew/mozdev/mc-vert-layout/xpcom/base/nsDebugImpl.cpp:478
#2  0x000000010160b261 in Break (aMsg=0x7fff5fbf6238 "[78605] ###!!! ASSERTION: writing-mode mismatch: 'aPoint.GetWritingMode() == GetWritingMode()', file /Users/jkew/mozdev/mc-vert-layout/layout/generic/WritingModes.h, line 1388") at /Users/jkew/mozdev/mc-vert-layout/xpcom/base/nsDebugImpl.cpp:567
#3  0x000000010160ad8a in NS_DebugBreak (aSeverity=1, aStr=0x108335c1c "writing-mode mismatch", aExpr=0x108356b0c "aPoint.GetWritingMode() == GetWritingMode()", aFile=0x1083471a0 "/Users/jkew/mozdev/mc-vert-layout/layout/generic/WritingModes.h", aLine=1388) at /Users/jkew/mozdev/mc-vert-layout/xpcom/base/nsDebugImpl.cpp:461
#4  0x0000000104e6f425 in mozilla::LogicalRect::operator+ (this=0x7fff5fbf6820, aPoint=@0x1280ff284) at WritingModes.h:1388
#5  0x0000000104e0b83f in nsFloatManager::AddFloat (this=0x1280ff280, aFloatFrame=0x1287355b0, aMarginRect=@0x7fff5fbf6820, aWM={mWritingMode = 18 '\022'}, aContainerWidth=34800) at nsFloatManager.cpp:258
#6  0x0000000104de1ffe in nsBlockReflowState::RecoverFloats (this=0x7fff5fbf73a0, aLine={mCurrent = 0x1287faec0, mListLink = 0x1287349d8}, aDeltaBCoord=0) at nsBlockReflowState.cpp:465
#7  0x0000000104dceb37 in nsBlockReflowState::RecoverStateFrom (this=0x7fff5fbf73a0, aLine={mCurrent = 0x1287faec0, mListLink = 0x1287349d8}, aDeltaBCoord=0) at nsBlockReflowState.cpp:497
#8  0x0000000104dc9f01 in nsBlockFrame::ReflowDirtyLines (this=0x128734960, aState=@0x7fff5fbf73a0) at nsBlockFrame.cpp:2249
#9  0x0000000104dc6340 in nsBlockFrame::Reflow (this=0x128734960, aPresContext=0x1177c5000, aMetrics=@0x7fff5fbf7f0c, aReflowState=@0x7fff5fbf7c20, aStatus=@0x7fff5fbf7bdc) at nsBlockFrame.cpp:1136
#10 0x0000000104dd3d4a in nsBlockReflowContext::ReflowBlock (this=0x7fff5fbf7ed0, aSpace=@0x7fff5fbf7d28, aApplyBStartMargin=false, aPrevMargin=@0x7fff5fbf8b48, aClearance=0, aIsAdjacentWithBStart=true, aLine=0x1287faf60, aFrameRS=@0x7fff5fbf7c20, aFrameReflowStatus=@0x7fff5fbf7bdc, aState=@0x7fff5fbf8a60) at nsBlockReflowContext.cpp:288
#11 0x0000000104dd0670 in nsBlockFrame::ReflowBlockFrame (this=0x1287345d8, aState=@0x7fff5fbf8a60, aLine={mCurrent = 0x1287faf60, mListLink = 0x128734650}, aKeepReflowGoing=0x7fff5fbf852f) at nsBlockFrame.cpp:3200
#12 0x0000000104dce74e in nsBlockFrame::ReflowLine (this=0x1287345d8, aState=@0x7fff5fbf8a60, aLine={mCurrent = 0x1287faf60, mListLink = 0x128734650}, aKeepReflowGoing=0x7fff5fbf852f) at nsBlockFrame.cpp:2613
#13 0x0000000104dc9b4c in nsBlockFrame::ReflowDirtyLines (this=0x1287345d8, aState=@0x7fff5fbf8a60) at nsBlockFrame.cpp:2151
#14 0x0000000104dc6340 in nsBlockFrame::Reflow (this=0x1287345d8, aPresContext=0x1177c5000, aMetrics=@0x7fff5fbf95cc, aReflowState=@0x7fff5fbf92e0, aStatus=@0x7fff5fbf929c) at nsBlockFrame.cpp:1136
#15 0x0000000104dd3d4a in nsBlockReflowContext::ReflowBlock (this=0x7fff5fbf9590, aSpace=@0x7fff5fbf93e8, aApplyBStartMargin=false, aPrevMargin=@0x7fff5fbfa208, aClearance=0, aIsAdjacentWithBStart=true, aLine=0x128ef61a0, aFrameRS=@0x7fff5fbf92e0, aFrameReflowStatus=@0x7fff5fbf929c, aState=@0x7fff5fbfa120) at nsBlockReflowContext.cpp:288
#16 0x0000000104dd0670 in nsBlockFrame::ReflowBlockFrame (this=0x1287341d0, aState=@0x7fff5fbfa120, aLine={mCurrent = 0x128ef61a0, mListLink = 0x128734248}, aKeepReflowGoing=0x7fff5fbf9bef) at nsBlockFrame.cpp:3200
#17 0x0000000104dce74e in nsBlockFrame::ReflowLine (this=0x1287341d0, aState=@0x7fff5fbfa120, aLine={mCurrent = 0x128ef61a0, mListLink = 0x128734248}, aKeepReflowGoing=0x7fff5fbf9bef) at nsBlockFrame.cpp:2613
#18 0x0000000104dc9b4c in nsBlockFrame::ReflowDirtyLines (this=0x1287341d0, aState=@0x7fff5fbfa120) at nsBlockFrame.cpp:2151
#19 0x0000000104dc6340 in nsBlockFrame::Reflow (this=0x1287341d0, aPresContext=0x1177c5000, aMetrics=@0x7fff5fbfac8c, aReflowState=@0x7fff5fbfa9a0, aStatus=@0x7fff5fbfa95c) at nsBlockFrame.cpp:1136
(More stack frames follow...)

So the problem arises from the writing-mode mismatch in nsFloatManager::AddFloat, which causes the float accounting to be incorrect.

Try run in progress with a patch to fix this: https://tbpl.mozilla.org/?tree=Try&rev=615b692ed852.

Comment 37

[Jonathan Kew [:jfkthame]]

Attachment: followup to fix writing-mode mismatch in nsFloatManager::AddFloat.

This seems to fix the problem in my local desktop build; waiting for tryserver to confirm emulator results.

Comment 38

[Jonathan Kew [:jfkthame]]

Attachment: reftest that catches the floatmanager writing-mode mismatch bug on container resize.

And here's a reftest (extracted and adapted from the test that failed on b2g-emulator here) that asserts and fails on desktop Firefox as well, so it would have gone consistently orange across all platforms.

Comment 39

[Jonathan Kew [:jfkthame]]

One more try run, with the followup patch and reftest, just to check for any unpleasant surprises...
https://tbpl.mozilla.org/?tree=Try&rev=c1f018af194b

Comment 40

[Simon Montagu :smontagu]

Attachment: 1062963.c2.diff

Ha, we seem to have been working in parallel! I think I prefer this way of fixing it, but it amounts to the same thing. What do you think?

Comment 41

[Jonathan Kew [:jfkthame]]

On the surface, at least, it looks like my patch would be slightly cheaper in the case where there aren't any floats present (as the added code doesn't get called at all unless we actually need to add a float), while yours would be cheaper if there are multiple floats to handle (as it potentially does the origin change once for a whole collection of them).

No strong view one way or the other here. I'll r+ yours, so you can pick whichever you decide.

Comment 42

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e81449fe462
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de2c87f94e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2316981708
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d7da5ff1ed

(with attachment #8508785 [details] [diff] [review] folded in)

Comment 43

[Jonathan Kew [:jfkthame]]

Comment on attachment 8508696 [details] [diff] [review]
followup to fix writing-mode mismatch in nsFloatManager::AddFloat.

Just clearing r? here, as we're using Simon's version of the fix instead.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e81449fe462
https://hg.mozilla.org/mozilla-central/rev/1de2c87f94e5
https://hg.mozilla.org/mozilla-central/rev/2a2316981708
https://hg.mozilla.org/mozilla-central/rev/f1d7da5ff1ed

Comment 45

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Fx37 as part of addressing bug 1114329.