Document lifecycle violation

Document lifecycle violation workaround

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

NOTRY=true

Committed: https://crrev.com/ff891bf37aad64dbb2fcdedd94b7e4868f1f7528
Cr-Commit-Position: refs/heads/master@{#372715}

[davve, 2015-12-23 09:09:25]

The CQ bit was checked by davve@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-23 09:09:35]

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

[davve, 2015-12-23 09:13:13]

davve@opera.com changed reviewers:
+ fs@opera.com

[davve, 2015-12-23 09:13:14]

I've run this locally against svg/ and fast/ and haven't found any fallout. But
let's see what the bots say...

[commit-bot: I haz the power, 2015-12-23 10:21:01]

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

[commit-bot: I haz the power, 2015-12-23 10:21:02]

Dry run: This issue passed the CQ dry run.

[fs, 2015-12-23 10:35:28]

On 2015/12/23 at 09:13:14, davve wrote:
> I've run this locally against svg/ and fast/ and haven't found any fallout.
But let's see what the bots say...

I've always considered DeprecatedScheduleStyleRecalcDuringLayout as something
that should go closer to the actual violation/violator rather than the
"top-most" trigger. Maybe you should ask whoever added this "override" and/or
reviewer.

[davve, 2015-12-23 12:10:32]

davve@opera.com changed reviewers:
+ ojan@chromium.org

[davve, 2015-12-23 12:10:32]

Patch set #2 gives more power to scheduleSVGFilterLayerUpdateHack() than it
actually needs in most cases but hopefully it will go away eventually anyway.

Just to have a reference to this _somewhere_, this is the only entry point I've
found for scheduleSVGFilterLayerUpdateHack to be called during layout:

SVGDocumentExtensions::invalidateSVGRootsWithRelativeLengthDescendents
SVGElement::invalidateRelativeLengthClients
LayoutSVGResourceContainer::invalidateCacheAndMarkForLayout
LayoutSVGResourceFilter::removeAllClientsFromCache
LayoutSVGResourceContainer::markAllClientsForInvalidation
LayoutSVGResourceContainer::markAllClientLayersForInvalidation
PaintLayer::filterNeedsPaintInvalidation
Element::scheduleSVGFilterLayerUpdateHack
Document::scheduleSVGFilterLayerUpdateHack

+ojan, since he reviewed https://codereview.chromium.org/146023008 and might
have some input.

[davve, 2015-12-23 12:15:47]

Description was changed from

==========
Move DeprecatedScheduleStyleRecalcDuringLayout upstream

In an attempt to make it clearer what's going on, move
DeprecatedScheduleStyleRecalcDuringLayout from
PaintLayer::filterNeedsPaintInvalidation() to
LayoutView::layout().

This will catch any other user of
PaintLayer::filterNeedsPaintInvalidation() during layout, outside the
invalidateSVGRootsWithRelativeLengthDescendents() call.

Through comments tie together this lifecycle violation with the extra
layout tree update and layout in
FrameView::updateStyleAndLayoutIfNeededRecursive().

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

to

==========
Document scheduleSVGFilterLayerUpdateHack lifecycle violation

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

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

[davve, 2016-01-04 13:20:01]

Description was changed from

==========
Document scheduleSVGFilterLayerUpdateHack lifecycle violation

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

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

to

==========
Document scheduleSVGFilterLayerUpdateHack lifecycle violation

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.
==========

[ojan, 2016-01-22 02:07:42]

Sorry, I just saw this. The point of putting the deprecation override at the
top-most location is that it's usually the top-most location that needs changing
to fix the invalid codepath. So it serves to document the problem locations so
they can be burned down. If you put it at the bottom location, it makes it
easier to use scheduleSVGFilterLayerUpdateHack and not realize you're doing a
bad thing.

In patch #1, you can clearly see that there is a problem with
invalidateSVGRootsWithRelativeLengthDescendants. That's completely hidden in
patch #2.

I'm a bit confused though. The first version of this patch, in addition to
adding comments, ads a new DeprecatedScheduleStyleRecalcDuringLayout. Is there a
test case that hits an assert due to that being missing? Can you add a test case
with this patch?

Sorry for the delay reviewing this. It's been a busy couple weeks.

[esprehn, 2016-01-22 19:24:43]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-01-22 19:24:44]

https://codereview.chromium.org/1544973002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1544973002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:2106:
DeprecatedScheduleStyleRecalcDuringLayout marker(lifecycle());
Yeah this is supposed to be forced on all the users at the low level to make
their usage of this method look extra ugly and discourage more usage.

[davve, 2016-01-25 09:49:39]

On 2016/01/22 02:07:42, ojan wrote:
> Sorry, I just saw this. The point of putting the deprecation override at the
> top-most location is that it's usually the top-most location that needs
changing
> to fix the invalid codepath. So it serves to document the problem locations so
> they can be burned down. If you put it at the bottom location, it makes it
> easier to use scheduleSVGFilterLayerUpdateHack and not realize you're doing a
> bad thing.
> 
> In patch #1, you can clearly see that there is a problem with
> invalidateSVGRootsWithRelativeLengthDescendants. That's completely hidden in
> patch #2.

Yes, the problem seems to be with
invalidateSVGRootsWithRelativeLengthDescendants() in combination with
PaintLayer::filterNeedsPaintInvalidation() and
scheduleSVGFilterLayerUpdateHack().

> I'm a bit confused though. The first version of this patch, in addition to
> adding comments, ads a new DeprecatedScheduleStyleRecalcDuringLayout. Is there
a
> test case that hits an assert due to that being missing? Can you add a test
case
> with this patch?

It was moved, not added. Some background to this patch: As a
side-track when looking into how to get rid of
FrameView::forceLayoutParentViewIfNeeded I noticed that the 'When an
<iframe> gets composited ...' comment in
FrameView::updateStyleAndLayoutIfNeededRecursive() was out-of-date. I
then tried to figure out when
Document::hasSVGFilterElementsRequiringLayerUpdate() actually was true
in order to replace the comment.

Turns out it's true when scheduleSVGFilterLayerUpdateHack() has been
called during layout. Next question: how/when could this happen?
Chased callers to PaintLayer::filterNeedsPaintInvalidation() and
continued until I found out that the only way I could find this
happening was due to the
invalidateSVGRootsWithRelativeLengthDescendents() call in
LayoutView::layout(). Wanted to verify that so I moved the
DeprecatedScheduleStyleRecalcDuringLayout instance up-stack to
LayoutView::layout() and ask the bots about it. This is patch #1. It
doesn't add any DeprecatedScheduleStyleRecalcDuringLayout, it just
moves it. Then fs had a valid point about where to best put these
DeprecatedScheduleStyleRecalcDuringLayout things. There are at least
three alternatives:

 A. Leave it in PaintLayer::filterNeedsPaintInvalidation().

    Pros: Makes it obvious that this call to
    scheduleSVGFilterLayerUpdateHack() is special and allowed during
    layout.

    Cons: If one wants to find out how/when this is triggered during
    layout, they basically have to redo the investigation I did.

 B. Move it upstack to LayoutView::layout(). (ps#1)

    Pros: Limits the deprecation to a very specific scenario during
    layout.

    Cons: The scenario rather than the actual violator has the
    DeprecatedScheduleStyleRecalcDuringLayout.

 C. Have it at the bottom, in scary sounding
    scheduleSVGFilterLayerUpdateHack(). (ps#2)

    Pros: Makes it obvious why and how
    scheduleSVGFilterLayerUpdateHack() actually is a hack.

    Cons: Gives more power than necessary for
    scheduleSVGFilterLayerUpdateHack(). Depending on when it's called,
    it doesn't need this lifecycle violation.

I understand it as esprehn wanting A. As I understand more of this
code myself (or rather how little I know...), I tend to think A is
fine for now as well. If there's no objections I'll upload a patch for
A and only update out-of-date comments.

[ojan, 2016-01-25 18:57:50]

I agree. Option A is what makes the most sense to me. Hopefully we can get rid
of this hack entirely at some point.

senorblanco: I vaguely recall that some of your work on filters would remove the
need for this. Am I misremembering?

[Stephen White, 2016-01-25 19:09:32]

On 2016/01/25 18:57:50, ojan wrote:
> I agree. Option A is what makes the most sense to me. Hopefully we can get rid
> of this hack entirely at some point.
> 
> senorblanco: I vaguely recall that some of your work on filters would remove
the
> need for this. Am I misremembering?

Sadly, most of my work is downstream (in Skia filter infrastructure and levels
just above it). I'm not sure how to fix this.

[davve, 2016-01-26 10:17:21]

Description was changed from

==========
Document scheduleSVGFilterLayerUpdateHack lifecycle violation

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.
==========

to

==========
Document scheduleSVGFilterLayerUpdateHack lifecycle violation workaround

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

NOTRY=true
==========

[davve, 2016-01-26 10:17:55]

Description was changed from

==========
Document scheduleSVGFilterLayerUpdateHack lifecycle violation workaround

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

NOTRY=true
==========

to

==========
Document lifecycle violation workaround

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

NOTRY=true
==========

[davve, 2016-01-26 10:19:13]

On 2016/01/25 18:57:50, ojan wrote:
> I agree. Option A is what makes the most sense to me. Hopefully we can get rid
> of this hack entirely at some point.

PTAL.

[fs, 2016-01-26 10:25:38]

On 2016/01/25 at 18:57:50, ojan wrote:
> I agree. Option A is what makes the most sense to me. Hopefully we can get rid
of this hack entirely at some point.
> 
> senorblanco: I vaguely recall that some of your work on filters would remove
the need for this. Am I misremembering?

That may have been me - I said something to that effect on a paint team meeting
(which I think you were sitting in on.) Been a bit side-tracked from that lately
though, but hope to get back to it soonish.

[davve, 2016-01-28 13:06:53]

ping

[chrishtr, 2016-02-01 19:06:10]

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

[chrishtr, 2016-02-01 19:06:10]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-02-01 19:06:10]

lgtm

[commit-bot: I haz the power, 2016-02-01 19:08:42]

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

[commit-bot: I haz the power, 2016-02-01 19:15:57]

Description was changed from

==========
Document lifecycle violation workaround

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

NOTRY=true
==========

to

==========
Document lifecycle violation workaround

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

NOTRY=true
==========

[commit-bot: I haz the power, 2016-02-01 19:15:58]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-01 19:16:55]

Description was changed from

==========
Document lifecycle violation workaround

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

NOTRY=true
==========

to

==========
Document lifecycle violation workaround

Add comments describing how scheduleSVGFilterLayerUpdateHack's
lifecycle violation is handled and remove stale comments about
<iframe> compositioning long fixed.

NOTRY=true

Committed: https://crrev.com/ff891bf37aad64dbb2fcdedd94b7e4868f1f7528
Cr-Commit-Position: refs/heads/master@{#372715}
==========

[commit-bot: I haz the power, 2016-02-01 19:16:56]

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