Allow setNeedsLayout on positioned objects when no parent container

Allow setNeedsLayout on positioned objects when no parent container

We assert !needsLayout in a LayoutCounter contained within a
positioned block flow element. The condition arises due to code
in LayoutObject::markContainerChainForLayout which refuses
to mark objects with no container for layout, under the
commented assumption that it means the object is not attached to
the tree.

Failure to layout the counter results in stale text layout
inside the counter, which is very bad. Because the path from
the counter to the root is never marked as needing layout, the
counter will not ever get laid out.

This patch adds a condition for positioned objects so that they
do get marked for layout.

Test added to clusterfuzz.

R=ojan@chromium.org,esphren@chromium.org
BUG=492490

[Stephen Chennney, 2015-06-01 15:43:48]

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

[commit-bot: I haz the power, 2015-06-01 15:44:08]

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

[Stephen Chennney, 2015-06-01 15:44:28]

schenney@chromium.org changed reviewers:
+ esprehn@chromium.org
- esphren@chromium.org

[Stephen Chennney, 2015-06-01 15:46:44]

This fix seems hackish, but I don't see immediately how else to address the
problem. My concern is that it seems to result in the LayoutBlockFlow parent of
the counter always marked as needing layout, but maybe that's because we don't
clear the layout bit on it.

It may also be that the real error is the presence of the LayoutCounter in a
layout tree of this configuration. If that's the case then this is an editing
bug.

[esprehn, 2015-06-01 16:28:51]

You should definitely always have a container() if you're in the tree. How
does that end up null? That seems like a bug.

I don't think we want another special case for positioned objects.

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[mstensho (USE GERRIT), 2015-06-01 16:34:23]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2015-06-01 16:34:24]

Would be nice to have a simple layout test, so that we could figure out the root
cause here.

[Stephen Chennney, 2015-06-01 16:49:19]

On 2015/06/01 16:34:24, mstensho wrote:
> Would be nice to have a simple layout test, so that we could figure out the
root
> cause here.

There's a repro case, the smallest I could make easily, in this bug comment.
I've added you to the CC so you should be able to see it.
https://code.google.com/p/chromium/issues/detail?id=492490#c6

[Stephen Chennney, 2015-06-01 16:51:07]

On 2015/06/01 16:28:51, esprehn wrote:
> You should definitely always have a container() if you're in the tree. How
> does that end up null? That seems like a bug.
> 
> I don't think we want another special case for positioned objects.

I agree in principle that we don't want another special case. The goal was to
get some feedback.

It may be that the bug really is in the edit code then, in the way we insert the
line break in this particular situation.

I'm out of my depth here, so someone with layout experience should take up the
bug.

[commit-bot: I haz the power, 2015-06-01 18:59:02]

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

[commit-bot: I haz the power, 2015-06-01 18:59:03]

Dry run: 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/64602)

[ojan, 2015-06-01 23:23:14]

+dsinclair,eae, FYI sounds like this needs a layout team owner. dsinclair, the
bug has been assigned to you, I don't know if that means you want to own it or
not.

[mstensho (USE GERRIT), 2015-06-04 18:08:11]

https://codereview.chromium.org/1143323011/ fixes this issue, so this one can be
closed now.