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.

> 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.

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.

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.

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.

(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 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.

> 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".)

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)

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

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