Hide modal permission prompts on Android upon tab navigation/destruction

chrome/browser/permissions/permission_dialog_delegate.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/permissions/permission_dialog_delegate.h"
 
 #include <utility>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/feature_list.h"
 #include "build/build_config.h"
 #include "chrome/browser/android/resource_mapper.h"
 #include "chrome/browser/android/tab_android.h"
 #include "chrome/browser/geolocation/geolocation_infobar_delegate_android.h"
 #include "chrome/browser/media/midi_permission_infobar_delegate_android.h"
 #include "chrome/browser/media/protected_media_identifier_infobar_delegate_android.h"
 #include "chrome/browser/media/webrtc/media_stream_infobar_delegate_android.h"
 #include "chrome/browser/notifications/notification_permission_infobar_delegate.h"
 #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"
 #include "ui/android/window_android.h"
 #include "ui/base/window_open_disposition.h"
 
 using base::android::ConvertUTF16ToJavaString;
 
 namespace {
 
@@ skipping 61 matching lines @@
   if (require_gesture == "false")
     return true;
   return has_user_gesture;
 }
 
 // static
 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()),
       ConvertUTF16ToJavaString(env, infobar_delegate_->GetLinkText()),
       ConvertUTF16ToJavaString(env, infobar_delegate_->GetButtonLabel(
                                         PermissionInfoBarDelegate::BUTTON_OK)),
       ConvertUTF16ToJavaString(env,
                                infobar_delegate_->GetButtonLabel(
                                    PermissionInfoBarDelegate::BUTTON_CANCEL)),
-      infobar_delegate_->ShouldShowPersistenceToggle());
+      infobar_delegate_->ShouldShowPersistenceToggle()));
 }
 
 void PermissionDialogDelegate::Accept(JNIEnv* env,
                                       const JavaParamRef<jobject>& obj,
                                       jboolean persist) {
   if (infobar_delegate_->ShouldShowPersistenceToggle())
     infobar_delegate_->set_persist(persist);
   infobar_delegate_->Accept();
 }
 
@@ skipping 18 matching lines @@
   if (tab_->web_contents()) {
     tab_->web_contents()->OpenURL(content::OpenURLParams(
         infobar_delegate_->GetLinkURL(), content::Referrer(),
         WindowOpenDisposition::NEW_FOREGROUND_TAB, ui::PAGE_TRANSITION_LINK,
         false));
   }
 }
 
 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;
+}