403129 - Overlapping floats and BandRect leak

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

Steps to reproduce:
1. Run Firefox with XPCOM_MEM_LEAK_LOG=2
2. Load the testcase.
3. Quit Firefox.

Result: trace-refcnt reports that two BandRects and two nsVoidArrays have leaked.

Unlike in bug 307242, this leak is not accompanied by an assertion failure.

Also, the layout is different than in WebKit.  WebKit puts each float on a separate line; Gecko has them overlap.

Comment 1

[Daniel Holbert [:dholbert]]

> Also, the layout is different than in WebKit.  WebKit puts each float on a
> separate line; Gecko has them overlap.

FWIW, IE7 matches WebKit, wrapping the second float to a new line.

If we increase the body width from 0 to 1px, then Gecko wraps, too.  In fact, on my machine, I can set the width as low as
 trunk    0.008333333px
 branch   0.0333333333px
and keep the wrapping.  Take it any lower, and the wrapping goes away, and the floats overlap.

Comment 2

[Daniel Holbert [:dholbert]]

Here's what I've found, from comparing a testcase with body width = 0px to a reference case with body width = 1px.

In nsBlockReflowState::FlowAndPlaceFloat
 775   while (!CanPlaceFloat(floatSize, floatDisplay->mFloats, aForceFit)) {

In the reference case, we enter this loop (and increment mY by one line-height), but in test case, we don't.

 ---> Why?

In nsBlockReflowState::CanPlaceFloat
 616   if (0 != mBand.GetFloatCount())
 ...
 621       result = PR_FALSE;

In the reference case, GetFloatCount() returns 1; in the test case, it returns 0 and we skip this clause.  (So CanPlaceFloat returns FALSE for ref case vs. TRUE for test case.)

 --> Why?

In nsBlockBandData::ComputeAvailSpaceRect
 209   else if (mTrapezoids[0].mFrames) {
 210     // We have a float using up all the available space
 211     leftFloats = 1;
 212   }
 ...
 216   mLeftFloats = leftFloats;

In the reference case, mTrapezoids[0].mFrames is non-null here sometimes.  In the test case, it's always null.  (So we never increase mLeftFloats, and so GetFloatCount() always returns 0.)

 --> Why?

In nsSpaceManager::GetBandAvailableSpace
 267   while ((aBand->mTop == topOfBand) && (aBand->mLeft < rightEdge)) {
 ...
 296     trapezoid->mFrames = &aBand->mFrames;

In the reference case, we enter this loop, because with aBand->mLeft = 0 and rightEdge = 60.  In the testcase, rightEdge = 0, so we don't enter the loop.  Instead, we end up triggering this code below the loop:
 322   if (left < rightEdge || aBandData.mCount == 0) {
 ...
 328     trapezoid->mFrames = nsnull;

 --> Why?

Because in the reference case, the body is 1px (60 app-units) wide, but in the test case, the body is 0px (0 app-units) wide.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right, so there's nowhere to place the float and we fall back. I guess we just need to modify the fallback path so that when there are no bands we advance to the bottom of the floats using ClearFloats or something.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: possible patch

This patch fixes both the float-wrapping bug and the memory leak, by allowing us to enter the loop in nsSpaceManager::GetBandAvailableSpace in this edge case where rightEdge == 0. (Getting at the root of the problem, as diagnosed in my last post)

Not sure if this patch would have bad ramifications, though -- I'm not especially familiar with the space manager code.  If someone else could confirm whether this makes sense, that'd be awesome.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #3)

Oops, hadn't seen roc's comment when I posted "possible patch" & comment #4.  This patch probably hits at the wrong place, then.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: reftest

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch v2

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch v3

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch v4 (with reftests)

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 288871 [details] [diff] [review]
patch v4 (with reftests)

Justification for patch:
========================
This patch fixes how we handle zero-width clipping regions[1] that are aligned exactly at the edge of an (occupied) BandRect.

Previously, we wouldn't recognize this situation as an overlap, and we'd return an "available-space"[2] trapezoid with zero width. (which is set up here:
http://mxr.mozilla.org/seamonkey/source/layout/generic/nsSpaceManager.cpp#328 )
This was bad because
   a) a zero-width "available space" trapezoid makes no sense in principle
   b) we behaved very differently for very-small clipping regions vs. 0-width clipping regions. (see comment #1)

Now we recognize this situation as overlap, and we return a trapezoid indicating that the space is unavailable, as we do in the very-small clipping region case.  So both bad things ((a) and (b)) are fixed for such cases.

Summary of Reftests:
====================
 - All 4 reftests fail pre-patching.
 - Patches v2-3 fix reftests 1 and 2, but they don't fix reftests 3 and 4, because those early patches didn't include the "aBand->mRight" section.  (to check for 0-width clipping regions aligned with the *right* edge of a BandRect)
 - Patch v4 fixes all 4 reftests.


[1] Clipping region = the horizontal region from mX to mX + aMaxSize.width
[2] Available space trapezoid = a trapezoid with mFrames set to null.

Comment 11

[Daniel Holbert [:dholbert]]

> Now we recognize this situation as overlap, and we return a trapezoid
> indicating that the space is unavailable, as we do in the very-small clipping
> region case.  So both bad things ((a) and (b)) are fixed for such cases.

(By "Now", I mean "once this patch has been applied".)

Comment 12

[Daniel Holbert [:dholbert]]

Slightly more detailed / graphical explanation:

As I understand it, nsSpaceManager::GetBandAvailableSpace basically tries to superimpose the clip region [mX, rightEdge == mX + aMaxSize.width] on top of a BandRect of occupied space [aBand->mLeft, aBand->mRight].  It aims to chop up the clip interval into as many as three output intervals ('trapezoids' in the code), like so:
 
Clip:     [----------------------------------------]
BandRect:              [----------------]
Output:   [available--][unavailable-----][available]


Now, consider these two cases, and the output they produce in the pre-patched code.

REFERENCE CASE:
  Clip:                  [-]
  BandRect:              [----------------]
  --> Output:            [unavailable-----]

TEST CASE:
  Clip:                  X   (0 width)
  BandRect:              [----------------]
  --> Output:            X   (**available** trapezoid with 0 width)

Our behavior in the test case is wrong.  The clipping region does not intersect any available space, and yet we return a 0-width **available** space rectangle.


After this patch, we'll do this instead
TEST CASE:
  Clip:                  X   (0 width)
  BandRect:              [----------------]
  --> Output:            [unavailable-----]

(now matching the behavior of the reference case)

Comment 13

[Daniel Holbert [:dholbert]]

patch v4 checked in.

Checking in generic/nsSpaceManager.cpp;
  new revision: 3.90; previous revision: 3.89
Checking in reftests/bugs/403129-1-ref.html;
  initial revision: 1.1
Checking in reftests/bugs/403129-1.html;
  initial revision: 1.1
Checking in reftests/bugs/403129-2-ref.html;
  initial revision: 1.1
Checking in reftests/bugs/403129-2.html;
  initial revision: 1.1
Checking in reftests/bugs/403129-3-ref.html;
  initial revision: 1.1
Checking in reftests/bugs/403129-3.html;
  initial revision: 1.1
Checking in reftests/bugs/403129-4-ref.html;
  initial revision: 1.1
Checking in reftests/bugs/403129-4.html;
  initial revision: 1.1
Checking in reftests/bugs/reftest.list;
  new revision: 1.210; previous revision: 1.209

Comment 14

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020819 Minefield/3.0b4pre (Leak Debug Build)

no leak on testcase -> Verified fixed