Check for buffer under/over run in table cell painting

Check for buffer under/over run in table cell painting

R=chrishtr@chromium.org,jchaffraix@chromium.org
BUG=475698
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196716

[Stephen Chennney, 2015-06-01 21:13:20]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-01 21:13:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162243004/1

[Stephen Chennney, 2015-06-01 21:14:32]

Attempted fix (hat tip chrishtr) for BlockPainter crashes. We would indeed fail
on 1x1 tables if margins are of a certain size and the repaint rect is of a
certain size. Hard to see how to write a test.

[chrishtr, 2015-06-01 22:03:53]

I think you could repro by positioning a 1x1 table such that the border crosses
a compositor tile boundary, then invalidate something else in the tile that
contains the border. (can be a different div as a test) Maybe tables have
incremental invalidation also, not sure.

[commit-bot: I haz the power, 2015-06-02 00:23:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-02 00:23:21]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[Stephen Chennney, 2015-06-03 13:37:44]

On 2015/06/01 22:03:53, chrishtr wrote:
> I think you could repro by positioning a 1x1 table such that the border
crosses
> a compositor tile boundary, then invalidate something else in the tile that
> contains the border. (can be a different div as a test) Maybe tables have
> incremental invalidation also, not sure.

A unit test might be the way to go here. Any test relying on tile boundaries is
going to be unstable as the underlying architecture changes. I'll poke around.

[Stephen Chennney, 2015-06-05 19:29:27]

In trying to write the test I have come to realize that the code is correct. So
there's no need to land this.

Back to the drawing board for identifying the cause of the crash.

[Stephen Chennney, 2015-06-05 19:42:14]

On 2015/06/05 19:29:27, Stephen Chenney wrote:
> In trying to write the test I have come to realize that the code is correct.
So
> there's no need to land this.
> 
> Back to the drawing board for identifying the cause of the crash.

Scratch that last comment. It was the result of a bad revert to a local change.

[Stephen Chennney, 2015-06-05 20:24:09]

The CQ bit was checked by schenney@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 20:24:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162243004/20001

[Stephen Chennney, 2015-06-05 20:25:27]

The code was indeed correct, although somewhat non-obvious. Adding asserts to
see if that is true in the wild. We might be somehow using this method when
layout is invalid or the row and column position vectors are otherwise invalid.

[commit-bot: I haz the power, 2015-06-05 21:57:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 21:57:45]

Dry run: This issue passed the CQ dry run.

[Julien - ping for review, 2015-06-08 15:23:48]

me gustó! lgtm

https://codereview.chromium.org/1162243004/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/1162243004/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutTableSection.cpp:1339:
RELEASE_ASSERT(coveredColumns.start() <= coveredColumns.end());
This code is repeated twice so I think it ought to be shared.

I would put it into CellSpace::ensureConsistency() as we could add it to
increaseStart / increaseEnd later if we want to.

[Stephen Chennney, 2015-06-08 16:19:57]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2015-06-08 16:19:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
jchaffraix@chromium.org
Link to the patchset: https://codereview.chromium.org/1162243004/#ps40001
(title: "Extract common code")

[commit-bot: I haz the power, 2015-06-08 16:20:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162243004/40001

[commit-bot: I haz the power, 2015-06-08 18:45:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-08 18:45:04]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/65691)

[Stephen Chennney, 2015-06-08 20:51:36]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2015-06-08 20:52:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1162243004/40001

[commit-bot: I haz the power, 2015-06-09 00:04:28]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196716