ASSERTION: bad argument: '(aRight > mLeft) && (aRight < mRight)', @ nsSpaceManager::BandRect::SplitHorizontally

VERIFIED FIXED in mozilla1.9beta2

Status

()

Core
Layout: Floats
P5
normal
VERIFIED FIXED
13 years ago
10 years ago

People

(Reporter: bc, Assigned: roc)

Tracking

({assertion, memory-leak, testcase})

Trunk
mozilla1.9beta2
assertion, memory-leak, testcase
Points:
---
Bug Flags:
blocking1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:Rm], URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

13 years ago
Found in a Firefox 1.5 build from 2005-09-05 while scanning abc.com

###!!! ASSERTION: bad argument: '(aRight > mLeft) && (aRight < mRight)', file
c:/work/mozilla/builds/ff/1.5/mozilla/layout/generic/nsSpaceManager.cpp, line 1350

nsSpaceManager::BandRect::SplitHorizontally(int 5955) line 1350
nsSpaceManager::AddRectToBand(nsSpaceManager::BandRect * 0x03490b50,
nsSpaceManager::BandRect * 0x03491228) line 651 + 15 bytes
nsSpaceManager::InsertBandRect(nsSpaceManager::BandRect * 0x03491228) line 779
nsSpaceManager::AddRectRegion(nsIFrame * 0x03488fe0, const nsRect & {...}) line 842
nsBlockReflowState::FlowAndPlaceFloat(nsFloatCache * 0x034910d0, int *
0x0012774c, unsigned int & 0, int 0) line 989 + 22 bytes
nsBlockReflowState::AddFloat(nsLineLayout & {...}, nsPlaceholderFrame *
0x03489034, int 0, unsigned int & 0) line 626 + 24 bytes
nsLineLayout::AddFloat(nsPlaceholderFrame * 0x03489034, unsigned int & 0) line 261
nsLineLayout::ReflowFrame(nsIFrame * 0x03489034, unsigned int & 0,
nsHTMLReflowMetrics * 0x00000000, int & 0) line 1016 + 22 bytes
nsBlockFrame::ReflowInlineFrame(nsBlockReflowState & {...}, nsLineLayout &
{...}, nsLineList_iterator {...}, nsIFrame * 0x03489034, unsigned char *
0x00127a0b) line 4000 + 22 bytes
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState & {...}, nsLineLayout &
{...}, nsLineList_iterator {...}, int * 0x00127db4, unsigned char * 0x00127aff,
int 1, int 0) line 3839 + 32 bytes
nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x00127db4, int 0, int 1) line 3712 + 46 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x00127db4, int 0) line 2681
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}, int 1) line 2240 + 31
bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x03486994, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 877 + 17 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 0,
nsCollapsingMargin & {...}, int 0, int 1, nsMargin & {...}, nsHTMLReflowState &
{...}, unsigned int & 0) line 588 + 42 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x00128a00) line 3427 + 66 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x00128a00, int 0) line 2588 + 27 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}, int 1) line 2240 + 31
bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x03486834, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 877 + 17 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 1,
nsCollapsingMargin & {...}, int 0, int 1, nsMargin & {...}, nsHTMLReflowState &
{...}, unsigned int & 0) line 588 + 42 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x0012964c) line 3427 + 66 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x0012964c, int 0) line 2588 + 27 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}, int 1) line 2240 + 31
bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x0347c96c, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 877 + 17 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 1,
nsCollapsingMargin & {...}, int 0, int 0, nsMargin & {...}, nsHTMLReflowState &
{...}, unsigned int & 0) line 588 + 42 bytes
nsBlockFrame::ReflowFloat(nsBlockReflowState & {...}, nsPlaceholderFrame *
0x0347c9c0, nsFloatCache * 0x0348da90, unsigned int & 0) line 5899 + 54 bytes
nsBlockReflowState::FlowAndPlaceFloat(nsFloatCache * 0x0348da90, int *
0x0012a110, unsigned int & 0, int 0) line 841
nsBlockReflowState::AddFloat(nsLineLayout & {...}, nsPlaceholderFrame *
0x0347c9c0, int 0, unsigned int & 0) line 626 + 24 bytes
nsLineLayout::AddFloat(nsPlaceholderFrame * 0x0347c9c0, unsigned int & 0) line 261
nsLineLayout::ReflowFrame(nsIFrame * 0x0347c9c0, unsigned int & 0,
nsHTMLReflowMetrics * 0x00000000, int & 0) line 1016 + 22 bytes
nsBlockFrame::ReflowInlineFrame(nsBlockReflowState & {...}, nsLineLayout &
{...}, nsLineList_iterator {...}, nsIFrame * 0x0347c9c0, unsigned char *
0x0012a3cf) line 4000 + 22 bytes
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState & {...}, nsLineLayout &
{...}, nsLineList_iterator {...}, int * 0x0012a778, unsigned char * 0x0012a4c3,
int 1, int 1) line 3839 + 32 bytes
nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x0012a778, int 1, int 1) line 3712 + 46 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x0012a778, int 1) line 2681
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}, int 1) line 2240 + 31
bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x03428264, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 877 + 17 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 1,
nsCollapsingMargin & {...}, int 0, int 1, nsMargin & {...}, nsHTMLReflowState &
{...}, unsigned int & 0) line 605 + 42 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x0012b3c4) line 3427 + 66 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x0012b3c4, int 1) line 2588 + 27 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}, int 1) line 2240 + 31
bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x03428074, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 877 + 17 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x03428074, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 15, int 0,
unsigned int 0, unsigned int & 0) line 904 + 31 bytes
nsTableCellFrame::Reflow(nsTableCellFrame * const 0x03428000, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 858
nsContainerFrame::ReflowChild(nsIFrame * 0x03428000, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 105, int 0,
unsigned int 0, unsigned int & 0) line 904 + 31 bytes
nsTableRowFrame::IR_TargetIsChild(nsTableRowFrame * const 0x0346e268,
nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState
& {...}, nsTableFrame & {...}, unsigned int & 0, nsIFrame * 0x03428000) line
1226 + 45 bytes
nsTableRowFrame::IncrementalReflow(nsTableRowFrame * const 0x0346e268,
nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState
& {...}, nsTableFrame & {...}, unsigned int & 0) line 1111 + 46 bytes
nsTableRowFrame::Reflow(nsTableRowFrame * const 0x0346e268, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 1409 + 35 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x0346e268, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0,
unsigned int 0, unsigned int & 0) line 904 + 31 bytes
nsTableRowGroupFrame::IR_TargetIsChild(nsTableRowGroupFrame * const 0x033f7754,
nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, nsRowGroupReflowState &
{...}, unsigned int & 0, nsIFrame * 0x0346e268) line 1640 + 45 bytes
nsTableRowGroupFrame::IncrementalReflow(nsTableRowGroupFrame * const 0x033f7754,
nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, nsRowGroupReflowState &
{...}, unsigned int & 0) line 1324 + 42 bytes
nsTableRowGroupFrame::Reflow(nsTableRowGroupFrame * const 0x033f7754,
nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState
& {...}, unsigned int & 0) line 1230 + 31 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x033f7754, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0,
unsigned int 0, unsigned int & 0) line 904 + 31 bytes
nsTableFrame::IR_TargetIsChild(nsTableFrame * const 0x0346df34,
nsTableReflowState & {...}, unsigned int & 0, nsIFrame * 0x033f7754) line 2932 +
50 bytes
nsTableFrame::IncrementalReflow(nsTableFrame * const 0x0346df34, const
nsHTMLReflowState & {...}, unsigned int & 0) line 2669 + 34 bytes
nsTableFrame::Reflow(nsTableFrame * const 0x0346df34, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 1925 + 23 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x0346df34, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0,
unsigned int 3, unsigned int & 0) line 904 + 31 bytes
nsTableOuterFrame::OuterReflowChild(nsTableOuterFrame * const 0x0346ddd0,
nsPresContext * 0x0325c090, nsIFrame * 0x0346df34, const nsHTMLReflowState &
{...}, nsHTMLReflowMetrics & {...}, int 11400, nsSize & {...}, nsMargin & {...},
nsMargin & {...}, nsMargin & {...}, nsReflowReason eReflowReason_Incremental,
unsigned int & 0, int * 0x0012c6ac) line 1314 + 47 bytes
nsTableOuterFrame::IR_InnerTableReflow(nsTableOuterFrame * const 0x0346ddd0,
nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState
& {...}, unsigned int & 0) line 1662 + 81 bytes
nsTableOuterFrame::IR_TargetIsInnerTableFrame(nsTableOuterFrame * const
0x0346ddd0, nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, const
nsHTMLReflowState & {...}, unsigned int & 0) line 1426 + 31 bytes
nsTableOuterFrame::IR_TargetIsChild(nsTableOuterFrame * const 0x0346ddd0,
nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState
& {...}, unsigned int & 0, nsIFrame * 0x0346df34) line 1399 + 31 bytes
nsTableOuterFrame::IncrementalReflow(nsTableOuterFrame * const 0x0346ddd0,
nsPresContext * 0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState
& {...}, unsigned int & 0) line 1379 + 42 bytes
nsTableOuterFrame::Reflow(nsTableOuterFrame * const 0x0346ddd0, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 1925 + 31 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 0,
nsCollapsingMargin & {...}, int 0, int 1, nsMargin & {...}, nsHTMLReflowState &
{...}, unsigned int & 0) line 605 + 42 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x0012d240) line 3427 + 66 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x0012d240, int 1) line 2588 + 27 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}, int 1) line 2240 + 31
bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x0346db74, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 877 + 17 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 0,
nsCollapsingMargin & {...}, int 0, int 1, nsMargin & {...}, nsHTMLReflowState &
{...}, unsigned int & 0) line 605 + 42 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x0012de8c) line 3427 + 66 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x0012de8c, int 1) line 2588 + 27 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}, int 1) line 2240 + 31
bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x0346b584, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 877 + 17 bytes
nsBlockReflowContext::ReflowBlock(const nsRect & {...}, int 1,
nsCollapsingMargin & {...}, int 0, int 1, nsMargin & {...}, nsHTMLReflowState &
{...}, unsigned int & 0) line 605 + 42 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineList_iterator
{...}, int * 0x0012ead8) line 3427 + 66 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineList_iterator {...},
int * 0x0012ead8, int 1) line 2588 + 27 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}, int 1) line 2240 + 31
bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x0346b400, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 877 + 17 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x0346b400, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0,
unsigned int 0, unsigned int & 0) line 904 + 31 bytes
CanvasFrame::Reflow(CanvasFrame * const 0x033f717c, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0)
line 522
nsContainerFrame::ReflowChild(nsIFrame * 0x033f717c, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0,
unsigned int 3, unsigned int & 0) line 904 + 31 bytes
nsHTMLScrollFrame::ReflowScrolledFrame(const ScrollReflowState & {...}, int 0,
nsHTMLReflowMetrics * 0x0012f3b0, int 1) line 512 + 54 bytes
nsHTMLScrollFrame::ReflowContents(ScrollReflowState * 0x0012f558, const
nsHTMLReflowMetrics & {...}) line 567 + 25 bytes
nsHTMLScrollFrame::Reflow(nsHTMLScrollFrame * const 0x033f72c8, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 759 + 16 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x033f72c8, nsPresContext * 0x0325c090,
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0, int 0,
unsigned int 0, unsigned int & 0) line 904 + 31 bytes
ViewportFrame::Reflow(ViewportFrame * const 0x033f70e8, nsPresContext *
0x0325c090, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...},
unsigned int & 0) line 239 + 43 bytes
IncrementalReflow::Dispatch(nsPresContext * 0x0325c090, nsHTMLReflowMetrics &
{...}, const nsSize & {...}, nsIRenderingContext & {...}) line 912
PresShell::ProcessReflowCommands(int 1) line 6874
ReflowEvent::HandleEvent() line 6700
HandlePLEvent(ReflowEvent * 0x03476778) line 6717
PL_HandleEvent(PLEvent * 0x03476778) line 688 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00f39460) line 623 + 9 bytes
_md_TimerProc(HWND__ * 0x001504d0, unsigned int 275, unsigned int 0, unsigned
long 89559940) line 1013 + 9 bytes
USER32! 77d48734()
USER32! 77d49857()
USER32! 77d49791()
USER32! 77d48a10()
nsAppShell::Run(nsAppShell * const 0x00f51588) line 135
nsAppStartup::Run(nsAppStartup * const 0x00f514e8) line 145 + 26 bytes
XRE_main(int 3, char * * 0x003f6f60, const nsXREAppData * 0x0042201c kAppData)
line 2322 + 35 bytes
main(int 3, char * * 0x003f6f60) line 61 + 18 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816d4f()
(Reporter)

Comment 1

13 years ago
(In reply to comment #0)
This stack is probably entirely bogus. I will try to get a good one later today.
Summary: ASSERTION: bad argument: '(aRight > mLeft) && (aRight < mRight)', @ nsSpaceManager::BandRect::SplitHorizontally → ASSERTION: bad argument: '(aRight > mLeft) && (aRight < mRight)', @ nsSpaceManager::BandRect::SplitHorizontally
(Reporter)

Comment 2

13 years ago
(In reply to comment #1)

The previous stack is actually good and reproducible in Trunk as well as 1.8
branch. In this case aRight == 0x2472, mLeft == 0x2472, mRight == 0x2b89
Version: 1.8 Branch → Trunk
Minimal testcase would help.  :(
Keywords: qawanted
Is there an actual observable layout bug in the page in question? This assertion was added pretty late in 1.5.
(Reporter)

Comment 5

12 years ago
(In reply to comment #4)

Not that I can see.
Created attachment 205985 [details]
testcase

This testcase still asserts in my debug build.

Updated

12 years ago
Keywords: qawanted → testcase

Updated

12 years ago
Keywords: assertion

Comment 7

12 years ago
I see the assertion using Martijn's testcase.

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9a2?
Should this assertion have an "or equal to"?
Flags: blocking1.9a2? → blocking1.9-
Whiteboard: [wanted-1.9]
No. When equality holds, we're splitting a BandRect in a way that creates an empty BandRect, in which case we shouldn't be splitting at all.

Updated

11 years ago
Component: Layout → Layout: Floats
QA Contact: layout → layout.floats
FWIW, nsSpaceManager::AddRectRegion (nsSpaceManager.cpp#833) has the first indication of a failure.  A breakpoint set for:

bandRect->mLeft == bandRect->mRight

immediately after bandRect is created triggers under the attached testcase.

CNN loves this assertion failure, by the way.

Comment 11

11 years ago
WFM with the URL and with Martijn's testcase.

Comment 12

11 years ago
I can trigger this assertion by visiting http://dailymotion.com, with the same situation as described in Comment #2.

XPCOM_MEM_LEAK_LOG also claims we're leaking BandRect objects on that page, but there are no remaining references.
Fwiw, it is all wfm now, using a 5 day old debug build, testing the url, the url in comment 12 and the testcase.

Comment 14

11 years ago
Created attachment 277773 [details] [diff] [review]
exit early for zero-width rects as well

This patch fixes the assertions and leaks for me, but perhaps we're supposed to insert zero-width rects that have a height.

I'll also update my build to check that I'm not missing a patch.
Attachment #277773 - Flags: review?(roc)

Comment 15

11 years ago
(In reply to comment #14)
> 
> I'll also update my build to check that I'm not missing a patch.

I can still reproduce with an up-to-date build.

Comment 16

11 years ago
See Bug 387201 and the regression caused by exactly this change, bug 391412.

Comment 17

11 years ago
I just looked at this a bit more; this change specifically breaks nsSpaceManager::YMost.  That looks to be fixable, though; I think all that is needed is to change it to do something more like ClearFloats (possibly just calling "ClearFloats(nscoord_MIN, NS_STYLE_CLEAR_BOTH)" would work, although I'm not sure what sort of perf impact that would have).

Comment 18

11 years ago
Comment on attachment 277773 [details] [diff] [review]
exit early for zero-width rects as well

Well, this patch is no good. It crashes on cnn.com.
Attachment #277773 - Flags: review?(roc)

Comment 19

11 years ago
Yeah, you want something like Attachment 273676 [details] [diff], except for the fact that it breaks nsSpaceManager::YMost, and therefore bug 391412.
(Reporter)

Comment 20

11 years ago
The asserts are not reproducible for me in winxp/linux with the url and testcase in this bug, but are still reproducible with the testcase in bug 387214 for both winxp/linux. Since this assertion was #3 in a recent top site test run with over 3000 occurrences, it is definitely not fixed. I suggest duping forward to 387214.

Comment 21

11 years ago
Created attachment 279786 [details] [diff] [review]
Patch

Here's the fix I outlined earlier.  I think it fixes the BandRect leak from http://wiki.mozilla.org/LeakingPages as well.
Assignee: nobody → sharparrow1
Status: NEW → ASSIGNED
Attachment #279786 - Flags: review?(roc)

Updated

11 years ago
Duplicate of this bug: 387214
+      autoHeight = PR_MAX(autoHeight,
+                          aReflowState.mSpaceManager->ClearFloats(nscoord_MIN, NS_STYLE_CLEAR_LEFT_AND_RIGHT));

I think this can result in two calls to ClearFloats.

Comment 24

11 years ago
Yes, you're right... this is why PR_MAX sucks.  I'll post a fixed version.

Comment 25

11 years ago
Created attachment 279827 [details] [diff] [review]
Patch v2
Attachment #279786 - Attachment is obsolete: true
Attachment #279827 - Flags: review?(roc)
Attachment #279786 - Flags: review?(roc)
Comment on attachment 279827 [details] [diff] [review]
Patch v2

Approving because this fixes common assertions and also because it might fix a leak
Attachment #279827 - Flags: superreview+
Attachment #279827 - Flags: review?(roc)
Attachment #279827 - Flags: review+

Updated

11 years ago
Attachment #279827 - Flags: approval1.9?
Attachment #279827 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checked-in for elif since he's awol.

Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.872; previous revision: 3.871
done
Checking in layout/generic/nsSpaceManager.cpp;
/cvsroot/mozilla/layout/generic/nsSpaceManager.cpp,v  <--  nsSpaceManager.cpp
new revision: 3.86; previous revision: 3.85
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
Backed out due to this patch causing a reftest failure for the test in bug 218473.

REFTEST UNEXPECTED FAIL: file:///builds/slave/trunk_osx/mozilla/layout/reftests/bugs/218473-1.html
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9 M9 → ---
Reassigning to roc (as per his request) and re-requesting blocking1.9? due to the leaks this patch might fix.
Assignee: sharparrow1 → roc
Status: REOPENED → NEW
Flags: blocking1.9- → blocking1.9?

Updated

10 years ago
Keywords: mlk
Flags: blocking1.9? → blocking1.9+
Whiteboard: [wanted-1.9] → [wanted-1.9][dbaron-1.9:Rm]
FWIW, I don't see the assertion for the link in the URL field (on Linux).
I do see it loading this URL though:
http://www.alistapart.com/d/multicolumnlayouts/3ColLiquidWithContent.html
The "Patch v2" does NOT fix it.
Comment on attachment 279827 [details] [diff] [review]
Patch v2

Resetting approval flags on bugs that have not been checked in by Oct 22, 11:59PM PDT.  Please re-request approval if needed.
Attachment #279827 - Flags: approval1.9+
Created attachment 286086 [details] [diff] [review]
patch v3

Simple problem ... mSpaceManager->ClearFloats was returning a value relative to the block's top content edge, but autoHeight is relative to the block's top border edge, so the padding wasn't being included in the computed height in the 218473-1.html testcase.

The fix is easy, just go through nsBlockReflowState::ClearFloats, which adds the padding-top in for us. We do need to pass autoHeight instead of nscoord_MIN as the default result, but that's OK, it won't affect anything either way.

I'l see if I can get Eli to review this change.
Attachment #277773 - Attachment is obsolete: true
Attachment #279827 - Attachment is obsolete: true
Attachment #286086 - Flags: superreview+
Attachment #286086 - Flags: review?(sharparrow1)
Whiteboard: [wanted-1.9][dbaron-1.9:Rm] → [needs review][dbaron-1.9:Rm]

Comment 33

10 years ago
I get this assert @ the same function when visiting https://bugs.launchpad.net/ubuntu/+source/mozilla-thunderbird/+bug/128288 in my own debug build from 2007110101.  I don't assert at all in the testcase with that build.  Jesse doesn't assert either on that testcase. (he's also using a recent trunk debug)
OS: Windows XP → All
Hardware: PC → All
Please attach something (not necessarily minimal) to this bug that reproduces that issue.  Relying on an external URI to not go away is a recipe for disaster.

Comment 35

10 years ago
Created attachment 287368 [details]
testcase 2

Reduced from layout/reftests/bugs/387201-3.html.
Priority: -- → P5

Comment 36

10 years ago
Comment on attachment 286086 [details] [diff] [review]
patch v3

This looks right.
Attachment #286086 - Flags: review?(sharparrow1) → review+
Thanks Eli.
Whiteboard: [needs review][dbaron-1.9:Rm] → [needs landing][dbaron-1.9:Rm]
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.884; previous revision: 3.883
done
Checking in layout/generic/nsSpaceManager.cpp;
/cvsroot/mozilla/layout/generic/nsSpaceManager.cpp,v  <--  nsSpaceManager.cpp
new revision: 3.89; previous revision: 3.88
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing][dbaron-1.9:Rm] → [dbaron-1.9:Rm]
Target Milestone: --- → mozilla1.9 M10
Need to make a crashtest assertion test here.
Flags: in-testsuite?
verified fixed using debug build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120520 Minefield/3.0b2pre and the testcases from this bug 

no assertions on testcases -> Verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.