[Autofill Assistant] Cleanup some possibly unsafe pointers accesses.

This change replaces base::Unretained with a weak ptr everywhere where
there's possible a doubt that something could go wrong.

In particular, it uses weak ptrs for the try_done and all_done callbacks
of batch element checkers, since they can be called after the checker
has been deleted, even if it's unlikely.

This change also moves the script tracker in the Controller to make it
the last owned type to be deleted, as the script tracker will access
everything that exists in the controller, through actions. It's best to
delete it first.

Finally, this change adds null checks for ui_delegate_ in
android_ui_controller.cc, as the controller can delete itself separate
from any UI decisions. In this case, it sets ui_delegate_ to null.

Bug: 806868
Change-Id: Ie2e11ea54a1bdf80c4bc80d79a158a1658bc0d1b
Reviewed-on: https://chromium-review.googlesource.com/c/1353942
Commit-Queue: Stephane Zermatten <szermatt@chromium.org>
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612264}

chrome/browser/android/autofill_assistant/ui_controller_android.cc

@@ -102,6 +102,9 @@ UiControllerAndroid::~UiControllerAndroid() {}
 void UiControllerAndroid::Start(JNIEnv* env,
                                 const JavaParamRef<jobject>& jcaller,
                                 const JavaParamRef<jstring>& initialUrlString) {
+  if (!ui_delegate_)
+    return;
+
   GURL initialUrl =
       GURL(base::android::ConvertJavaStringToUTF8(env, initialUrlString));
   ui_delegate_->Start(initialUrl);
@@ -179,12 +182,18 @@ void UiControllerAndroid::ScrollBy(
     const base::android::JavaParamRef<jobject>& obj,
     float distanceX,
     float distanceY) {
+  if (!ui_delegate_)
+    return;
+
   ui_delegate_->ScrollBy(distanceX, distanceY);
 }
 
 void UiControllerAndroid::UpdateTouchableArea(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& obj) {
+  if (!ui_delegate_)
+    return;
+
   ui_delegate_->UpdateTouchableArea();
 }
 
@@ -192,6 +201,9 @@ void UiControllerAndroid::OnScriptSelected(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
     const JavaParamRef<jstring>& jscript_path) {
+  if (!ui_delegate_)
+    return;
+
   std::string script_path;
   base::android::ConvertJavaStringToUTF8(env, jscript_path, &script_path);
   ui_delegate_->OnScriptSelected(script_path);
@@ -447,6 +459,9 @@ void UiControllerAndroid::UpdateTouchableArea(bool enabled,
 }
 
 std::string UiControllerAndroid::GetDebugContext() const {
+  if (!ui_delegate_)
+    return "";
+
   return ui_delegate_->GetDebugContext();
 }
 

chrome/browser/android/autofill_assistant/ui_controller_android.h

@@ -132,6 +132,8 @@ class UiControllerAndroid : public UiController,
   base::android::ScopedJavaGlobalRef<jobject>
       java_autofill_assistant_ui_controller_;
 
+  // UI delegate. It can be nullptr during initialization and after the delegate
+  // has been deleted. Always check for nullptr before using it.
   UiDelegate* ui_delegate_;
   content::BrowserContext* browser_context_;
 

components/autofill_assistant/browser/batch_element_checker.h

@@ -93,6 +93,10 @@ class BatchElementChecker {
   // |try_done| or |all_done| can delete their calling BatchElementChecker to
   // interrupt any future checks. If |try_done| deletes BatchElementChecker,
   // |all_done| will never be called.
+  //
+  // Sync |try_done| and |all_done| can be called after the element checker has
+  // been deleted, they should always be called with callbacks that use a weak
+  // ptr instead of Unretained(this), even from objects that own the checker.
   void Run(const base::TimeDelta& duration,
            base::RepeatingCallback<void()> try_done,
            base::OnceCallback<void()> all_done);

components/autofill_assistant/browser/controller.cc

@@ -115,11 +115,11 @@ Controller::Controller(
       client_(std::move(client)),
       web_controller_(std::move(web_controller)),
       service_(std::move(service)),
-      script_tracker_(std::make_unique<ScriptTracker>(/* delegate= */ this,
-                                                      /* listener= */ this)),
       parameters_(std::move(parameters)),
       memory_(std::make_unique<ClientMemory>()),
       touchable_element_area_(web_controller_.get()),
+      script_tracker_(std::make_unique<ScriptTracker>(/* delegate= */ this,
+                                                      /* listener= */ this)),
       weak_ptr_factory_(this) {
   DCHECK(parameters_);
 

components/autofill_assistant/browser/controller.h

@@ -132,12 +132,11 @@ class Controller : public ScriptExecutorDelegate,
   std::unique_ptr<Client> client_;
   std::unique_ptr<WebController> web_controller_;
   std::unique_ptr<Service> service_;
-  std::unique_ptr<ScriptTracker> script_tracker_;
   std::unique_ptr<std::map<std::string, std::string>> parameters_;
+  std::unique_ptr<ClientMemory> memory_;
 
   // Domain of the last URL the controller requested scripts from.
   std::string script_domain_;
-  std::unique_ptr<ClientMemory> memory_;
   bool allow_autostart_ = true;
 
   // Whether a task for periodic checks is scheduled.
@@ -162,6 +161,11 @@ class Controller : public ScriptExecutorDelegate,
   // Flag indicates whether it is ready to fetch and execute scripts.
   bool started_ = false;
 
+  // Tracks scripts and script execution. It's kept at the end, as it tend to
+  // depend on everything the controller support, through script and script
+  // actions.
+  std::unique_ptr<ScriptTracker> script_tracker_;
+
   base::WeakPtrFactory<Controller> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(Controller);

components/autofill_assistant/browser/script_executor.cc

@@ -100,7 +100,7 @@ void ScriptExecutor::WaitForElementVisible(
   wait_with_interrupts_ = std::make_unique<WaitWithInterrupts>(
       this, max_wait_time, kVisibilityCheck, selector,
       base::BindOnce(&ScriptExecutor::OnWaitForElementVisible,
-                     base::Unretained(this), std::move(callback)));
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   wait_with_interrupts_->Run();
 }
 
@@ -479,7 +479,8 @@ ScriptExecutor::WaitWithInterrupts::WaitWithInterrupts(
       check_type_(check_type),
       selector_(selector),
       callback_(std::move(callback)),
-      element_found_(false) {}
+      element_found_(false),
+      weak_ptr_factory_(this) {}
 
 ScriptExecutor::WaitWithInterrupts::~WaitWithInterrupts() = default;
 
@@ -500,17 +501,18 @@ void ScriptExecutor::WaitWithInterrupts::Run() {
         batch_element_checker_.get(), main_script_->delegate_->GetParameters(),
         *main_script_->scripts_state_,
         base::BindOnce(&WaitWithInterrupts::OnPreconditionCheckDone,
-                       base::Unretained(this), base::Unretained(interrupt)));
+                       weak_ptr_factory_.GetWeakPtr(),
+                       base::Unretained(interrupt)));
   }
+  // The base::Unretained(this) above are safe, since the pointers belong to the
+  // main script, which own this instance.
 
   batch_element_checker_->Run(
       max_wait_time_,
       base::BindRepeating(&WaitWithInterrupts::OnTryDone,
-                          base::Unretained(this)),
-      base::BindOnce(&WaitWithInterrupts::OnAllDone, base::Unretained(this)));
-
-  // base::Unretained(this) above is safe because batch_element_checker_ belongs
-  // to this.
+                          weak_ptr_factory_.GetWeakPtr()),
+      base::BindOnce(&WaitWithInterrupts::OnAllDone,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ScriptExecutor::WaitWithInterrupts::OnServerPayloadChanged(

components/autofill_assistant/browser/script_executor.h

@@ -230,6 +230,8 @@ class ScriptExecutor : public ActionDelegate {
     // The status message that was displayed when the interrupt started.
     std::string pre_interrupt_status_;
 
+    base::WeakPtrFactory<WaitWithInterrupts> weak_ptr_factory_;
+
     DISALLOW_COPY_AND_ASSIGN(WaitWithInterrupts);
   };
   friend class WaitWithInterrupts;