Bug 1524480 - Set NS_FRAME_MAY_BE_TRANSFORMED bit when we have nsChangeHint_UpdateTransformLayer; r=hiro
authorBrian Birtles <birtles@gmail.com>
Fri, 15 Feb 2019 06:35:55 +0000
changeset 459522 656ababf4eda2a93a460b5129d1f615c43f96695
parent 459521 0d8937fbde0faed0df91bd523cad61101e660621
child 459523 1f26e2c082a8cd867431646765652a01381465e5
push id111964
push usercsabou@mozilla.com
push dateFri, 15 Feb 2019 18:54:44 +0000
treeherdermozilla-inbound@db3c4f905082 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1524480
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1524480 - Set NS_FRAME_MAY_BE_TRANSFORMED bit when we have nsChangeHint_UpdateTransformLayer; r=hiro Typically we set the NS_FRAME_MAY_BE_TRANSFORMED bit on a frame when one of the following situations arises: a. We update the keyframes of a KeyframeEffect to include transforms or we create a new KeyframeEffect that animates transforms (in KeyframeEffect::SetKeyframes), or b. We retarget a KeyframeEffect with transforms at a new element (in KeyframeEffect::SetTarget), or c. We create an nsFrame with transform animations applied to it (in nsFrame::Init), or d. We get a nsChangeHint_AddOrRemoveTransform hint in RestyleManager::ProcessRestyledFrames and decide to update the frame bit on the frame and its continuations accordingly. However, there are some situations where we can have a transform animation on a frame where none of the above are triggered. For example, if the style frame is not unavailable (e.g. a display:none element) when the KeyframeEffect is initialized we will fail to the frame bit in (a) and if we never retarget the effect we will never set reach (b). Furthermore, if we have an animation that is "not relevant" (e.g. idle) and hence not registered with the EffectSet when the frame is initialized we will fail to set the frame bit in (c). Finally, if the the animation does not produce a style change that causes the nsChangeHint_AddOrRemoveTransform bit to be set (e.g. the transform animation begins at 'none') we will not set the frame bit in (d). As a result, we can end up failing to set the NS_FRAME_MAY_BE_TRANSFORMED bit for some content. The crashtest included in this patch produces such a case and, without the code changes in this patch, will fail the assertion in ApplyRenderingChangeToTree[1]: NS_ASSERTION(!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle(), "Unexpected UpdateTransformLayer hint"); That is because although the nsChangeHint_UpdateTransformLayer bit will be set, aFrame->IsTransformed() will return false because the frame bit has not been set, and aFrame->StyleDisplay()->HasTransformStyle() will return false because the animation sets the transform to 'none'. Not only will this assertion fail, but once we cease flushing style as part of triggering an animation later in this patch, the reftest layout/reftests/web-animations/stacking-context-transform-changing-effect.html will begin to fail. That reftest produces a similar situation to the crashtest but it currently does not fail because the style flush that happens as part of creating an animation ensures the style frame is available at the point when the animation is triggered and hence case (a) from above is hit. This patch addresses this by detecting the case where we have this change hint set but not the corresponding frame bit, and setting the frame bit. [1] https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/layout/base/RestyleManager.cpp#1191-1199 Differential Revision: https://phabricator.services.mozilla.com/D18911
dom/animation/test/crashtests/1524480-1.html
dom/animation/test/crashtests/crashtests.list
layout/base/RestyleManager.cpp
new file mode 100644
--- /dev/null
+++ b/dom/animation/test/crashtests/1524480-1.html
@@ -0,0 +1,37 @@
+<!doctype html>
+<html class="reftest-wait">
+<meta charset=utf-8>
+<style>
+div {
+  display: none;
+  width: 100px;
+  height: 100px;
+  background: blue;
+}
+</style>
+<div id=div></div>
+<script>
+async function test() {
+  const animation = div.animate({ transform: ['none', 'none'] }, 1000);
+  animation.cancel();
+
+  await waitForFrame();
+
+  div.style.display = 'block';
+
+  await waitForFrame();
+  await waitForFrame();
+
+  animation.play();
+  await animation.finished;
+
+  document.documentElement.className = "";
+}
+
+function waitForFrame() {
+  return new Promise(resolve => requestAnimationFrame(resolve));
+}
+
+test();
+</script>
+</html>
--- a/dom/animation/test/crashtests/crashtests.list
+++ b/dom/animation/test/crashtests/crashtests.list
@@ -37,8 +37,9 @@ pref(dom.animations-api.core.enabled,tru
 pref(dom.animations-api.implicit-keyframes.enabled,true) load 1373712-1.html
 pref(dom.animations-api.implicit-keyframes.enabled,true) load 1379606-1.html
 load 1393605-1.html
 load 1400022-1.html
 pref(dom.animations-api.core.enabled,true) pref(dom.animations-api.implicit-keyframes.enabled,true) load 1401809.html
 pref(dom.animations-api.core.enabled,true) pref(dom.animations-api.timelines.enabled,true) pref(dom.animations-api.implicit-keyframes.enabled,true) load 1411318-1.html
 pref(dom.animations-api.implicit-keyframes.enabled,true) load 1468294-1.html
 pref(dom.animations-api.implicit-keyframes.enabled,true) load 1467277-1.html
+load 1524480-1.html
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -1610,16 +1610,34 @@ void RestyleManager::ProcessRestyledFram
 
       if (!(frame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED)) {
         // Frame can not be transformed, and thus a change in transform will
         // have no effect and we should not use the
         // nsChangeHint_UpdatePostTransformOverflow hint.
         hint &= ~nsChangeHint_UpdatePostTransformOverflow;
       }
 
+      if ((hint & nsChangeHint_UpdateTransformLayer) &&
+          !(frame->GetStateBits() & NS_FRAME_MAY_BE_TRANSFORMED) &&
+          frame->HasAnimationOfTransform()) {
+        // If we have an nsChangeHint_UpdateTransformLayer hint but no
+        // corresponding frame bit, it's possible we have a transform animation
+        // with transform style 'none' that was initialized independently from
+        // this frame and associated after the fact.
+        //
+        // In that case we should set the frame bit.
+        //
+        // FIXME: Bug 1527210 - Use the primary frame here instead so that
+        // we handle display: table correctly.
+        for (nsIFrame* cont = frame; cont;
+             cont = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(cont)) {
+          cont->AddStateBits(NS_FRAME_MAY_BE_TRANSFORMED);
+        }
+      }
+
       if (hint & nsChangeHint_AddOrRemoveTransform) {
         // When dropping a running transform animation we will first add an
         // nsChangeHint_UpdateTransformLayer hint as part of the animation-only
         // restyle. During the subsequent regular restyle, if the animation was
         // the only reason the element had any transform applied, we will add
         // nsChangeHint_AddOrRemoveTransform as part of the regular restyle.
         //
         // With the Gecko backend, these two change hints are processed