Bug 1479681: Fix the loop in nsTransitionManager dealing with stopping `all` transitions. r=hiro a=aryx
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 31 Jul 2018 11:32:53 +0200
changeset 484430 0d72c7996d60
parent 484429 5a49a2ff0ee0
child 484440 5debea1e5341
child 484468 b28bc7c7b6f8
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro, aryx
bugs1479681, 1478990, 1309752
milestone63.0a1
first release with
nightly linux32
0d72c7996d60 / 63.0a1 / 20180731105217 / files
nightly linux64
0d72c7996d60 / 63.0a1 / 20180731105217 / files
nightly mac
0d72c7996d60 / 63.0a1 / 20180731105217 / files
nightly win32
0d72c7996d60 / 63.0a1 / 20180731105217 / files
nightly win64
0d72c7996d60 / 63.0a1 / 20180731105217 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1479681: Fix the loop in nsTransitionManager dealing with stopping `all` transitions. r=hiro a=aryx The loop was mutating the nsCSSPropertyID used to guard the exit, which is obviously wrong. This branch is pretty rarely taken, since people don't usually specify `all` as a transition property other than the first, for which case we take the fast path with `checkProperties = false`. Our test-suite failed to catch this. Added a crashtest that hangs without this patch. The reason bug 1478990 regressed this is because it changed the order of nsCSSPropertyID so that `p` actually went backwards causing the infinite loop, but the bug was introduced (by me, whoops) in bug 1309752. Differential Revision: https://phabricator.services.mozilla.com/D2552 MozReview-Commit-ID: Ii3D1FaZ31R
layout/style/crashtests/1479681.html
layout/style/crashtests/crashtests.list
layout/style/nsTransitionManager.cpp
new file mode 100644
--- /dev/null
+++ b/layout/style/crashtests/1479681.html
@@ -0,0 +1,14 @@
+<!doctype html>
+<style>
+  div {
+    color: red;
+    transition: margin 1s ease, all 2s;
+  }
+</style>
+<div>Should turn Green</div>
+<script>
+onload = function() {
+  document.querySelector('div').style.color = "green";
+  document.body.offsetTop;
+}
+</script>
--- a/layout/style/crashtests/crashtests.list
+++ b/layout/style/crashtests/crashtests.list
@@ -286,8 +286,9 @@ load 1450691.html
 pref(dom.webcomponents.shadowdom.enabled,true) load 1453206.html
 load 1454140.html
 load 1455108.html
 load 1457288.html
 load 1457985.html
 pref(dom.webcomponents.shadowdom.enabled,true) load 1468640.html
 load 1469076.html
 load 1475003.html
+load 1479681.html
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -539,18 +539,17 @@ nsTransitionManager::DoUpdateTransitions
         if (property == eCSSPropertyExtra_no_properties ||
             property == eCSSPropertyExtra_variable ||
             property == eCSSProperty_UNKNOWN) {
           // Nothing to do, but need to exclude this from cases below.
         } else if (property == eCSSPropertyExtra_all_properties) {
           for (nsCSSPropertyID p = nsCSSPropertyID(0);
                p < eCSSProperty_COUNT_no_shorthands;
                p = nsCSSPropertyID(p + 1)) {
-            p = nsCSSProps::Physicalize(p, aNewStyle);
-            allTransitionProperties.AddProperty(p);
+            allTransitionProperties.AddProperty(nsCSSProps::Physicalize(p, aNewStyle));
           }
         } else if (nsCSSProps::IsShorthand(property)) {
           CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(
               subprop, property, CSSEnabledState::eForAllContent) {
             auto p = nsCSSProps::Physicalize(*subprop, aNewStyle);
             allTransitionProperties.AddProperty(p);
           }
         } else {