812879 - Various things go wrong with page-break-inside: avoid (along with table, huge margins, float, overflow)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

This testcase triggers:

ASSERTION: invalid col index: 'Error', file layout/tables/nsTableFrame.cpp
ASSERTION: column frames out of sync with cell map: 'Error', file layout/tables/BasicTableLayoutStrategy.cpp

A related testcase triggers:

ASSERTION: overflow list w/o frames: 'mFrames.NotEmpty()', file layout/generic/nsContainerFrame.cpp
Assertion failure: i < Length() (invalid array index),

Comment 1

[Mats Palmgren (inactive)]

Removing 'page-break-inside: avoid' from the test makes the assertions go away,
so it must be a regression from bug 685012.

Comment 2

[Mats Palmgren (inactive)]

Jesse, could you attach the related testcase too please?

Comment 3

[Andrew McCreight [:mccr8]]

Marking sec-high due to the array bounds assertion. Please adjust as appropriate.

Comment 4

[Jesse Ruderman]

Attachment: testcase B

Comment 5

[Jesse Ruderman]

"testcase B" also crashes nightly: bp-fa867553-07e7-431c-ae31-933292121121

Comment 6

[Mats Palmgren (inactive)]

The testcases reveals several bugs in the way nsTableFrame deals with row-group
continuations.  I've filed bug 815315 to address the underlying issues.  In this
bug I'll just remove the mistake that bug 685012 did to cause these edge cases
to occur.  The fault is in the block added for pushing floats for
'page-break-inside:avoid' here:
https://bugzilla.mozilla.org/attachment.cgi?id=677034&action=diff#a/layout/generic/nsBlockReflowState.cpp_sec1

Comment 7

[Mats Palmgren (inactive)]

Attachment: crashtests (DONT LAND UNTIL BUG IS PUBLIC)

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix

Don't try to push a float when the height is unconstrained.

Comment 9

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=a6bd1c72a530

Comment 10

[fantasai]

Comment on attachment 685904 [details] [diff] [review]
fix

We should never be incomplete if the the availableHeight is unconstrained, so maybe add an assertion that
  mContentArea.height == NS_UNCONSTRAINEDSIZE
and
  !NS_FRAME_IS_FULLY_COMPLETE(reflowStatus)
are not both true?

Comment 11

[Mats Palmgren (inactive)]

True, but we already do that in nsBlockFrame.cpp:1413 and it does assert
for both testcases here.  (Not sure why Jesse didn't mention it - perhaps
because we have other assertions involving UNCONSTRAINED sizes that safely
can be ignored.)

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 685904 [details] [diff] [review]
fix

[Security approval request comment]
How easily can the security issue be deduced from the patch?
not at all

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
no

Which older supported branches are affected by this flaw?
Aurora

If not all supported branches, which bug introduced the flaw?
bug 685012

(FYI, the last two questions are ALWAYS redundant since other
fields in the bug already has that data)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
same patch applies

How likely is this patch to cause regressions; how much testing does it need?
unlikely, the change makes this code block execute less often, so the
result is more like before the block was added by bug 685012

Comment 13

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b169dfc8c38

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9b169dfc8c38

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 685904 [details] [diff] [review]
fix

See comment 12, low risk

Comment 16

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/19c3aa22d0ea

Comment 17

[Mats Palmgren (inactive)]

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6047dff1521c

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6047dff1521c