[Autofill Assistant] Fix crash when lite script ends.

The crash seems to happen because the delegate is deleted potentially
while a call to Java is still pending. I assume (but am not sure) that
JNI calls are put into the message queue and not executed immediately.

The workaround for now is to use callbacks instead of a dedicated
delegate, thereby circumventing lifecycle issues of the delegate.

Bug: b/169129405
Change-Id: I6e34874141792df47a117cd1f7f045759f852254
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2423885
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Sandro Maggi <sandromaggi@google.com>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809357}

chrome/browser/android/autofill_assistant/lite_service_bridge.cc

@@ -14,26 +14,18 @@
 
 namespace autofill_assistant {
 
-class LiteServiceJavaDelegate : public LiteService::Delegate {
- public:
-  LiteServiceJavaDelegate(
-      const base::android::JavaParamRef<jobject>& java_lite_service)
-      : java_lite_service_(java_lite_service) {}
-  void OnUiShown() const override {
-    JNIEnv* env = base::android::AttachCurrentThread();
-    Java_AutofillAssistantLiteService_onUiShown(env, java_lite_service_);
-  }
-
-  void OnFinished(Metrics::LiteScriptFinishedState state) const override {
-    VLOG(1) << "Lite service finished with state " << static_cast<int>(state);
-    JNIEnv* env = base::android::AttachCurrentThread();
-    Java_AutofillAssistantLiteService_onFinished(env, java_lite_service_,
-                                                 static_cast<int>(state));
-  }
+void OnFinished(base::android::ScopedJavaGlobalRef<jobject> java_lite_service,
+                Metrics::LiteScriptFinishedState state) {
+  VLOG(1) << "Lite service finished with state " << static_cast<int>(state);
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_AutofillAssistantLiteService_onFinished(env, java_lite_service,
+                                               static_cast<int>(state));
+}
 
- private:
-  base::android::ScopedJavaGlobalRef<jobject> java_lite_service_;
-};
+void OnUiShown(base::android::ScopedJavaGlobalRef<jobject> java_lite_service) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_AutofillAssistantLiteService_onUiShown(env, java_lite_service);
+}
 
 // static
 jlong JNI_AutofillAssistantLiteService_CreateLiteService(
@@ -57,7 +49,10 @@ jlong JNI_AutofillAssistantLiteService_CreateLiteService(
                                     /* access_token_fetcher = */ nullptr,
                                     /* auth_enabled = */ false),
       base::android::ConvertJavaStringToUTF8(env, jtrigger_script_path),
-      std::make_unique<LiteServiceJavaDelegate>(java_lite_service)));
+      base::BindOnce(&OnFinished, base::android::ScopedJavaGlobalRef<jobject>(
+                                      java_lite_service)),
+      base::BindOnce(&OnUiShown, base::android::ScopedJavaGlobalRef<jobject>(
+                                     java_lite_service))));
 }
 
 }  // namespace autofill_assistant

components/autofill_assistant/browser/lite_service.cc

@@ -15,18 +15,21 @@
 
 namespace autofill_assistant {
 
-LiteService::LiteService(std::unique_ptr<Service> service_impl,
-                         const std::string& trigger_script_path,
-                         std::unique_ptr<Delegate> delegate)
+LiteService::LiteService(
+    std::unique_ptr<Service> service_impl,
+    const std::string& trigger_script_path,
+    base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
+        notify_finished_callback,
+    base::OnceCallback<void()> notify_ui_shown_callback)
     : service_impl_(std::move(service_impl)),
       trigger_script_path_(trigger_script_path),
-      delegate_(std::move(delegate)) {}
+      notify_finished_callback_(std::move(notify_finished_callback)),
+      notify_ui_shown_callback_(std::move(notify_ui_shown_callback)) {}
 
 LiteService::~LiteService() {
-  if (delegate_) {
-    delegate_->OnFinished(
-        Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED);
-    delegate_.reset();
+  if (notify_finished_callback_) {
+    std::move(notify_finished_callback_)
+        .Run(Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED);
   }
 }
 
@@ -136,7 +139,7 @@ void LiteService::GetNextActions(
     const std::string& previous_script_payload,
     const std::vector<ProcessedActionProto>& processed_actions,
     ResponseCallback callback) {
-  if (!delegate_) {
+  if (!notify_finished_callback_) {
     // The lite script has already terminated. We need to run |callback| with
     // |success|=true and an empty response to ensure a graceful stop of the
     // script (i.e., without error message).
@@ -174,7 +177,9 @@ void LiteService::GetNextActions(
         trigger_script_second_part_->SerializeToString(&serialized_second_part);
         trigger_script_second_part_.reset();
         std::move(callback).Run(true, serialized_second_part);
-        delegate_->OnUiShown();
+        if (notify_ui_shown_callback_) {
+          std::move(notify_ui_shown_callback_).Run();
+        }
         return;
     }
   } else {
@@ -219,9 +224,8 @@ void LiteService::StopWithoutErrorMessage(
     Metrics::LiteScriptFinishedState state) {
   // Notify delegate BEFORE terminating the controller. See comment in header
   // for |OnFinished|.
-  if (delegate_) {
-    delegate_->OnFinished(state);
-    delegate_.reset();
+  if (notify_finished_callback_) {
+    std::move(notify_finished_callback_).Run(state);
   }
 
   // Stop script.

components/autofill_assistant/browser/lite_service.h

@@ -24,32 +24,12 @@ namespace autofill_assistant {
 // other information.
 class LiteService : public Service {
  public:
-  class Delegate {
-   public:
-    virtual ~Delegate() {}
-    // Called the first time the UI is shown to the user.
-    virtual void OnUiShown() const = 0;
-
-    // The lite script has finished with |state|.
-    //
-    // Note that this notification will be run BEFORE the controller shuts down.
-    // This is necessary to transition between old and new bottom sheet
-    // contents.
-    // Case 1: onboarding will be shown. The controller will terminate
-    // gracefully with an explicit stop action executed after the notification
-    // was run.
-    // Case 2: onboarding will not be shown. The controller must terminate
-    // immediately and make way for the main script controller. Since the
-    // notification is run while the old controller is still around, the caller
-    // can hot-swap controllers and smoothly transition bottom sheet contents.
-    // Case 3: the lite script failed (|state| != LITE_SCRIPT_SUCCESS). The
-    // controller will terminate gracefully with an explicit stop action.
-    virtual void OnFinished(Metrics::LiteScriptFinishedState state) const = 0;
-  };
-
-  explicit LiteService(std::unique_ptr<Service> service_impl,
-                       const std::string& trigger_script_path,
-                       std::unique_ptr<Delegate> delegate);
+  explicit LiteService(
+      std::unique_ptr<Service> service_impl,
+      const std::string& trigger_script_path,
+      base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
+          notify_finished_callback,
+      base::OnceCallback<void()> notify_ui_shown_callback);
   // If the destructor is called before |GetNextActions|, the script was
   // terminated before finishing (user cancelled, closed the tab, etc.).
   ~LiteService() override;
@@ -101,8 +81,23 @@ class LiteService : public Service {
   std::unique_ptr<Service> service_impl_;
   // The script path to fetch actions from.
   std::string trigger_script_path_;
-  // The delegate to notify of changes.
-  std::unique_ptr<Delegate> delegate_;
+
+  // Notifies the java bridge of the finished state.
+  //
+  // Note that this callback will be run BEFORE the controller shuts down. This
+  // is necessary to transition between old and new bottom sheet contents.
+  // Case 1: onboarding will be shown. The controller will terminate gracefully
+  // with an explicit stop action executed after the callback was run.
+  // Case 2: onboarding will not be shown. The controller must terminate
+  // immediately and make way for the main script controller. Since the callback
+  // is run while the old controller is still around, the caller can hot-swap
+  // controllers and smoothly transition bottom sheet contents.
+  // Case 3: the lite script failed (|state| != LITE_SCRIPT_SUCCESS). The
+  // controller will terminate gracefully with an explicit stop action.
+  base::OnceCallback<void(Metrics::LiteScriptFinishedState)>
+      notify_finished_callback_;
+  // Notifies the java bridge that the UI was shown for the first time.
+  base::OnceCallback<void(void)> notify_ui_shown_callback_;
 
   // The second part of the trigger script, i.e., the actions that should be run
   // after a successful prompt(browse) action in the first part of the script.

components/autofill_assistant/browser/lite_service_unittest.cc

@@ -32,24 +32,15 @@ using ::testing::_;
 using ::testing::AtMost;
 using ::testing::Eq;
 
-class MockDelegate : public LiteService::Delegate {
- public:
-  ~MockDelegate() {}
-  MOCK_CONST_METHOD0(OnUiShown, void());
-  MOCK_CONST_METHOD1(OnFinished, void(Metrics::LiteScriptFinishedState state));
-};
-
 class LiteServiceTest : public testing::Test {
  protected:
   LiteServiceTest() {
     auto service_impl = std::make_unique<MockService>();
     mock_native_service_ = service_impl.get();
 
-    auto delegate = std::make_unique<MockDelegate>();
-    mock_delegate_ = delegate.get();
-
     lite_service_ = std::make_unique<LiteService>(
-        std::move(service_impl), kFakeScriptPath, std::move(delegate));
+        std::move(service_impl), kFakeScriptPath, mock_finished_callback_.Get(),
+        mock_ui_shown_callback_.Get());
     EXPECT_CALL(*mock_native_service_, OnGetScriptsForUrl).Times(0);
     EXPECT_CALL(*mock_native_service_, OnGetNextActions).Times(0);
 
@@ -82,27 +73,30 @@ class LiteServiceTest : public testing::Test {
     std::string serialized_stop;
     stop.SerializeToString(&serialized_stop);
     EXPECT_CALL(mock_response_callback_, Run(true, serialized_stop)).Times(1);
-    EXPECT_CALL(*mock_delegate_, OnFinished(state));
+    EXPECT_CALL(mock_finished_callback_, Run(state));
   }
 
+  base::MockCallback<base::OnceCallback<void(Metrics::LiteScriptFinishedState)>>
+      mock_finished_callback_;
+  base::MockCallback<base::OnceCallback<void(void)>> mock_ui_shown_callback_;
   base::MockCallback<base::OnceCallback<void(bool, const std::string&)>>
       mock_response_callback_;
   MockService* mock_native_service_ = nullptr;
-  MockDelegate* mock_delegate_ = nullptr;
   std::unique_ptr<LiteService> lite_service_;
   ActionsResponseProto get_actions_response_;
 };
 
 TEST_F(LiteServiceTest, RunsNotificationOnDelete) {
-  auto delegate = std::make_unique<MockDelegate>();
-  auto* delegate_ptr = delegate.get();
+  base::MockCallback<base::OnceCallback<void(Metrics::LiteScriptFinishedState)>>
+      notification_callback;
+  base::MockCallback<base::OnceCallback<void()>> ui_shown_callback;
   EXPECT_CALL(
-      *delegate_ptr,
-      OnFinished(
-          Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED));
+      notification_callback,
+      Run(Metrics::LiteScriptFinishedState::LITE_SCRIPT_SERVICE_DELETED));
   {
     LiteService lite_service(std::make_unique<MockService>(), kFakeScriptPath,
-                             std::move(delegate));
+                             notification_callback.Get(),
+                             ui_shown_callback.Get());
   }
 }
 
@@ -169,7 +163,8 @@ TEST_F(LiteServiceTest, StopsOnGetActionsFailed) {
                     Service::ResponseCallback& callback) {
         std::move(callback).Run(false, std::string());
       });
-  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(0);
+
+  EXPECT_CALL(mock_ui_shown_callback_, Run).Times(0);
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
                             TriggerContextImpl(), "", "",
                             mock_response_callback_.Get());
@@ -188,7 +183,7 @@ TEST_F(LiteServiceTest, StopsOnGetActionsParsingError) {
                     Service::ResponseCallback& callback) {
         std::move(callback).Run(true, std::string("invalid proto"));
       });
-  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(0);
+  EXPECT_CALL(mock_ui_shown_callback_, Run).Times(0);
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
                             TriggerContextImpl(), "", "",
                             mock_response_callback_.Get());
@@ -200,7 +195,7 @@ TEST_F(LiteServiceTest, StopsOnGetActionsContainsUnsafeActions) {
   get_actions_response_.add_actions()->mutable_prompt();
   ExpectStopWithFinishedState(
       Metrics::LiteScriptFinishedState::LITE_SCRIPT_UNSAFE_ACTIONS);
-  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(0);
+  EXPECT_CALL(mock_ui_shown_callback_, Run).Times(0);
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
                             TriggerContextImpl(), "", "",
                             mock_response_callback_.Get());
@@ -266,7 +261,7 @@ TEST_F(LiteServiceTest, GetActionsSplitsActionsResponseAtLastBrowse) {
         processed_actions.back().mutable_prompt_choice()->set_server_payload(
             proto.actions(3).prompt().choices(0).server_payload());
       });
-  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(1);
+  EXPECT_CALL(mock_ui_shown_callback_, Run).Times(1);
   lite_service_->GetActions(kFakeScriptPath, GURL(kFakeUrl),
                             TriggerContextImpl(), "", "",
                             mock_response_callback_.Get());
@@ -295,7 +290,7 @@ TEST_F(LiteServiceTest, GetNextActionsFirstPartStopsOnUserNavigateAway) {
 
   ExpectStopWithFinishedState(
       Metrics::LiteScriptFinishedState::LITE_SCRIPT_BROWSE_FAILED_NAVIGATE);
-  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(0);
+  EXPECT_CALL(mock_ui_shown_callback_, Run).Times(0);
   lite_service_->GetNextActions(TriggerContextImpl(), "", "", processed_actions,
                                 mock_response_callback_.Get());
 }
@@ -314,7 +309,7 @@ TEST_F(LiteServiceTest, GetNextActionsFirstPartSucceedsOnAutoSelectChoice) {
       "payload");
 
   EXPECT_CALL(mock_response_callback_, Run(true, ""));
-  EXPECT_CALL(*mock_delegate_, OnUiShown).Times(1);
+  EXPECT_CALL(mock_ui_shown_callback_, Run).Times(1);
   lite_service_->GetNextActions(TriggerContextImpl(), "", "", processed_actions,
                                 mock_response_callback_.Get());
 }