Commit a36e6e1d authored by Avi Drissman's avatar Avi Drissman Committed by Commit Bot

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

This reverts commit 5f4ce97e.

Reason for revert: On https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29?limit=200, virtual/display-lock/display-lock/lock-before-append/acquire-update-measure-remove.html has been failing about 90% of the time since this landed.

Original change's description:
> [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/1387944
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#618713}

TBR=vmpstr@chromium.org,chrishtr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 882663
Change-Id: I491a3f7db511a69f2538e3158bc4666c6f9eaf85
Reviewed-on: https://chromium-review.googlesource.com/c/1390433Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618847}
parent 92d47cc9
...@@ -139,8 +139,8 @@ ScriptPromise DisplayLockContext::acquire(ScriptState* script_state, ...@@ -139,8 +139,8 @@ ScriptPromise DisplayLockContext::acquire(ScriptState* script_state,
} }
ScriptPromise DisplayLockContext::update(ScriptState* script_state) { ScriptPromise DisplayLockContext::update(ScriptState* script_state) {
// Reject if we're unlocked or disconnected. // Reject if we're unlocked.
if (state_ == kUnlocked || !element_->isConnected()) if (state_ == kUnlocked)
return GetRejectedPromise(script_state); return GetRejectedPromise(script_state);
// If we have a resolver, then we're at least updating already, just return // If we have a resolver, then we're at least updating already, just return
...@@ -151,13 +151,16 @@ ScriptPromise DisplayLockContext::update(ScriptState* script_state) { ...@@ -151,13 +151,16 @@ ScriptPromise DisplayLockContext::update(ScriptState* script_state) {
} }
update_resolver_ = ScriptPromiseResolver::Create(script_state); update_resolver_ = ScriptPromiseResolver::Create(script_state);
StartUpdateIfNeeded(); // 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();
return update_resolver_->Promise(); return update_resolver_->Promise();
} }
ScriptPromise DisplayLockContext::commit(ScriptState* script_state) { ScriptPromise DisplayLockContext::commit(ScriptState* script_state) {
// Reject if we're unlocked or disonnected. // Reject if we're unlocked.
if (state_ == kUnlocked || !element_->isConnected()) if (state_ == kUnlocked)
return GetRejectedPromise(script_state); return GetRejectedPromise(script_state);
// If we have a resolver, we must be committing already, just return the same // If we have a resolver, we must be committing already, just return the same
...@@ -388,19 +391,12 @@ void DisplayLockContext::StartCommit() { ...@@ -388,19 +391,12 @@ void DisplayLockContext::StartCommit() {
layout_invalidation_reason::kDisplayLockCommitting); layout_invalidation_reason::kDisplayLockCommitting);
} }
void DisplayLockContext::StartUpdateIfNeeded() { void DisplayLockContext::StartUpdate() {
// We should not be calling this if we're unlocked. DCHECK_EQ(state_, kLocked);
DCHECK_NE(state_, kUnlocked); state_ = kUpdating;
// 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 // We don't need to mark anything dirty since the budget will take care of
// that for us. // that for us.
update_budget_ = CreateNewBudget(); update_budget_ = CreateNewBudget();
state_ = kUpdating;
ScheduleAnimation(); ScheduleAnimation();
} }
...@@ -513,8 +509,6 @@ void DisplayLockContext::DidFinishLifecycleUpdate() { ...@@ -513,8 +509,6 @@ void DisplayLockContext::DidFinishLifecycleUpdate() {
} }
void DisplayLockContext::ScheduleAnimation() { void DisplayLockContext::ScheduleAnimation() {
DCHECK(element_->isConnected());
// Schedule an animation to perform the lifecycle phases. // Schedule an animation to perform the lifecycle phases.
element_->GetDocument().GetPage()->Animator().ScheduleVisualUpdate( element_->GetDocument().GetPage()->Animator().ScheduleVisualUpdate(
element_->GetDocument().GetFrame()); element_->GetDocument().GetFrame());
......
...@@ -149,7 +149,7 @@ class CORE_EXPORT DisplayLockContext final ...@@ -149,7 +149,7 @@ class CORE_EXPORT DisplayLockContext final
// Initiate a commit. // Initiate a commit.
void StartCommit(); void StartCommit();
// Initiate an update. // Initiate an update.
void StartUpdateIfNeeded(); void StartUpdate();
// The following functions propagate dirty bits from the locked element up to // 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 // the ancestors in order to be reached. They return true if the element or
......
<!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>
<!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>
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment