Hide modal permission prompts on Android upon tab navigation/destruction

chrome/browser/permissions/permission_dialog_delegate.cc

Index: chrome/browser/permissions/permission_dialog_delegate.cc
diff --git a/chrome/browser/permissions/permission_dialog_delegate.cc b/chrome/browser/permissions/permission_dialog_delegate.cc
index 1c1e81cea63b23d579cc1cb2f3d139807811fdd1..8b7fe9b25216d87d1571e5c9559e67f0830cfab2 100644
--- a/chrome/browser/permissions/permission_dialog_delegate.cc
+++ b/chrome/browser/permissions/permission_dialog_delegate.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_features.h"
 #include "components/variations/variations_associated_data.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "jni/PermissionDialogController_jni.h"
 #include "jni/PermissionDialogDelegate_jni.h"
@@ -102,14 +103,12 @@ bool PermissionDialogDelegate::RegisterPermissionDialogDelegate(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
-ScopedJavaLocalRef<jobject> PermissionDialogDelegate::CreateJavaDelegate(
-    JNIEnv* env) {
+void PermissionDialogDelegate::CreateJavaDelegate(JNIEnv* env) {
   std::vector<int> content_settings_types{
       infobar_delegate_->content_settings_types()};
 
-  return Java_PermissionDialogDelegate_create(
-      env, reinterpret_cast<uintptr_t>(this),
-      tab_->GetJavaObject(),
+  j_delegate_.Reset(Java_PermissionDialogDelegate_create(
+      env, reinterpret_cast<uintptr_t>(this), tab_->GetJavaObject(),
       base::android::ToJavaIntArray(env, content_settings_types).obj(),
       ResourceMapper::MapFromChromiumId(infobar_delegate_->GetIconId()),
       ConvertUTF16ToJavaString(env, infobar_delegate_->GetMessageText()),
@@ -119,7 +118,7 @@ ScopedJavaLocalRef<jobject> PermissionDialogDelegate::CreateJavaDelegate(
       ConvertUTF16ToJavaString(env,
                                infobar_delegate_->GetButtonLabel(
                                    PermissionInfoBarDelegate::BUTTON_CANCEL)),
-      infobar_delegate_->ShouldShowPersistenceToggle());
+      infobar_delegate_->ShouldShowPersistenceToggle()));
 }
 
 void PermissionDialogDelegate::Accept(JNIEnv* env,
@@ -158,25 +157,49 @@ void PermissionDialogDelegate::LinkClicked(JNIEnv* env,
 
 void PermissionDialogDelegate::Destroy(JNIEnv* env,
                                        const JavaParamRef<jobject>& obj) {
+  j_delegate_ = nullptr;

[Bernhard Bauer, 2017/05/25 09:57:17]

This isn't really necessary if the object is going to be destroyed anyway.

[Timothy Loh, 2017/05/26 05:08:03]

Err yeah, removed.

   delete this;
 }
 
 PermissionDialogDelegate::PermissionDialogDelegate(
     TabAndroid* tab,
     std::unique_ptr<PermissionInfoBarDelegate> infobar_delegate)
-    : tab_(tab), infobar_delegate_(std::move(infobar_delegate)) {
+    : content::WebContentsObserver(tab->web_contents()),
+      tab_(tab),
+      infobar_delegate_(std::move(infobar_delegate)) {
   DCHECK(tab_);
   DCHECK(infobar_delegate_);
 
   // Create our Java counterpart, which manages our lifetime.
   JNIEnv* env = base::android::AttachCurrentThread();
-  base::android::ScopedJavaLocalRef<jobject> j_delegate =
-      CreateJavaDelegate(env);
+  CreateJavaDelegate(env);
 
   // Send the Java delegate to the Java PermissionDialogController for display.
   // The controller takes over lifetime management; when the Java delegate is no
   // longer needed it will in turn free the native delegate.
-  Java_PermissionDialogController_createDialog(env, j_delegate.obj());
+  Java_PermissionDialogController_createDialog(env, j_delegate_.obj());
 }
 
 PermissionDialogDelegate::~PermissionDialogDelegate() {}
+
+void PermissionDialogDelegate::DismissDialog() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_PermissionDialogDelegate_dismissFromNative(env, j_delegate_.obj());
+}
+
+void PermissionDialogDelegate::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame() ||
+      !navigation_handle->HasCommitted() ||
+      navigation_handle->IsSameDocument()) {
+    return;
+  }
+
+  DismissDialog();
+  delete this;

[Bernhard Bauer, 2017/05/25 09:57:17]

I have to say, I don't like these deletions very much either. For me it's not
even that much potential UAF, but more that it feels weird for this class to
both be destroyed by the Java object[*], but also destroy itself under certain
circumstances. If you think about it in terms of ownership, who owns this
object?

Would it make sense for PermissionDialogController#dismissFromNative to just
call PermissionDialogDelegate#destroy? That way you would have a clean ownership
story and not need additional code paths to destroy this object.

[*] I'm regarding the Destroy() method here as external destruction and the
WebContentsObserver methods as self-destruction even though both internally call
`delete this`, because a method called Destroy() pretty much implies deletion in
its contract; it's just that we have to use this syntax because of JNI.

[Timothy Loh, 2017/05/26 05:08:03]

I considered it but figured it would be nicer this way because we don't have to
go C++ -> Java -> C++. I haven't done much C++ <--> Java stuff in the past
though, so if you think it's probably better to go about it that way I don't
mind. I changed it in the latest patch set.

[Bernhard Bauer, 2017/05/26 09:01:16]

Thanks! I do think this is nicer. If you want to, you could add a comment that
DismissDialog() will indirectly end up destroying the object, so the object
shouldn't be accessed afterwards.

+}
+
+void PermissionDialogDelegate::WebContentsDestroyed() {
+  DismissDialog();
+  delete this;
+}