[DL]: Ensure to reject update/commit promises on disconnected elements.

This patch ensures that we reject update/commit when it happens on a disconnected element. R=chrishtr@chromium.org Bug: 882663 Change-Id: Iaee923a4f41714b437c0a2b85bbb175359a2f32c Reviewed-on: https://chromium-review.googlesource.com/c/1387944Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#618713}

[DL]: Ensure to reject update/commit promises on disconnected elements.
This patch ensures that we reject update/commit when it happens on a disconnected element. R=chrishtr@chromium.org Bug: 882663 Change-Id: Iaee923a4f41714b437c0a2b85bbb175359a2f32c Reviewed-on: https://chromium-review.googlesource.com/c/1387944Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#618713}
5f4ce97e · Vladimir Levin · Commit Bot · f48817a9 · 5f4ce97e · 5f4ce97e
Commit 5f4ce97e authored Dec 22, 2018 by Vladimir Levin Committed by Commit Bot Dec 22, 2018
6 changed files
--- a/third_party/blink/renderer/core/display_lock/display_lock_context.cc
+++ b/third_party/blink/renderer/core/display_lock/display_lock_context.cc
@@ -139,8 +139,8 @@ ScriptPromise DisplayLockContext::acquire(ScriptState* script_state,
 }

 ScriptPromise DisplayLockContext::update(ScriptState* script_state) {
-  // Reject if we're unlocked.
-  if (state_ == kUnlocked)
+  // Reject if we're unlocked or disconnected.
+  if (state_ == kUnlocked || !element_->isConnected())
    return GetRejectedPromise(script_state);

  // If we have a resolver, then we're at least updating already, just return
@@ -151,16 +151,13 @@ ScriptPromise DisplayLockContext::update(ScriptState* script_state) {
  }

  update_resolver_ = ScriptPromiseResolver::Create(script_state);
-  // We only need to kick off an Update if we're in a locked state, all other
-  // states are already updating.
-  if (state_ == kLocked)
-    StartUpdate();
+  StartUpdateIfNeeded();
  return update_resolver_->Promise();
 }

 ScriptPromise DisplayLockContext::commit(ScriptState* script_state) {
-  // Reject if we're unlocked.
-  if (state_ == kUnlocked)
+  // Reject if we're unlocked or disonnected.
+  if (state_ == kUnlocked || !element_->isConnected())
    return GetRejectedPromise(script_state);

  // If we have a resolver, we must be committing already, just return the same
@@ -391,12 +388,19 @@ void DisplayLockContext::StartCommit() {
      layout_invalidation_reason::kDisplayLockCommitting);
 }

-void DisplayLockContext::StartUpdate() {
-  DCHECK_EQ(state_, kLocked);
-  state_ = kUpdating;
+void DisplayLockContext::StartUpdateIfNeeded() {
+  // We should not be calling this if we're unlocked.
+  DCHECK_NE(state_, kUnlocked);
+  // Any state other than kLocked means that we are already in the process of
+  // updating/committing, so we can piggy back on that process without kicking
+  // off any new updates.
+  if (state_ != kLocked)
+    return;
+
  // We don't need to mark anything dirty since the budget will take care of
  // that for us.
  update_budget_ = CreateNewBudget();
+  state_ = kUpdating;
  ScheduleAnimation();
 }

@@ -509,6 +513,8 @@ void DisplayLockContext::DidFinishLifecycleUpdate() {
 }

 void DisplayLockContext::ScheduleAnimation() {
+  DCHECK(element_->isConnected());
+
  // Schedule an animation to perform the lifecycle phases.
  element_->GetDocument().GetPage()->Animator().ScheduleVisualUpdate(
      element_->GetDocument().GetFrame());

--- a/third_party/blink/renderer/core/display_lock/display_lock_context.h
+++ b/third_party/blink/renderer/core/display_lock/display_lock_context.h
@@ -149,7 +149,7 @@ class CORE_EXPORT DisplayLockContext final
  // Initiate a commit.
  void StartCommit();
  // Initiate an update.
-  void StartUpdate();
+  void StartUpdateIfNeeded();

  // The following functions propagate dirty bits from the locked element up to
  // the ancestors in order to be reached. They return true if the element or

--- a/third_party/blink/web_tests/display-lock/lock-before-append/commit-while-disconnected-expected.html
+++ b/third_party/blink/web_tests/display-lock/lock-before-append/commit-while-disconnected-expected.html
+<!doctype HTML>
+
+<div id="log">PASS</div>
+
--- a/third_party/blink/web_tests/display-lock/lock-before-append/commit-while-disconnected.html
+++ b/third_party/blink/web_tests/display-lock/lock-before-append/commit-while-disconnected.html
+<!doctype HTML>
+
+<!--
+Runs an acquire(), then commits without connecting the element.
+-->
+
+<div id="log"></div>
+
+<script>
+// TODO(vmpstr): In WPT this needs to be replaced with reftest-wait.
+if (window.testRunner)
+  window.testRunner.waitUntilDone();
+
+function finishTest(status_string) {
+  if (document.getElementById("log").innerHTML === "")
+    document.getElementById("log").innerHTML = status_string;
+  if (window.testRunner)
+    window.testRunner.notifyDone();
+}
+
+function runTest() {
+  let container = document.createElement("div");
+  container.id = "container";
+
+  container.getDisplayLock().acquire({ timeout: Infinity }).then(() => {
+    container.getDisplayLock().commit().then(
+      () => { finishTest("FAIL"); },
+      () => { finishTest("PASS"); });
+  });
+}
+
+window.onload = runTest;
+</script>
--- a/third_party/blink/web_tests/display-lock/lock-before-append/update-while-disconnected-expected.html
+++ b/third_party/blink/web_tests/display-lock/lock-before-append/update-while-disconnected-expected.html
+<!doctype HTML>
+
+<div id="log">PASS</div>
+
--- a/third_party/blink/web_tests/display-lock/lock-before-append/update-while-disconnected.html
+++ b/third_party/blink/web_tests/display-lock/lock-before-append/update-while-disconnected.html
+<!doctype HTML>
+
+<!--
+Runs an acquire(), then updates without connecting the element.
+-->
+
+<div id="log"></div>
+
+<script>
+// TODO(vmpstr): In WPT this needs to be replaced with reftest-wait.
+if (window.testRunner)
+  window.testRunner.waitUntilDone();
+
+function finishTest(status_string) {
+  if (document.getElementById("log").innerHTML === "")
+    document.getElementById("log").innerHTML = status_string;
+  if (window.testRunner)
+    window.testRunner.notifyDone();
+}
+
+function runTest() {
+  let container = document.createElement("div");
+  container.id = "container";
+
+  container.getDisplayLock().acquire({ timeout: Infinity }).then(() => {
+    container.getDisplayLock().update().then(
+      () => { finishTest("FAIL"); },
+      () => { finishTest("PASS"); });
+  });
+}
+
+window.onload = runTest;
+</script>