Factor out ContentViewAndroidDelegate

chrome/browser/ui/android/autofill/autofill_popup_view_android.cc

Index: chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
diff --git a/chrome/browser/ui/android/autofill/autofill_popup_view_android.cc b/chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
index 663bd8b4192cd476f878eb1a6532e5e2d65692fb..82a56a02ead7d2985ba2b09c1b8eaf26d5f19b46 100644
--- a/chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
+++ b/chrome/browser/ui/android/autofill/autofill_popup_view_android.cc
@@ -36,11 +36,14 @@ void AutofillPopupViewAndroid::Show() {
   ui::ViewAndroid* view_android = controller_->container_view();
 
   DCHECK(view_android);
+  popup_view_ = view_android->AcquireAnchorView();
+  if (popup_view_.is_null())
+    return;
 
   java_object_.Reset(Java_AutofillPopupBridge_create(
-      env, reinterpret_cast<intptr_t>(this),
-      view_android->GetWindowAndroid()->GetJavaObject().obj(),
-      view_android->GetViewAndroidDelegate().obj()));
+      env, popup_view_.view().obj(), controller_->element_bounds().width(),
+      reinterpret_cast<intptr_t>(this),
+      view_android->GetWindowAndroid()->GetJavaObject().obj()));
 
   UpdateBoundsAndRedrawPopup();
 }
@@ -48,19 +51,24 @@ void AutofillPopupViewAndroid::Show() {
 void AutofillPopupViewAndroid::Hide() {
   controller_ = NULL;
   JNIEnv* env = base::android::AttachCurrentThread();
-  Java_AutofillPopupBridge_dismiss(env, java_object_.obj());
+  if (!java_object_.is_null()) {
+    Java_AutofillPopupBridge_dismiss(env, java_object_.obj());
+  } else {
+    PopupDismissedInternal();

[no sievers, 2016/07/20 18:46:04]

Yes, good catch to not leak |this| here.

But now that you point that out, I wonder if there is a bug in the preexisting
code here - and similarly for PasswordGenerationPopup. There are if-checks in
the Java dismiss/hide functions called here and I'm worried that there are
corner cases where |this| still leaks here on the native side. wdyt? maybe worth
filing a separate bug.
It might be more robust to always delete |this| here directly (and making sure
that at that point either the Java objects are gone or at least cannot call back
into |this| and cause use-after-free anymore).

nit: Since we reset |controller| I'd consider just calling 'delete this' here
directly. It makes it a bit more obvious since that has to be the last thing we
do in this function. And if you call another function that deletes |this| it's a
bit risky as an unnoticed side effect. So I'd put at least a comment. Maybe just
mention in general that either way Hide() has to delete |this| - whether it's
through the Java dismiss path or directly here.

[Jinsuk Kim, 2016/07/20 23:19:03]

Acknowledged. Called |delete this| directly and added a comment.

+  }
 }
 
 void AutofillPopupViewAndroid::UpdateBoundsAndRedrawPopup() {
-  JNIEnv* env = base::android::AttachCurrentThread();
-  Java_AutofillPopupBridge_setAnchorRect(
-      env,
-      java_object_.obj(),
-      controller_->element_bounds().x(),
-      controller_->element_bounds().y(),
-      controller_->element_bounds().width(),
-      controller_->element_bounds().height());
+  if (java_object_.is_null() || popup_view_.is_null())
+    return;
+
+  ui::ViewAndroid* view_android = controller_->container_view();
+
+  DCHECK(view_android);
+  view_android->SetAnchorRect(popup_view_.view(),
+                              controller_->element_bounds());
 
+  JNIEnv* env = base::android::AttachCurrentThread();
   size_t count = controller_->GetLineCount();
   ScopedJavaLocalRef<jobjectArray> data_array =
       Java_AutofillPopupBridge_createAutofillSuggestionArray(env, count);
@@ -105,7 +113,7 @@ void AutofillPopupViewAndroid::DeletionRequested(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     jint list_index) {
-  if (!controller_)
+  if (!controller_ || java_object_.is_null())
     return;
 
   base::string16 confirmation_title, confirmation_body;
@@ -137,6 +145,10 @@ void AutofillPopupViewAndroid::DeletionConfirmed(
 void AutofillPopupViewAndroid::PopupDismissed(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
+  PopupDismissedInternal();
+}
+
+void AutofillPopupViewAndroid::PopupDismissedInternal() {
   if (controller_)
     controller_->ViewDestroyed();