Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

This patch fixes TextAutosizer not to scheduleRelayout() when its
relayoutBehavior is AlreadyInLayout.

r378639 solved the same issue only when markContainerChainForLayout()
has SubtreeLayoutScope, but TextAutosizer does not always have
SubtreeLayoutScope.

To solve this case, MarkContainerChainInLayout is added to mark
container chain without scheduleRelayout(), even when
SubtreeLayoutScope is missing.

BUG=602908
Committed: https://crrev.com/31c3833651246f431a837c65e8f61b20aa1e00b1
Cr-Commit-Position: refs/heads/master@{#388410}

[kojii, 2016-04-18 04:54:18]

Description was changed from

==========
needs

BUG=
==========

to

==========
Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

This patch fixes TextAutosizer not to scheduleRelayout() when it is in
AlreadyInLayout.

r378639 solved the same issue only when markContainerChainForLayout()
has SubtreeLayoutScope, but TextAutosizer does not always have
SubtreeLayoutScope.

To solve this case, MarkContainerChainInLayout is added to mark
container chain without scheduleRelayout().

BUG=602908
==========

[kojii, 2016-04-18 04:56:20]

Description was changed from

==========
Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

This patch fixes TextAutosizer not to scheduleRelayout() when it is in
AlreadyInLayout.

r378639 solved the same issue only when markContainerChainForLayout()
has SubtreeLayoutScope, but TextAutosizer does not always have
SubtreeLayoutScope.

To solve this case, MarkContainerChainInLayout is added to mark
container chain without scheduleRelayout().

BUG=602908
==========

to

==========
Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

This patch fixes TextAutosizer not to scheduleRelayout() when it is in
AlreadyInLayout.

r378639 solved the same issue only when markContainerChainForLayout()
has SubtreeLayoutScope, but TextAutosizer does not always have
SubtreeLayoutScope.

To solve this case, MarkContainerChainInLayout is added to mark
container chain without scheduleRelayout(), even when
SubtreeLayoutScope is missing.

BUG=602908
==========

[kojii, 2016-04-18 05:04:58]

Description was changed from

==========
Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

This patch fixes TextAutosizer not to scheduleRelayout() when it is in
AlreadyInLayout.

r378639 solved the same issue only when markContainerChainForLayout()
has SubtreeLayoutScope, but TextAutosizer does not always have
SubtreeLayoutScope.

To solve this case, MarkContainerChainInLayout is added to mark
container chain without scheduleRelayout(), even when
SubtreeLayoutScope is missing.

BUG=602908
==========

to

==========
Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

This patch fixes TextAutosizer not to scheduleRelayout() when its
relayoutBehavior is AlreadyInLayout.

r378639 solved the same issue only when markContainerChainForLayout()
has SubtreeLayoutScope, but TextAutosizer does not always have
SubtreeLayoutScope.

To solve this case, MarkContainerChainInLayout is added to mark
container chain without scheduleRelayout(), even when
SubtreeLayoutScope is missing.

BUG=602908
==========

[kojii, 2016-04-18 06:32:27]

kojii@chromium.org changed reviewers:
+ cbiesinger@chromium.org, leviw@chromium.org

[kojii, 2016-04-18 06:32:28]

PTAL, the one we discussed in https://codereview.chromium.org/1887933002.

I left one comment in LayoutObject.cpp where I appreciate your comments on.

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:758:
ASSERT(!scheduleRelayout || !layouter);
Should we make this RELEASE_ASSERT()?

[cbiesinger, 2016-04-18 15:53:46]

lgtm, see my suggestion below

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:758:
ASSERT(!scheduleRelayout || !layouter);
On 2016/04/18 06:32:28, kojii wrote:
> Should we make this RELEASE_ASSERT()?

I don't really see value in that. The more interesting case, IMO, would be the
one where we have scheduleRelayout=true but should be false, when we did not
pass a layouter when we should have.

Actually, maybe we could do that -- RELEASE_ASSERT(!scheduleRelayout ||
!frameView->isInPerformLayout())?

Based on bug 600998, it seems that we'd usually crash in such a case anyway, and
making this one a release assert would help in tracking this down, so I would
support adding this.

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.h (left):

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:814: void
markContainerChainForLayout(SubtreeLayoutScope*);
Thanks, the three versions of this function have been bothering me :-)

[kojii, 2016-04-19 06:39:08]

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:758:
ASSERT(!scheduleRelayout || !layouter);
On 2016/04/18 at 15:53:46, cbiesinger wrote:
> On 2016/04/18 06:32:28, kojii wrote:
> > Should we make this RELEASE_ASSERT()?
> 
> I don't really see value in that. The more interesting case, IMO, would be the
one where we have scheduleRelayout=true but should be false, when we did not
pass a layouter when we should have.
> 
> Actually, maybe we could do that -- RELEASE_ASSERT(!scheduleRelayout ||
!frameView->isInPerformLayout())?
> 
> Based on bug 600998, it seems that we'd usually crash in such a case anyway,
and making this one a release assert would help in tracking this down, so I
would support adding this.

It doesn't always crash, bug 600998 was unable to minimize because it crashes
only 2 out of 10 tries, so having proper RELEASE_ASSERT() is good.

And your suggestion looks far better, thank you.

However, by doing that, over 100 tests fail. By looking at some failures
randomly, we can probably narrow down to 4-5 code paths to fix.

So I reverted the RELEASE_ASSERT() for now. I can use it locally to find what to
fix, and we can make the RELEASE_ASSERT when all of them are fixed.

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutObject.h (left):

https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutObject.h:814: void
markContainerChainForLayout(SubtreeLayoutScope*);
On 2016/04/18 at 15:53:46, cbiesinger wrote:
> Thanks, the three versions of this function have been bothering me :-)

Great to hear that, thought it's good before, but looking now, I found I didn't
like it too ;-P

[cbiesinger, 2016-04-19 20:37:52]

On 2016/04/19 06:39:08, kojii wrote:
>
https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):
> 
>
https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutObject.cpp:758:
> ASSERT(!scheduleRelayout || !layouter);
> On 2016/04/18 at 15:53:46, cbiesinger wrote:
> > On 2016/04/18 06:32:28, kojii wrote:
> > > Should we make this RELEASE_ASSERT()?
> > 
> > I don't really see value in that. The more interesting case, IMO, would be
the
> one where we have scheduleRelayout=true but should be false, when we did not
> pass a layouter when we should have.
> > 
> > Actually, maybe we could do that -- RELEASE_ASSERT(!scheduleRelayout ||
> !frameView->isInPerformLayout())?
> > 
> > Based on bug 600998, it seems that we'd usually crash in such a case anyway,
> and making this one a release assert would help in tracking this down, so I
> would support adding this.
> 
> It doesn't always crash, bug 600998 was unable to minimize because it crashes
> only 2 out of 10 tries, so having proper RELEASE_ASSERT() is good.
> 
> And your suggestion looks far better, thank you.
> 
> However, by doing that, over 100 tests fail. By looking at some failures
> randomly, we can probably narrow down to 4-5 code paths to fix.
> 
> So I reverted the RELEASE_ASSERT() for now. I can use it locally to find what
to
> fix, and we can make the RELEASE_ASSERT when all of them are fixed.
> 
>
https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutObject.h (left):
> 
>
https://codereview.chromium.org/1896803002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutObject.h:814: void
> markContainerChainForLayout(SubtreeLayoutScope*);
> On 2016/04/18 at 15:53:46, cbiesinger wrote:
> > Thanks, the three versions of this function have been bothering me :-)
> 
> Great to hear that, thought it's good before, but looking now, I found I
didn't
> like it too ;-P

lgtm. sad to hear that we trigger the release_assert path :( but it's good that
we're finding this. thanks for fixing!

[kojii, 2016-04-20 02:45:01]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-04-20 02:45:11]

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

[commit-bot: I haz the power, 2016-04-20 02:49:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-22 19:20:15]

Description was changed from

==========
Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

This patch fixes TextAutosizer not to scheduleRelayout() when its
relayoutBehavior is AlreadyInLayout.

r378639 solved the same issue only when markContainerChainForLayout()
has SubtreeLayoutScope, but TextAutosizer does not always have
SubtreeLayoutScope.

To solve this case, MarkContainerChainInLayout is added to mark
container chain without scheduleRelayout(), even when
SubtreeLayoutScope is missing.

BUG=602908
==========

to

==========
Fix TextAutosizer not to scheduleRelayout() by MarkContainerChainInLayout

This patch fixes TextAutosizer not to scheduleRelayout() when its
relayoutBehavior is AlreadyInLayout.

r378639 solved the same issue only when markContainerChainForLayout()
has SubtreeLayoutScope, but TextAutosizer does not always have
SubtreeLayoutScope.

To solve this case, MarkContainerChainInLayout is added to mark
container chain without scheduleRelayout(), even when
SubtreeLayoutScope is missing.

BUG=602908

Committed: https://crrev.com/31c3833651246f431a837c65e8f61b20aa1e00b1
Cr-Commit-Position: refs/heads/master@{#388410}
==========

[commit-bot: I haz the power, 2016-04-22 19:20:16]

Patchset 3 (id:??) landed as
https://crrev.com/31c3833651246f431a837c65e8f61b20aa1e00b1
Cr-Commit-Position: refs/heads/master@{#388410}