[Autofill Assistant] Have CheckOnTop check all frames.

Before this change, CheckOnTop would only check the current frame. While
this could detect the case where an element covers the goal element,
this did not detect the case where an element covers the iframe
containing the element.

With this change, CheckOnTop requires the target element as well as any
iframe containing the target element to be on top (not covered by
another element).

Bug: b/171463353
Change-Id: I3d32fdff3cb55724fd096de18034e9b4e002cab3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2523172
Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#825777}

components/autofill_assistant/browser/BUILD.gn

@@ -205,6 +205,8 @@ static_library("browser") {
     "value_util.cc",
     "value_util.h",
     "viewport_mode.h",
+    "web/check_on_top_worker.cc",
+    "web/check_on_top_worker.h",
     "web/element_finder.cc",
     "web/element_finder.h",
     "web/element_position_getter.cc",

components/autofill_assistant/browser/web/check_on_top_worker.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/autofill_assistant/browser/web/check_on_top_worker.h"
+
+#include "base/logging.h"
+#include "components/autofill_assistant/browser/web/web_controller_util.h"
+
+namespace autofill_assistant {
+
+CheckOnTopWorker::CheckOnTopWorker(DevtoolsClient* devtools_client)
+    : devtools_client_(devtools_client) {}
+
+CheckOnTopWorker::~CheckOnTopWorker() {}
+
+void CheckOnTopWorker::Start(const ElementFinder::Result& element,
+                             Callback callback) {
+  callback_ = std::move(callback);
+
+  // Each containing iframe must be checked to detect the case where an iframe
+  // element is covered by another element. The snippets might be run in
+  // parallel, if iframes run in different JavaScript contexts.
+
+  JsSnippet js_snippet;
+  js_snippet.AddLine("function(element) {");
+  AddReturnIfOnTop(&js_snippet, "element",
+                   /* on_top= */ "true",
+                   /* not_on_top= */ "false",
+                   /* not_in_view= */ "false");
+  js_snippet.AddLine("}");
+  std::string function = js_snippet.ToString();
+
+  pending_result_count_ = element.frame_stack.size() + 1;
+  for (const auto& frame : element.frame_stack) {
+    CallFunctionOn(function, frame.node_frame_id, frame.object_id);
+  }
+  CallFunctionOn(function, element.node_frame_id, element.object_id);
+}
+
+void CheckOnTopWorker::CallFunctionOn(const std::string& function,
+                                      const std::string& frame_id,
+                                      const std::string& object_id) {
+  std::vector<std::unique_ptr<runtime::CallArgument>> arguments;
+  AddRuntimeCallArgumentObjectId(object_id, &arguments);
+  devtools_client_->GetRuntime()->CallFunctionOn(
+      runtime::CallFunctionOnParams::Builder()
+          .SetObjectId(object_id)
+          .SetArguments(std::move(arguments))
+          .SetFunctionDeclaration(function)
+          .SetReturnByValue(true)
+          .Build(),
+      frame_id,
+      base::BindOnce(&CheckOnTopWorker::OnReply,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void CheckOnTopWorker::OnReply(
+    const DevtoolsClient::ReplyStatus& reply_status,
+    std::unique_ptr<runtime::CallFunctionOnResult> result) {
+  if (!callback_) {
+    // Already answered.
+    return;
+  }
+
+  ClientStatus status =
+      CheckJavaScriptResult(reply_status, result.get(), __FILE__, __LINE__);
+  if (!status.ok()) {
+    VLOG(1) << __func__ << " Failed JavaScript with status: " << status;
+    std::move(callback_).Run(status);
+    return;
+  }
+
+  bool onTop = false;
+  if (!SafeGetBool(result->GetResult(), &onTop)) {
+    VLOG(1) << __func__ << " JavaScript function failed to return a boolean.";
+    std::move(callback_).Run(UnexpectedErrorStatus(__FILE__, __LINE__));
+    return;
+  }
+
+  if (!onTop) {
+    std::move(callback_).Run(ClientStatus(ELEMENT_NOT_ON_TOP));
+    return;
+  }
+
+  if (pending_result_count_ == 1) {
+    std::move(callback_).Run(OkClientStatus());
+    return;
+  }
+
+  // Wait for a result from other frames.
+  DCHECK_GT(pending_result_count_, 1u);
+  pending_result_count_--;
+}
+
+}  // namespace autofill_assistant

components/autofill_assistant/browser/web/check_on_top_worker.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_WEB_CHECK_ON_TOP_WORKER_H_
+#define COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_WEB_CHECK_ON_TOP_WORKER_H_
+
+#include <memory>
+#include <vector>
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "components/autofill_assistant/browser/client_status.h"
+#include "components/autofill_assistant/browser/devtools/devtools/domains/types_runtime.h"
+#include "components/autofill_assistant/browser/devtools/devtools_client.h"
+#include "components/autofill_assistant/browser/web/element_finder.h"
+#include "components/autofill_assistant/browser/web/web_controller_worker.h"
+
+namespace autofill_assistant {
+
+// Worker class to check whether an element is on top, in all frames.
+class CheckOnTopWorker : public WebControllerWorker {
+ public:
+  // |devtools_client| must be valid for the lifetime of the instance.
+  CheckOnTopWorker(DevtoolsClient* devtools_client);
+  ~CheckOnTopWorker() override;
+
+  // Callback called when the worker is done.
+  using Callback = base::OnceCallback<void(const ClientStatus&)>;
+
+  // Have the worker check |element| and report the result to |callback|.
+  void Start(const ElementFinder::Result& element, Callback callback);
+
+ private:
+  void CallFunctionOn(const std::string& function,
+                      const std::string& frame_id,
+                      const std::string& object_id);
+  void OnReply(const DevtoolsClient::ReplyStatus& reply_status,
+               std::unique_ptr<runtime::CallFunctionOnResult> result);
+
+  DevtoolsClient* const devtools_client_;
+  Callback callback_;
+
+  // The number of successful results that are still expected before the check
+  // can be reported as successful. Note that an unsuccessful result is reported
+  // right away.
+  size_t pending_result_count_;
+
+  base::WeakPtrFactory<CheckOnTopWorker> weak_ptr_factory_{this};
+};
+}  // namespace autofill_assistant
+
+#endif  // COMPONENTS_AUTOFILL_ASSISTANT_BROWSER_WEB_CHECK_ON_TOP_WORKER_H_

components/autofill_assistant/browser/web/web_controller.cc

@@ -444,51 +444,28 @@ void WebController::OnWaitForDocumentToBecomeInteractive(
 void WebController::CheckOnTop(
     const ElementFinder::Result& element,
     base::OnceCallback<void(const ClientStatus&)> callback) {
-  JsSnippet js_snippet;
-  js_snippet.AddLine("function(element) {");
-  AddReturnIfOnTop(&js_snippet, "element",
-                   /* on_top= */ "true",
-                   /* not_on_top= */ "false",
-                   /* not_in_view= */ "false");
-  js_snippet.AddLine("}");
-
-  std::vector<std::unique_ptr<runtime::CallArgument>> arguments;
-  AddRuntimeCallArgumentObjectId(element.object_id, &arguments);
-  devtools_client_->GetRuntime()->CallFunctionOn(
-      runtime::CallFunctionOnParams::Builder()
-          .SetObjectId(element.object_id)
-          .SetArguments(std::move(arguments))
-          .SetFunctionDeclaration(js_snippet.ToString())
-          .SetReturnByValue(true)
-          .Build(),
-      element.node_frame_id,
-      base::BindOnce(&WebController::OnCheckOnTop,
-                     weak_ptr_factory_.GetWeakPtr(),
-                     base::BindOnce(&DecorateWebControllerStatus,
-                                    WebControllerErrorInfoProto::ON_TOP,
-                                    std::move(callback))));
+  auto worker = std::make_unique<CheckOnTopWorker>(devtools_client_.get());
+  auto* ptr = worker.get();
+  pending_workers_.emplace_back(std::move(worker));
+  ptr->Start(element,
+             base::BindOnce(&WebController::OnCheckOnTop,
+                            weak_ptr_factory_.GetWeakPtr(), ptr,
+                            base::BindOnce(&DecorateWebControllerStatus,
+                                           WebControllerErrorInfoProto::ON_TOP,
+                                           std::move(callback))));
 }
 
 void WebController::OnCheckOnTop(
+    CheckOnTopWorker* worker_to_release,
     base::OnceCallback<void(const ClientStatus&)> callback,
-    const DevtoolsClient::ReplyStatus& reply_status,
-    std::unique_ptr<runtime::CallFunctionOnResult> result) {
-  ClientStatus status =
-      CheckJavaScriptResult(reply_status, result.get(), __FILE__, __LINE__);
+    const ClientStatus& status) {
+  base::EraseIf(pending_workers_, [worker_to_release](const auto& worker) {
+    return worker.get() == worker_to_release;
+  });
   if (!status.ok()) {
-    VLOG(1) << __func__ << " Failed JavaScript with status: " << status;
-    std::move(callback).Run(status);
-    return;
+    VLOG(1) << __func__ << " Element is not on top: " << status;
   }
-
-  bool onTop = false;
-  if (!SafeGetBool(result->GetResult(), &onTop)) {
-    VLOG(1) << __func__ << " JavaScript function failed to return a boolean.";
-    std::move(callback).Run(UnexpectedErrorStatus(__FILE__, __LINE__));
-    return;
-  }
-  std::move(callback).Run(
-      ClientStatus(onTop ? ACTION_APPLIED : ELEMENT_NOT_ON_TOP));
+  std::move(callback).Run(status);
 }
 
 void WebController::WaitUntilElementIsStable(

components/autofill_assistant/browser/web/web_controller.h

@@ -22,6 +22,7 @@
 #include "components/autofill_assistant/browser/rectf.h"
 #include "components/autofill_assistant/browser/selector.h"
 #include "components/autofill_assistant/browser/top_padding.h"
+#include "components/autofill_assistant/browser/web/check_on_top_worker.h"
 #include "components/autofill_assistant/browser/web/element_finder.h"
 #include "components/autofill_assistant/browser/web/element_position_getter.h"
 #include "components/autofill_assistant/browser/web/element_rect_getter.h"
@@ -310,9 +311,9 @@ class WebController {
   void OnWaitForDocumentToBecomeInteractive(
       base::OnceCallback<void(const ClientStatus&)> callback,
       bool result);
-  void OnCheckOnTop(base::OnceCallback<void(const ClientStatus&)> callback,
-                    const DevtoolsClient::ReplyStatus& reply_status,
-                    std::unique_ptr<runtime::CallFunctionOnResult> result);
+  void OnCheckOnTop(CheckOnTopWorker* worker,
+                    base::OnceCallback<void(const ClientStatus&)> callback,
+                    const ClientStatus& status);
   void OnWaitUntilElementIsStable(
       ElementPositionGetter* getter_to_release,
       base::OnceCallback<void(const ClientStatus&)> callback,

components/autofill_assistant/browser/web/web_controller_browsertest.cc

@@ -875,6 +875,22 @@ document.getElementById("overlay_in_frame").style.visibility='visible';
 )"));
   }
 
+  // Hide the overlay in the main page.
+  void HideOverlay() {
+    EXPECT_TRUE(ExecJs(shell(),
+                       R"(
+document.getElementById("overlay").style.visibility='hidden';
+)"));
+  }
+
+  // Hide the overlay in the first iframe.
+  void HideOverlayInFrame() {
+    EXPECT_TRUE(ExecJs(shell()->web_contents()->GetAllFrames()[1],
+                       R"(
+document.getElementById("overlay_in_frame").style.visibility='hidden';
+)"));
+  }
+
   // Make sure scrolling is necessary for #scroll_container , no matter the
   // screen height
   void SetupScrollContainerHeights() {
@@ -2497,6 +2513,32 @@ IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, CheckOnTop) {
   EXPECT_EQ(WebControllerErrorInfoProto::ON_TOP,
             status.details().web_controller_error_info().failed_web_action());
 }
+
+IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, CheckOnTopInFrame) {
+  ClientStatus status;
+  ElementFinder::Result element;
+  FindElement(Selector({"#iframe", "#button"}), &status, &element);
+  ASSERT_TRUE(status.ok());
+
+  // Make sure the button is visible.
+  EXPECT_TRUE(
+      ExecJs(shell()->web_contents()->GetAllFrames()[1],
+             "document.getElementById('button').scrollIntoViewIfNeeded();"));
+
+  // The button is covered by an overlay in the main frame
+  ShowOverlay();
+  EXPECT_EQ(ELEMENT_NOT_ON_TOP, CheckOnTop(element).proto_status());
+
+  // The button is covered by an overlay in the iframe
+  HideOverlay();
+  ShowOverlayInFrame();
+  EXPECT_EQ(ELEMENT_NOT_ON_TOP, CheckOnTop(element).proto_status());
+
+  // The button is not covered by any overlay
+  HideOverlayInFrame();
+  EXPECT_EQ(ACTION_APPLIED, CheckOnTop(element).proto_status());
+}
+
 IN_PROC_BROWSER_TEST_F(WebControllerBrowserTest, NthMatch) {
   Selector selector;
   selector.proto.add_filters()->set_css_selector(".nth_match_parent");