550325 - "ASSERTION: We should have padding here" with abs pos, overflow:scroll

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: We should have padding here! (out of memory?): 'hasPadding', file /Users/jruderman/central/layout/generic/nsFrame.cpp, line 695

This assertion (in its current form) was added by bug 542361.

Comment 1

[Mats Palmgren (inactive)]

I'll take a look...

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Adding an "X" to show that the left position is wrong.

Comment 3

[Mats Palmgren (inactive)]

The problem is that the UsedPaddingProperty is missing, that causes the
assertion and the layout error.  nsGfxScrollFrame explicitly sets it:
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsGfxScrollFrame.cpp#l495
but this is immediately removed by nsHTMLReflowState::InitConstraints
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsHTMLReflowState.cpp#l1656
which then never sets it up again, since in nsCSSOffsetState::InitOffsets
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsHTMLReflowState.cpp#l1893
we take the first 'else' which just assigns 'mComputedPadding' --
it's ComputePadding() in the last 'else' that sets it up (when needed):
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsHTMLReflowState.cpp#l2190

Comment 4

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Let's handle these properties in InitOffsets() instead of in multiple places.
(It's called from InitConstraints() unconditionally, and only from there.)
This also allows us to optimize the Delete+Set in case the property
exists (avoiding delete+new and extra property search).

The bug fix is that when the aPadding argument is non-null we set the
property if the padding style is width dependent.

I'm also slightly changing ComputePadding() for some table elements;
it used to compute a %-padding and set the property to that value and
then set mComputedPadding to zero.  This seems like a bug, I think the
property value should always be the same as the computed value.

Passed TryServer unit tests (some known orange).

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

> property value should always be the same as the computed value

We should also fix that for border-collapse tables and scrollbars
which set mComputedPadding to zero at the end of InitOffsets:
http://hg.mozilla.org/mozilla-central/annotate/9b79a3d0c554/layout/generic/nsHTMLReflowState.cpp#l1934

Updating UsedPaddingProperty at the end InitOffsets should fix that.

Comment 6

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

Comment on attachment 488871 [details] [diff] [review]
Patch rev. 2

Split needProp into needMarginProp and needPaddingProp.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Nit fixed.  Also, I moved local variable 'presContext' a bit earlier
to eliminate a 'frame->PresContext()'.  Trivial change, so not going
to ask for a new review.

Comment 8

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/7f16577a2671

Comment 9

[Jesse Ruderman]

Bug 611532 has another testcase that triggers the assertion.