Majid, would you be able to look into this? Peter or Rob can give more context about blink gen property trees.

I will look into this. 
Can you provide some context on why this is P1? Is it blocking shipping BGPT?

P1 is identifying whether it does.  If we can be sure that the workaround is OK for our existing common milestone commitments, we can take time.  If not, then it is blocking.

I am no longer able to reproduce the original crashes. Without any will-change and with enable-blink-gen-property-trees. I will run a bisect to see if I can find where these got fixed!



Instead I am seeing two other failures which I will try to get to the bottom of:



[6/24] virtual/threaded/fast/animationworklet/animation-worklet-inside-iframe.html failed unexpectedly (renderer crashed)
[22/24] virtual/threaded/fast/animationworklet/worklet-animation-local-time-undefined.html failed unexpectedly (reference mismatch)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ccb96a60862f13d4ab6d8540714c54a60e7a5516

commit ccb96a60862f13d4ab6d8540714c54a60e7a5516
Author: Majid Valipour <majidvp@chromium.org>
Date: Wed Aug 29 19:17:48 2018

[animation-worklet] Removed unnecessary will-change in test

will-change workaround is no longer necessary. Most other cases have been
removed in http://crrev.com/584946

Bug:  873673 
Change-Id: I3f531c58951aba923f390674c7acac58fe39b860
Reviewed-on: https://chromium-review.googlesource.com/1195646
Reviewed-by: Peter Mayo <petermayo@chromium.org>
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587232}
[modify] https://crrev.com/ccb96a60862f13d4ab6d8540714c54a60e7a5516/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/worklet-animation-currentTime.html

So I bisected the "fix" to  "[animation-worklet] Support undefined as default localTime value"

https://chromium.googlesource.com/chromium/src/+/82b3d45811c2ce34b664eacd55258a2e0d367c12

The originally reported test where not setting any local time but previously they were failing because we would default the local time to '0'.
So I don't think this actual problem have actually been fixed but rather masked. Indeed when I add 'effect.localTime = 0' (which is valid to do) in original tests then
they crash the same.   

I have now tracked the issue down to this line [1]. AFAICT, the layout object does not believe it is a stacking context so PPTB does not build an effect node for it. If I comment out /* style.IsStackingContext() */ then the tests pass fine!

Obviously 'will-change' creates stacking context but I believe animations should also create one so I am not sure why this is not the case with BGPT. ideas?  


[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc?type=cs&g=0&l=555

I don't believe the spec calls for a stacking-context. It calls for treating animating elements as if a 'will-change' is present for the given property (https://drafts.csswg.org/web-animations-1/#side-effects-section), but we don't currently do that ( issue 764783 , xidachen@ was working on it in his spare time).

Quickest fix would likely be a temporary is-animating check there too, or you could be a hero and help xidachen fix  issue 764783  ! :)

(To be clear, it doesn't call *directly* for a stacking context. But one would be applied in the case of, e.g., will-change: transform).

I filed issue 882625 to track having animations imply will-change in general. My quick hack to do just that seems to break stuff so it is a bit more involved.

Meanwhile, I have an initial patch that is adding a check for active composited animation when stacking context is being decided: https://chromium-review.googlesource.com/c/chromium/src/+/1217445

That seems to be fixing the issue at hand.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b9b95d27975aa1d19476c54e75f2c630c38f4b7

commit 8b9b95d27975aa1d19476c54e75f2c630c38f4b7
Author: Majid Valipour <majidvp@chromium.org>
Date: Wed Sep 12 19:35:22 2018

[BGPT] Take active animation into account for stacking context calc

BGPT requires the layout object to be a stacking context for it to have an
effect node. Composited opacity animations require effect node but they
are not causing an stacking context. This breaks BGPT.

This patch makes active composited animations cause stacking context.
This is in line with specification which requires active animations to
behave as if there is an equivalent will-change. [1]

In effect this fix is a workaround until we actually match the
specification. See crbug.com/882625.

[1] https://drafts.csswg.org/web-animations-1/#side-effects-section

luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;

Bug:  873673 
Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I6090c1b4bb0be7848f0c05119f3347a25b746f51
Reviewed-on: https://chromium-review.googlesource.com/1217445
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590789}
[add] https://crrev.com/8b9b95d27975aa1d19476c54e75f2c630c38f4b7/third_party/WebKit/LayoutTests/compositing/animation/animations-establish-stacking-context-expected.html
[add] https://crrev.com/8b9b95d27975aa1d19476c54e75f2c630c38f4b7/third_party/WebKit/LayoutTests/compositing/animation/animations-establish-stacking-context.html
[modify] https://crrev.com/8b9b95d27975aa1d19476c54e75f2c630c38f4b7/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animator-animate.html
[modify] https://crrev.com/8b9b95d27975aa1d19476c54e75f2c630c38f4b7/third_party/blink/renderer/core/style/computed_style.cc

I checked the last run of blink-gen-property-run bot [1] and it none of the above tests are failing. Marking as fixed. 

[1] https://test-results.appspot.com/data/layout_results/linux-blink-gen-property-trees/487/layout-test-results/results.html