Fixed UserActivationV2 handling in APIRequestHandler::StartRequest.

Bug: 778769
Change-Id: I8404561578d76a33032a21f050150d35ac0bd553
Reviewed-on: https://chromium-review.googlesource.com/1194336
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591376}

extensions/renderer/bindings/api_binding_test_util.cc

@@ -45,6 +45,8 @@ bool RunFunctionImpl(v8::Local<v8::Function> function,
   return true;
 }
 
+bool g_mock_user_activation_v2_state_ = false;
+
 }  // namespace
 
 std::string ReplaceSingleQuotes(base::StringPiece str) {
@@ -206,4 +208,17 @@ std::string GetStringPropertyFromObject(v8::Local<v8::Object> object,
   return V8ToString(GetPropertyFromObject(object, context, key), context);
 }
 
+ScopedTestUserActivation::ScopedTestUserActivation() {
+  DCHECK(!g_mock_user_activation_v2_state_);  // Nested scopes are not allowed.
+  g_mock_user_activation_v2_state_ = true;
+}
+
+ScopedTestUserActivation::~ScopedTestUserActivation() {
+  g_mock_user_activation_v2_state_ = false;
+}
+
+bool GetTestUserActivationState(v8::Local<v8::Context>) {
+  return g_mock_user_activation_v2_state_;
+}
+
 }  // namespace extensions

extensions/renderer/bindings/api_binding_test_util.h

@@ -125,6 +125,17 @@ std::string GetStringPropertyFromObject(v8::Local<v8::Object> object,
                                         v8::Local<v8::Context> context,
                                         base::StringPiece key);
 
+// User activation mock for test: sets transient activation state on
+// construction, resets on destruction.
+class ScopedTestUserActivation {
+ public:
+  ScopedTestUserActivation();
+  ~ScopedTestUserActivation();
+};
+
+// Returns current transient activation mock state.
+bool GetTestUserActivationState(v8::Local<v8::Context>);
+
 }  // extensions
 
 #endif  // EXTENSIONS_RENDERER_BINDINGS_API_BINDING_TEST_UTIL_H_

extensions/renderer/bindings/api_binding_unittest.cc

@@ -26,7 +26,6 @@
 #include "gin/public/context_holder.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "third_party/blink/public/web/web_scoped_user_gesture.h"
 #include "v8/include/v8.h"
 
 namespace extensions {
@@ -126,9 +125,10 @@ class APIBindingUnittest : public APIBindingTest {
   void SetUp() override {
     APIBindingTest::SetUp();
     request_handler_ = std::make_unique<APIRequestHandler>(
-        base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
+        base::BindRepeating(&APIBindingUnittest::OnFunctionCall,
+                            base::Unretained(this)),
         APILastError(APILastError::GetParent(), binding::AddConsoleError()),
-        nullptr);
+        nullptr, base::BindRepeating(&GetTestUserActivationState));
   }
 
   void TearDown() override {
@@ -1211,12 +1211,13 @@ TEST_F(APIBindingUnittest, TestUserGestures) {
   ASSERT_FALSE(function.IsEmpty());
 
   v8::Local<v8::Value> argv[] = {binding_object};
+
   RunFunction(function, context, arraysize(argv), argv);
   ASSERT_TRUE(last_request());
   EXPECT_FALSE(last_request()->has_user_gesture);
   reset_last_request();
 
-  blink::WebScopedUserGesture user_gesture(nullptr);
+  ScopedTestUserActivation test_user_activation;
   RunFunction(function, context, arraysize(argv), argv);
   ASSERT_TRUE(last_request());
   EXPECT_TRUE(last_request()->has_user_gesture);

extensions/renderer/bindings/api_bindings_system.cc

@@ -16,6 +16,8 @@ APIBindingsSystem::APIBindingsSystem(
     const GetAPISchemaMethod& get_api_schema,
     const BindingAccessChecker::AvailabilityCallback& is_available,
     const APIRequestHandler::SendRequestMethod& send_request,
+    const APIRequestHandler::GetUserActivationState&
+        get_user_activation_state_callback,
     const APIEventListeners::ListenersUpdated& event_listeners_changed,
     const APIEventHandler::ContextOwnerIdGetter& context_owner_getter,
     const APIBinding::OnSilentRequest& on_silent_request,
@@ -26,7 +28,8 @@ APIBindingsSystem::APIBindingsSystem(
       exception_handler_(add_console_error),
       request_handler_(send_request,
                        std::move(last_error),
-                       &exception_handler_),
+                       &exception_handler_,
+                       get_user_activation_state_callback),
       event_handler_(event_listeners_changed,
                      context_owner_getter,
                      &exception_handler_),

extensions/renderer/bindings/api_bindings_system.h

@@ -48,6 +48,8 @@ class APIBindingsSystem {
       const GetAPISchemaMethod& get_api_schema,
       const BindingAccessChecker::AvailabilityCallback& is_available,
       const APIRequestHandler::SendRequestMethod& send_request,
+      const APIRequestHandler::GetUserActivationState&
+          get_user_activation_state_callback,
       const APIEventListeners::ListenersUpdated& event_listeners_changed,
       const APIEventHandler::ContextOwnerIdGetter& context_owner_getter,
       const APIBinding::OnSilentRequest& on_silent_request,

extensions/renderer/bindings/api_bindings_system_unittest.cc

@@ -116,11 +116,14 @@ void APIBindingsSystemTest::SetUp() {
     return std::string("context");
   };
   bindings_system_ = std::make_unique<APIBindingsSystem>(
-      base::Bind(&APIBindingsSystemTest::GetAPISchema, base::Unretained(this)),
-      base::Bind(&AllowAllAPIs),
-      base::Bind(&APIBindingsSystemTest::OnAPIRequest, base::Unretained(this)),
-      base::Bind(&APIBindingsSystemTest::OnEventListenersChanged,
-                 base::Unretained(this)),
+      base::BindRepeating(&APIBindingsSystemTest::GetAPISchema,
+                          base::Unretained(this)),
+      base::BindRepeating(&AllowAllAPIs),
+      base::BindRepeating(&APIBindingsSystemTest::OnAPIRequest,
+                          base::Unretained(this)),
+      base::BindRepeating(&GetTestUserActivationState),
+      base::BindRepeating(&APIBindingsSystemTest::OnEventListenersChanged,
+                          base::Unretained(this)),
       base::BindRepeating(get_context_owner), base::DoNothing(),
       add_console_error,
       APILastError(base::Bind(&APIBindingsSystemTest::GetLastErrorParent,

extensions/renderer/bindings/api_request_handler.cc

@@ -5,8 +5,10 @@
 #include "extensions/renderer/bindings/api_request_handler.h"
 
 #include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/guid.h"
 #include "base/values.h"
+#include "content/public/common/content_features.h"
 #include "content/public/renderer/v8_value_converter.h"
 #include "extensions/renderer/bindings/api_response_validator.h"
 #include "extensions/renderer/bindings/exception_handler.h"
@@ -95,12 +97,15 @@ APIRequestHandler::PendingRequest::PendingRequest(PendingRequest&&) = default;
 APIRequestHandler::PendingRequest& APIRequestHandler::PendingRequest::operator=(
     PendingRequest&&) = default;
 
-APIRequestHandler::APIRequestHandler(const SendRequestMethod& send_request,
-                                     APILastError last_error,
-                                     ExceptionHandler* exception_handler)
+APIRequestHandler::APIRequestHandler(
+    const SendRequestMethod& send_request,
+    APILastError last_error,
+    ExceptionHandler* exception_handler,
+    const GetUserActivationState& get_user_activation_state_callback)
     : send_request_(send_request),
       last_error_(std::move(last_error)),
-      exception_handler_(exception_handler) {}
+      exception_handler_(exception_handler),
+      get_user_activation_state_callback_(get_user_activation_state_callback) {}
 
 APIRequestHandler::~APIRequestHandler() {}
 
@@ -155,8 +160,7 @@ int APIRequestHandler::StartRequest(v8::Local<v8::Context> context,
       request_id,
       PendingRequest(isolate, context, method, callback, callback_args)));
 
-  request->has_user_gesture =
-      blink::WebUserGestureIndicator::IsProcessingUserGestureThreadSafe();
+  request->has_user_gesture = get_user_activation_state_callback_.Run(context);
   request->arguments = std::move(arguments);
   request->method_name = method;
   request->thread = thread;
@@ -253,7 +257,14 @@ void APIRequestHandler::CompleteRequestImpl(int request_id,
   }
   full_args.insert(full_args.end(), response_args.begin(), response_args.end());
 
-  blink::WebScopedUserGesture user_gesture(*pending_request.user_gesture_token);
+  std::unique_ptr<blink::WebScopedUserGesture> user_gesture;
+  // UserActivationV2 replaces the concept of (scoped) tokens with a frame-wide
+  // state, hence skips token forwarding.
+  if (!base::FeatureList::IsEnabled(features::kUserActivationV2)) {
+    user_gesture = std::make_unique<blink::WebScopedUserGesture>(
+        *pending_request.user_gesture_token);
+  }
+
   if (!error.empty())
     last_error_.SetError(context, error);
 

extensions/renderer/bindings/api_request_handler.h

@@ -50,9 +50,14 @@ class APIRequestHandler {
   using SendRequestMethod =
       base::Callback<void(std::unique_ptr<Request>, v8::Local<v8::Context>)>;
 
-  APIRequestHandler(const SendRequestMethod& send_request,
-                    APILastError last_error,
-                    ExceptionHandler* exception_handler);
+  using GetUserActivationState =
+      base::RepeatingCallback<bool(v8::Local<v8::Context>)>;
+
+  APIRequestHandler(
+      const SendRequestMethod& send_request,
+      APILastError last_error,
+      ExceptionHandler* exception_handler,
+      const GetUserActivationState& get_user_activation_state_callback);
   ~APIRequestHandler();
 
   // Begins the process of processing the request. Returns the identifier of the
@@ -148,6 +153,9 @@ class APIRequestHandler {
   // Null if response validation is disabled.
   std::unique_ptr<APIResponseValidator> response_validator_;
 
+  // The callback to determine transient user activation state of the context.
+  GetUserActivationState get_user_activation_state_callback_;
+
   DISALLOW_COPY_AND_ASSIGN(APIRequestHandler);
 };
 

extensions/renderer/bindings/api_request_handler_unittest.cc

@@ -18,8 +18,6 @@
 #include "gin/public/context_holder.h"
 #include "gin/public/isolate_holder.h"
 #include "testing/gmock/include/gmock/gmock.h"
-#include "third_party/blink/public/web/web_scoped_user_gesture.h"
-#include "third_party/blink/public/web/web_user_gesture_indicator.h"
 
 namespace extensions {
 
@@ -43,16 +41,20 @@ class APIRequestHandlerTest : public APIBindingTest {
     return std::make_unique<APIRequestHandler>(
         base::DoNothing(),
         APILastError(APILastError::GetParent(), binding::AddConsoleError()),
-        nullptr);
+        nullptr, base::BindRepeating(&GetTestUserActivationState));
   }
 
+  void SaveUserActivationState(base::Optional<bool>* ran_with_user_gesture) {
+    *ran_with_user_gesture = GetTestUserActivationState(MainContext());
+  };
+
  protected:
   APIRequestHandlerTest() {}
   ~APIRequestHandlerTest() override {}
 
   std::unique_ptr<TestJSRunner::Scope> CreateTestJSRunner() override {
     return std::make_unique<TestJSRunner::Scope>(
-        std::make_unique<TestJSRunner>(base::Bind(
+        std::make_unique<TestJSRunner>(base::BindRepeating(
             &APIRequestHandlerTest::SetDidRunJS, base::Unretained(this))));
   }
 
@@ -278,17 +280,14 @@ TEST_F(APIRequestHandlerTest, UserGestureTest) {
 
   std::unique_ptr<APIRequestHandler> request_handler = CreateRequestHandler();
 
-  auto callback = [](base::Optional<bool>* ran_with_user_gesture) {
-    *ran_with_user_gesture =
-        blink::WebUserGestureIndicator::IsProcessingUserGestureThreadSafe();
-  };
-
   // Set up a callback to be used with the request so we can check if a user
   // gesture was active.
   base::Optional<bool> ran_with_user_gesture;
   v8::Local<v8::FunctionTemplate> function_template =
-      gin::CreateFunctionTemplate(isolate(),
-                                  base::Bind(callback, &ran_with_user_gesture));
+      gin::CreateFunctionTemplate(
+          isolate(),
+          base::BindRepeating(&APIRequestHandlerTest::SaveUserActivationState,
+                              base::Unretained(this), &ran_with_user_gesture));
   v8::Local<v8::Function> v8_callback =
       function_template->GetFunction(context).ToLocalChecked();
 
@@ -305,25 +304,24 @@ TEST_F(APIRequestHandlerTest, UserGestureTest) {
 
   // Next try calling with a user gesture. Since a gesture will be active at the
   // time of the call, it should also be active during the callback.
-  {
-    blink::WebScopedUserGesture user_gesture(nullptr);
-    EXPECT_TRUE(
-        blink::WebUserGestureIndicator::IsProcessingUserGestureThreadSafe());
-    request_id = request_handler->StartRequest(
-        context, kMethod, std::make_unique<base::ListValue>(), v8_callback,
-        v8::Local<v8::Function>(), binding::RequestThread::UI);
-  }
-  EXPECT_FALSE(
-      blink::WebUserGestureIndicator::IsProcessingUserGestureThreadSafe());
 
+  ScopedTestUserActivation test_user_activation;
+  // TODO(devlin): This isn't quite right with UAv1/UAv2.  V1 should properly
+  // activate a new user gesture on the stack, and v2 should rely on the gesture
+  // being persisted (or generated from the browser). We should clean this up.
+
+  EXPECT_TRUE(GetTestUserActivationState(MainContext()));
+
+  request_id = request_handler->StartRequest(
+      context, kMethod, std::make_unique<base::ListValue>(), v8_callback,
+      v8::Local<v8::Function>(), binding::RequestThread::UI);
   request_handler->CompleteRequest(request_id, *ListValueFromString("[]"),
                                    std::string());
   ASSERT_TRUE(ran_with_user_gesture);
   EXPECT_TRUE(*ran_with_user_gesture);
-  // Sanity check - after the callback ran, there shouldn't be an active
-  // gesture.
-  EXPECT_FALSE(
-      blink::WebUserGestureIndicator::IsProcessingUserGestureThreadSafe());
+
+  // Sanity check: the callback doesn't change the state
+  EXPECT_TRUE(GetTestUserActivationState(MainContext()));
 }
 
 TEST_F(APIRequestHandlerTest, RequestThread) {
@@ -338,9 +336,9 @@ TEST_F(APIRequestHandlerTest, RequestThread) {
   };
 
   APIRequestHandler request_handler(
-      base::Bind(on_request, &thread),
+      base::BindRepeating(on_request, &thread),
       APILastError(APILastError::GetParent(), binding::AddConsoleError()),
-      nullptr);
+      nullptr, base::BindRepeating(&GetTestUserActivationState));
 
   request_handler.StartRequest(
       context, kMethod, std::make_unique<base::ListValue>(),
@@ -375,9 +373,9 @@ TEST_F(APIRequestHandlerTest, SettingLastError) {
 
   APIRequestHandler request_handler(
       base::DoNothing(),
-      APILastError(base::Bind(get_parent),
-                   base::Bind(log_error, &logged_error)),
-      nullptr);
+      APILastError(base::BindRepeating(get_parent),
+                   base::BindRepeating(log_error, &logged_error)),
+      nullptr, base::BindRepeating(&GetTestUserActivationState));
 
   const char kReportExposedLastError[] =
       "(function() {\n"
@@ -477,9 +475,9 @@ TEST_F(APIRequestHandlerTest, AddPendingRequest) {
   };
 
   APIRequestHandler request_handler(
-      base::Bind(handle_request, &dispatched_request),
+      base::BindRepeating(handle_request, &dispatched_request),
       APILastError(APILastError::GetParent(), binding::AddConsoleError()),
-      nullptr);
+      nullptr, base::BindRepeating(&GetTestUserActivationState));
 
   EXPECT_TRUE(request_handler.GetPendingRequestIdsForTesting().empty());
   v8::Local<v8::Function> function = FunctionFromString(context, kEchoArgs);
@@ -517,12 +515,12 @@ TEST_F(APIRequestHandlerTest, ThrowExceptionInCallback) {
 
   base::Optional<std::string> logged_error;
   ExceptionHandler exception_handler(
-      base::Bind(add_console_error, &logged_error));
+      base::BindRepeating(add_console_error, &logged_error));
 
   APIRequestHandler request_handler(
       base::DoNothing(),
       APILastError(APILastError::GetParent(), binding::AddConsoleError()),
-      &exception_handler);
+      &exception_handler, base::BindRepeating(&GetTestUserActivationState));
 
   v8::TryCatch outer_try_catch(isolate());
   v8::Local<v8::Function> callback_throwing_error =

extensions/renderer/bindings/declarative_event_unittest.cc

@@ -93,7 +93,7 @@ class DeclarativeEventTest : public APIBindingTest {
     request_handler_ = std::make_unique<APIRequestHandler>(
         base::Bind(&DeclarativeEventTest::OnRequest, base::Unretained(this)),
         APILastError(APILastError::GetParent(), binding::AddConsoleError()),
-        nullptr);
+        nullptr, base::BindRepeating(&GetTestUserActivationState));
   }
 
   void TearDown() override {

extensions/renderer/native_extension_bindings_system.cc

@@ -42,6 +42,7 @@
 #include "third_party/blink/public/mojom/service_worker/service_worker_registration.mojom.h"
 #include "third_party/blink/public/web/web_document.h"
 #include "third_party/blink/public/web/web_local_frame.h"
+#include "third_party/blink/public/web/web_user_gesture_indicator.h"
 
 namespace extensions {
 
@@ -342,15 +343,19 @@ NativeExtensionBindingsSystem::NativeExtensionBindingsSystem(
     std::unique_ptr<IPCMessageSender> ipc_message_sender)
     : ipc_message_sender_(std::move(ipc_message_sender)),
       api_system_(
-          base::Bind(&GetAPISchema),
-          base::Bind(&IsAPIFeatureAvailable),
-          base::Bind(&NativeExtensionBindingsSystem::SendRequest,
-                     base::Unretained(this)),
-          base::Bind(&NativeExtensionBindingsSystem::OnEventListenerChanged,
-                     base::Unretained(this)),
-          base::Bind(&GetContextOwner),
-          base::Bind(&APIActivityLogger::LogAPICall),
-          base::Bind(&AddConsoleError),
+          base::BindRepeating(&GetAPISchema),
+          base::BindRepeating(&IsAPIFeatureAvailable),
+          base::BindRepeating(&NativeExtensionBindingsSystem::SendRequest,
+                              base::Unretained(this)),
+          base::BindRepeating(
+              &NativeExtensionBindingsSystem::GetUserActivationState,
+              base::Unretained(this)),
+          base::BindRepeating(
+              &NativeExtensionBindingsSystem::OnEventListenerChanged,
+              base::Unretained(this)),
+          base::BindRepeating(&GetContextOwner),
+          base::BindRepeating(&APIActivityLogger::LogAPICall),
+          base::BindRepeating(&AddConsoleError),
           APILastError(base::Bind(&GetLastErrorParents),
                        base::Bind(&AddConsoleError))),
       messaging_service_(this),
@@ -758,6 +763,13 @@ void NativeExtensionBindingsSystem::SendRequest(
                                       request->thread);
 }
 
+bool NativeExtensionBindingsSystem::GetUserActivationState(
+    v8::Local<v8::Context> context) {
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
+  return blink::WebUserGestureIndicator::IsProcessingUserGestureThreadSafe(
+      script_context->web_frame());
+}
+
 void NativeExtensionBindingsSystem::OnEventListenerChanged(
     const std::string& event_name,
     binding::EventListenersChanged change,

extensions/renderer/native_extension_bindings_system.h

@@ -69,6 +69,9 @@ class NativeExtensionBindingsSystem : public ExtensionBindingsSystem {
   void SendRequest(std::unique_ptr<APIRequestHandler::Request> request,
                    v8::Local<v8::Context> context);
 
+  // Returns the transient user activation state for the |context|.
+  bool GetUserActivationState(v8::Local<v8::Context> context);
+
   // Called when listeners for a given event have changed, and forwards it along
   // to |send_event_listener_ipc_|.
   void OnEventListenerChanged(const std::string& event_name,