Re-do close watcher user activation tracking

Our previous approach for close watcher user activation tracking had
cases where we could  allow the cancel event to fire, but we were not
allowing it.

Fixing this is nontrivial. We need to more closely track the allowed
number of close watchers, and use it when making decisions about
creating ungrouped close watchers or firing cancel events. This allows
us to pass test cases like:

* A close watcher stack that is relatively empty compared to the amount
  of user activations so far, needs to allow cancel events.

* A close watcher stack that is full compared to the amount of user
  activations so far, needs to prevent cancel events.

Additionally, our previous mechanism of tracking groups by using
booleans on the close watchers was buggy when a close watcher was
destroyed. Instead, properly track the groups as a vector of vectors.

Spec PR: whatwg/html#10168

Bug: 1512224
Change-Id: I6d7ccdc27c69f457455f517dcdbcc71d615b4290
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5232387
Reviewed-by: Joey Arhar <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1268262}

third_party/blink/renderer/core/frame/local_frame.cc

@@ -2700,6 +2700,9 @@ void LocalFrame::SetHadUserInteraction(bool had_user_interaction) {
     history_user_activation_state_.Clear();
   }
 
+  DomWindow()->closewatcher_stack()->SetHadUserInteraction(
+      had_user_interaction);
+
   GetFrameScheduler()->SetHadUserActivation(had_user_interaction);
 }
 

third_party/blink/renderer/core/html/closewatcher/close_watcher.cc

@@ -5,6 +5,7 @@
 #include "third_party/blink/renderer/core/html/closewatcher/close_watcher.h"
 
 #include "base/auto_reset.h"
+#include "base/containers/adapters.h"
 #include "third_party/blink/public/mojom/frame/frame.mojom-blink.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_close_watcher_options.h"
 #include "third_party/blink/renderer/core/dom/abort_signal.h"
@@ -43,52 +44,112 @@ CloseWatcher::WatcherStack::WatcherStack(LocalDOMWindow* window)
     : receiver_(this, window), window_(window) {}
 
 void CloseWatcher::WatcherStack::Add(CloseWatcher* watcher) {
-  if (watchers_.empty()) {
+  if (watcher_groups_.empty()) {
     auto& host = window_->GetFrame()->GetLocalFrameHostRemote();
     host.SetCloseListener(receiver_.BindNewPipeAndPassRemote(
         window_->GetTaskRunner(TaskType::kMiscPlatformAPI)));
   }
-  watchers_.insert(watcher);
+
+  if (watcher_groups_.size() < allowed_groups_) {
+    HeapVector<Member<CloseWatcher>> group;
+    group.push_back(watcher);
+    watcher_groups_.push_back(group);
+  } else {
+    // watcher_groups_ should never be empty in this branch, because
+    // allowed_groups_ should always be >= 1 and so if watcher_groups_ is empty
+    // we would have taken the above branch.
+    CHECK(!watcher_groups_.empty());
+    watcher_groups_.back().push_back(watcher);
+  }
+
+  next_user_interaction_creates_a_new_allowed_group_ = true;
 }
 
 void CloseWatcher::WatcherStack::Remove(CloseWatcher* watcher) {
-  watchers_.erase(watcher);
-  if (watchers_.empty()) {
+  for (auto& group : watcher_groups_) {
+    auto watcher_it = std::find(group.begin(), group.end(), watcher);
+    if (watcher_it != group.end()) {
+      group.erase(watcher_it);
+      if (group.empty()) {
+        auto group_it =
+            std::find(watcher_groups_.begin(), watcher_groups_.end(), group);
+        watcher_groups_.erase(group_it);
+      }
+      break;
+    }
+  }
+
+  if (watcher_groups_.empty()) {
     receiver_.reset();
   }
 }
 
-void CloseWatcher::WatcherStack::Trace(Visitor* visitor) const {
-  visitor->Trace(watchers_);
-  visitor->Trace(receiver_);
-  visitor->Trace(window_);
+void CloseWatcher::WatcherStack::SetHadUserInteraction(
+    bool had_user_interaction) {
+  if (had_user_interaction) {
+    // We don't quite want to give one new allowed group for every user
+    // interaction. That would allow "banking" user interactions in a way that's
+    // a bit user-hostile: e.g., if the user clicks 20 times in a row with the
+    // page not responding at all, then the page would get 20 allowed groups,
+    // which at some later time it could use to create 20 close watchers.
+    // Instead, each time the user interacts with the page, the page has an
+    // *opportunity* to create a new ungrouped close watcher. But if the page
+    // doesn't use it, we don't bank the user interaction for the future. This
+    // ties close watcher creation to specific user interactions.
+    //
+    // In short:
+    // - OK: user interaction -> create ungrouped close watcher ->
+    //       user interaction -> create ungrouped close watcher
+    // - Not OK: user interaction x2 -> create ungrouped close watcher x2
+    //
+    // This does not prevent determined abuse and is not important for upholding
+    // our ultimate invariant, of (# of back presses to escape the page) <= (#
+    // of user interactions) + 2. A determined abuser will just create one close
+    // watcher per user interaction, banking them for future abuse. But it
+    // causes more predictable behavior for the normal case, and encourages
+    // non-abusive developers to create close watchers directly corresponding to
+    // user interactions.
+    if (next_user_interaction_creates_a_new_allowed_group_) {
+      ++allowed_groups_;
+    }
+    next_user_interaction_creates_a_new_allowed_group_ = false;
+  } else {
+    allowed_groups_ = 1;
+    next_user_interaction_creates_a_new_allowed_group_ = true;
+  }
+}
+
+bool CloseWatcher::WatcherStack::CanFireCancelEvent() const {
+  return watcher_groups_.size() < allowed_groups_ &&
+         window_->GetFrame()->IsHistoryUserActivationActive();
 }
 
 void CloseWatcher::WatcherStack::EscapeKeyHandler(KeyboardEvent* event) {
-  if (!watchers_.empty() && !event->DefaultHandled() && event->isTrusted() &&
-      event->keyCode() == VKEY_ESCAPE) {
+  if (!watcher_groups_.empty() && !event->DefaultHandled() &&
+      event->isTrusted() && event->keyCode() == VKEY_ESCAPE) {
     Signal();
   }
 }
 
 void CloseWatcher::WatcherStack::Signal() {
-  while (!watchers_.empty()) {
-    CloseWatcher* watcher = watchers_.back();
-    watcher->requestClose();
-
-    if (!watcher->IsGroupedWithPrevious()) {
-      break;
+  if (!watcher_groups_.empty()) {
+    auto& group = watcher_groups_.back();
+    for (auto& watcher : base::Reversed(group)) {
+      if (!watcher->requestClose()) {
+        break;
+      }
     }
   }
-}
 
-bool CloseWatcher::WatcherStack::HasConsumedFreeWatcher() const {
-  for (const auto& watcher : watchers_) {
-    if (!watcher->created_with_user_activation_) {
-      return true;
-    }
+  if (allowed_groups_ > 1) {
+    --allowed_groups_;
   }
-  return false;
+}
+
+void CloseWatcher::WatcherStack::Trace(Visitor* visitor) const {
+  visitor->Trace(watcher_groups_);
+  visitor->Trace(receiver_);
+  visitor->Trace(window_);
 }
 
 // static
@@ -125,18 +186,6 @@ CloseWatcher* CloseWatcher::CreateInternal(LocalDOMWindow& window,
 
   CloseWatcher* watcher = MakeGarbageCollected<CloseWatcher>(window);
 
-  if (window.GetFrame()->IsHistoryUserActivationActive()) {
-    window.GetFrame()->ConsumeHistoryUserActivation();
-    watcher->created_with_user_activation_ = true;
-    watcher->grouped_with_previous_ = false;
-  } else if (!stack.HasConsumedFreeWatcher()) {
-    watcher->created_with_user_activation_ = false;
-    watcher->grouped_with_previous_ = false;
-  } else {
-    watcher->created_with_user_activation_ = false;
-    watcher->grouped_with_previous_ = true;
-  }
-
   if (options && options->hasSignal()) {
     AbortSignal* signal = options->signal();
     if (signal->aborted()) {
@@ -154,12 +203,13 @@ CloseWatcher* CloseWatcher::CreateInternal(LocalDOMWindow& window,
 CloseWatcher::CloseWatcher(LocalDOMWindow& window)
     : ExecutionContextClient(&window) {}
 
-void CloseWatcher::requestClose() {
+bool CloseWatcher::requestClose() {
   if (IsClosed() || dispatching_cancel_ || !DomWindow()) {
-    return;
+    return true;
   }
 
-  if (DomWindow()->GetFrame()->IsHistoryUserActivationActive()) {
+  WatcherStack& stack = *DomWindow()->closewatcher_stack();
+  if (stack.CanFireCancelEvent()) {
     Event& cancel_event = *Event::CreateCancelable(event_type_names::kCancel);
     {
       base::AutoReset<bool> scoped_committing(&dispatching_cancel_, true);
@@ -169,11 +219,12 @@ void CloseWatcher::requestClose() {
       if (DomWindow()) {
         DomWindow()->GetFrame()->ConsumeHistoryUserActivation();
       }
-      return;
+      return false;
     }
   }
 
   close();
+  return true;
 }
 
 void CloseWatcher::close() {

third_party/blink/renderer/core/html/closewatcher/close_watcher.h

@@ -12,6 +12,7 @@
 #include "third_party/blink/renderer/core/execution_context/execution_context_lifecycle_observer.h"
 #include "third_party/blink/renderer/platform/heap/collection_support/heap_linked_hash_set.h"
 #include "third_party/blink/renderer/platform/mojo/heap_mojo_receiver.h"
+#include "third_party/blink/renderer/platform/wtf/wtf_size_t.h"
 
 namespace blink {
 
@@ -34,9 +35,10 @@ class CloseWatcher final : public EventTarget, public ExecutionContextClient {
   void Trace(Visitor*) const override;
 
   bool IsClosed() const { return state_ == State::kClosed; }
-  bool IsGroupedWithPrevious() const { return grouped_with_previous_; }
 
-  void requestClose();
+  // Note: return value is not exposed to JS via IDL; it's only for internal
+  // use.
+  bool requestClose();
   void close();
   void destroy();
 
@@ -49,19 +51,20 @@ class CloseWatcher final : public EventTarget, public ExecutionContextClient {
     return ExecutionContextClient::GetExecutionContext();
   }
 
-  // If multiple CloseWatchers are active in a given window, they form a
-  // stack, and a close request will pop the top watcher. If the stack is empty,
-  // the first CloseWatcher is "free", but creating a new
-  // CloseWatcher when the stack is non-empty requires a user activation.
+  // If multiple close watchers are active in a given window, they form a stack
+  // of groups of close watchers. Groups close together in response to a single
+  // close request, and new close watchers are either added to the topmost group
+  // or to a new group depending on user activation state.
   class WatcherStack final : public GarbageCollected<WatcherStack>,
                              public mojom::blink::CloseListener {
    public:
     explicit WatcherStack(LocalDOMWindow*);
 
     void Add(CloseWatcher*);
     void Remove(CloseWatcher*);
-    bool HasActiveWatcher() const { return !watchers_.empty(); }
-    bool HasConsumedFreeWatcher() const;
+
+    void SetHadUserInteraction(bool);
+    bool CanFireCancelEvent() const;
 
     void Trace(Visitor*) const;
 
@@ -71,7 +74,9 @@ class CloseWatcher final : public EventTarget, public ExecutionContextClient {
     // mojom::blink::CloseListener override:
     void Signal() final;
 
-    HeapLinkedHashSet<Member<CloseWatcher>> watchers_;
+    HeapVector<HeapVector<Member<CloseWatcher>>> watcher_groups_;
+    bool next_user_interaction_creates_a_new_allowed_group_ = true;
+    wtf_size_t allowed_groups_ = 1;
 
     // Holds a pipe which the service uses to notify this object
     // when the idle state has changed.
@@ -87,8 +92,6 @@ class CloseWatcher final : public EventTarget, public ExecutionContextClient {
   enum class State { kActive, kClosed };
   State state_ = State::kActive;
   bool dispatching_cancel_ = false;
-  bool grouped_with_previous_ = false;
-  bool created_with_user_activation_ = false;
   Member<AbortSignal::AlgorithmHandle> abort_handle_;
 };
 

third_party/blink/web_tests/external/wpt/html/semantics/interactive-elements/the-dialog-element/dialog-canceling.html

@@ -66,6 +66,7 @@
     event.preventDefault();
   });
 
+  await test_driver.bless();
   bottomDialog.showModal();
   await blessTopLayer(bottomDialog);
   topDialog.showModal();