Convert a RELEASE_ASSERT into an ASSERT

Fix the check for isDocumentElement in BlockPainter.

A previous patch (https://codereview.chromium.org/1205143003/) incorrectly inverted the sense of the isDocumentElement check. This patch fixes it.

Various RELEASE_ASSERT's were also left behind, but they show no impact on crash reporting, so change them into an ASSERT. There's no security implication to this code, so no reason to assert in release builds. This will cut the number of crashes a little.

R=chrishtr
BUG=475698, 504955, 509737
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198861

[Stephen Chennney, 2015-06-30 20:44:00]

This patch is part of the ongoing effort to track down the cause of the
BlockPainter crashes.

[chrishtr, 2015-06-30 21:20:40]

chrishtr@chromium.org changed reviewers:
+ trchen@chromium.org

[chrishtr, 2015-07-01 19:59:25]

Tien-Ren could could you comment?

[Stephen Chennney, 2015-07-13 17:33:22]

This is now the fix for
https://code.google.com/p/chromium/issues/detail?id=504955

... and removing unnecessary asserts.

[Stephen Chennney, 2015-07-13 17:36:32]

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

[commit-bot: I haz the power, 2015-07-13 17:36:47]

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

[chrishtr, 2015-07-13 17:37:58]

Please fix the CL description to emphasize the bugfix at the top level.

Also, this region of the code is not tested. Please add a TODO and file a bug,
so we can add testing soon.

[Stephen Chennney, 2015-07-13 17:54:05]

Done. Better documentation all round.

[chrishtr, 2015-07-13 18:00:04]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-07-13 18:00:05]

lgtm

[commit-bot: I haz the power, 2015-07-13 18:00:35]

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

[commit-bot: I haz the power, 2015-07-13 19:47:30]

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

[commit-bot: I haz the power, 2015-07-13 19:47:31]

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

[trchen, 2015-07-13 20:23:18]

https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
File Source/core/paint/BlockPainter.cpp (right):

https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:39: // that the LayoutView paints the root's
background.
Yep now LayoutView paints root background.

https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
Source/core/paint/BlockPainter.cpp:45: if (!m_layoutBlock.isDocumentElement()) {
It is probably okay to remove this check here. Does it break any tests if we
remove the check?

[Stephen Chennney, 2015-07-13 20:40:24]

On 2015/07/13 20:23:18, trchen wrote:
>
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> File Source/core/paint/BlockPainter.cpp (right):
> 
>
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> Source/core/paint/BlockPainter.cpp:39: // that the LayoutView paints the
root's
> background.
> Yep now LayoutView paints root background.
> 
>
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> Source/core/paint/BlockPainter.cpp:45: if (!m_layoutBlock.isDocumentElement())
{
> It is probably okay to remove this check here. Does it break any tests if we
> remove the check?

Just so I understand, you mean we should always do the check inside the if and
return early if we can?

Apparently we have exactly one test that failed, one mac only, due to having the
if the wrong way around.

[trchen, 2015-07-13 20:59:40]

On 2015/07/13 20:40:24, Stephen Chenney wrote:
> On 2015/07/13 20:23:18, trchen wrote:
> >
>
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> > File Source/core/paint/BlockPainter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> > Source/core/paint/BlockPainter.cpp:39: // that the LayoutView paints the
> root's
> > background.
> > Yep now LayoutView paints root background.
> > 
> >
>
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> > Source/core/paint/BlockPainter.cpp:45: if
(!m_layoutBlock.isDocumentElement())
> {
> > It is probably okay to remove this check here. Does it break any tests if we
> > remove the check?
> 
> Just so I understand, you mean we should always do the check inside the if and
> return early if we can?

Exactly.

> Apparently we have exactly one test that failed, one mac only, due to having
the
> if the wrong way around.

Hmm... Those blue dots are in fact very small texts. I wonder if it is the text
engine on Mac incorrectly rounds down the bounding box of the glyphs (thus being
empty). So there should be blue dots in the correct result, but are skipped if
we do the overflow early exit correctly.

[chrishtr, 2015-07-13 21:06:42]

On 2015/07/13 at 20:59:40, trchen wrote:
> On 2015/07/13 20:40:24, Stephen Chenney wrote:
> > On 2015/07/13 20:23:18, trchen wrote:
> > >
> >
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> > > File Source/core/paint/BlockPainter.cpp (right):
> > > 
> > >
> >
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> > > Source/core/paint/BlockPainter.cpp:39: // that the LayoutView paints the
> > root's
> > > background.
> > > Yep now LayoutView paints root background.
> > > 
> > >
> >
https://codereview.chromium.org/1219093002/diff/40001/Source/core/paint/Block...
> > > Source/core/paint/BlockPainter.cpp:45: if
(!m_layoutBlock.isDocumentElement())
> > {
> > > It is probably okay to remove this check here. Does it break any tests if
we
> > > remove the check?
> > 
> > Just so I understand, you mean we should always do the check inside the if
and
> > return early if we can?
> 
> Exactly.
> 
> > Apparently we have exactly one test that failed, one mac only, due to having
the
> > if the wrong way around.
> 
> Hmm... Those blue dots are in fact very small texts. I wonder if it is the
text engine on Mac incorrectly rounds down the bounding box of the glyphs (thus
being empty). So there should be blue dots in the correct result, but are
skipped if we do the overflow early exit correctly.

Please remove the check in a followup, to make the first patch safer to merge.

[Stephen Chennney, 2015-07-14 12:16:17]

The CQ bit was checked by schenney@chromium.org

[Stephen Chennney, 2015-07-14 12:16:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1219093002/#ps60001
(title: "Expectations added.")

[commit-bot: I haz the power, 2015-07-14 12:16:21]

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

[commit-bot: I haz the power, 2015-07-14 13:38:16]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198861

[chrishtr, 2015-07-16 18:43:36]

lgtm