[Context menu] Move RenderFrameHost reference from native to Java

This CL updates the ContextMenuNativeDelegate bridge to hold a reference
on the Java side and get a C++ pointer when it will be used. This is
done to prevent a use-after-free.

Bug: 1150622
Change-Id: Iaf22d7b6ba37923ec98d7e2f7c497dc45b5ce474
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2578127
Commit-Queue: Sinan Sahin <sinansahin@google.com>
Reviewed-by: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Reviewed-by: Theresa  <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834961}

chrome/browser/android/context_menu/context_menu_native_delegate_impl.cc

@@ -96,11 +96,8 @@ void OnRetrieveImageForContextMenu(
 
 ContextMenuNativeDelegateImpl::ContextMenuNativeDelegateImpl(
     content::WebContents* const web_contents,
-    content::ContextMenuParams* const context_menu_params,
-    content::RenderFrameHost* const render_frame_host)
-    : web_contents_(web_contents),
-      context_menu_params_(context_menu_params),
-      render_frame_host_(render_frame_host) {}
+    content::ContextMenuParams* const context_menu_params)
+    : web_contents_(web_contents), context_menu_params_(context_menu_params) {}
 
 void ContextMenuNativeDelegateImpl::StartDownload(
     JNIEnv* env,
@@ -113,50 +110,57 @@ void ContextMenuNativeDelegateImpl::StartDownload(
 
 void ContextMenuNativeDelegateImpl::SearchForImage(
     JNIEnv* env,
-    const JavaParamRef<jobject>& obj) {
-  if (!render_frame_host_)
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& jrender_frame_host) {
+  auto* render_frame_host =
+      content::RenderFrameHost::FromJavaRenderFrameHost(jrender_frame_host);
+  if (!render_frame_host)
     return;
 
   CoreTabHelper::FromWebContents(web_contents_)
-      ->SearchByImageInNewTab(render_frame_host_,
-                              context_menu_params_->src_url);
+      ->SearchByImageInNewTab(render_frame_host, context_menu_params_->src_url);
 }
 
 void ContextMenuNativeDelegateImpl::RetrieveImageForShare(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& jrender_frame_host,
     const JavaParamRef<jobject>& jcallback,
     jint max_width_px,
     jint max_height_px,
     jint jimage_format) {
   RetrieveImageInternal(env, base::BindOnce(&OnRetrieveImageForShare),
-                        jcallback, max_width_px, max_height_px,
-                        ToChromeMojomImageFormat(jimage_format));
+                        jrender_frame_host, jcallback, max_width_px,
+                        max_height_px, ToChromeMojomImageFormat(jimage_format));
 }
 
 void ContextMenuNativeDelegateImpl::RetrieveImageForContextMenu(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& jrender_frame_host,
     const JavaParamRef<jobject>& jcallback,
     jint max_width_px,
     jint max_height_px) {
   // For context menu, Image needs to be PNG for receiving transparency pixels.
   RetrieveImageInternal(env, base::BindOnce(&OnRetrieveImageForContextMenu),
-                        jcallback, max_width_px, max_height_px,
-                        chrome::mojom::ImageFormat::PNG);
+                        jrender_frame_host, jcallback, max_width_px,
+                        max_height_px, chrome::mojom::ImageFormat::PNG);
 }
 
 void ContextMenuNativeDelegateImpl::RetrieveImageInternal(
     JNIEnv* env,
     ImageRetrieveCallback retrieve_callback,
+    const JavaParamRef<jobject>& jrender_frame_host,
     const JavaParamRef<jobject>& jcallback,
     jint max_width_px,
     jint max_height_px,
     chrome::mojom::ImageFormat image_format) {
-  if (!render_frame_host_)
+  auto* render_frame_host =
+      content::RenderFrameHost::FromJavaRenderFrameHost(jrender_frame_host);
+  if (!render_frame_host)
     return;
   mojo::AssociatedRemote<chrome::mojom::ChromeRenderFrame> chrome_render_frame;
-  render_frame_host_->GetRemoteAssociatedInterfaces()->GetInterface(
+  render_frame_host->GetRemoteAssociatedInterfaces()->GetInterface(
       &chrome_render_frame);
 
   // Bind the InterfacePtr into the callback so that it's kept alive
@@ -173,16 +177,13 @@ void ContextMenuNativeDelegateImpl::RetrieveImageInternal(
 static jlong JNI_ContextMenuNativeDelegateImpl_Init(
     JNIEnv* env,
     const JavaParamRef<jobject>& jweb_contents,
-    const JavaParamRef<jobject>& jcontext_menu_params,
-    const JavaParamRef<jobject>& jrender_frame_host) {
+    const JavaParamRef<jobject>& jcontext_menu_params) {
   if (jweb_contents.is_null())
     return reinterpret_cast<intptr_t>(nullptr);
   auto* web_contents = content::WebContents::FromJavaWebContents(jweb_contents);
   DCHECK(web_contents);
   auto* params =
       context_menu::ContextMenuParamsFromJavaObject(jcontext_menu_params);
-  auto* render_frame_host =
-      content::RenderFrameHost::FromJavaRenderFrameHost(jrender_frame_host);
-  return reinterpret_cast<intptr_t>(new ContextMenuNativeDelegateImpl(
-      web_contents, params, render_frame_host));
+  return reinterpret_cast<intptr_t>(
+      new ContextMenuNativeDelegateImpl(web_contents, params));
 }

chrome/browser/android/context_menu/context_menu_native_delegate_impl.h

@@ -25,18 +25,19 @@ class ContextMenuNativeDelegateImpl {
  public:
   explicit ContextMenuNativeDelegateImpl(
       content::WebContents* const web_contents,
-      content::ContextMenuParams* const context_menu_params,
-      content::RenderFrameHost* const render_frame_host);
+      content::ContextMenuParams* const context_menu_params);
 
   void RetrieveImageForContextMenu(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jobject>& jrender_frame_host,
       const base::android::JavaParamRef<jobject>& jcallback,
       jint max_width_px,
       jint max_height_px);
   void RetrieveImageForShare(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jobject>& jrender_frame_host,
       const base::android::JavaParamRef<jobject>& jcallback,
       jint max_width_px,
       jint max_height_px,
@@ -44,8 +45,10 @@ class ContextMenuNativeDelegateImpl {
   void StartDownload(JNIEnv* env,
                      const base::android::JavaParamRef<jobject>& obj,
                      jboolean jis_link);
-  void SearchForImage(JNIEnv* env,
-                      const base::android::JavaParamRef<jobject>& obj);
+  void SearchForImage(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      const base::android::JavaParamRef<jobject>& jrender_frame_host);
 
  protected:
   using ImageRetrieveCallback = base::OnceCallback<void(
@@ -60,6 +63,7 @@ class ContextMenuNativeDelegateImpl {
   void RetrieveImageInternal(
       JNIEnv* env,
       ImageRetrieveCallback retrieve_callback,
+      const base::android::JavaParamRef<jobject>& jrender_frame_host,
       const base::android::JavaParamRef<jobject>& jcallback,
       jint max_width_px,
       jint max_height_px,
@@ -67,7 +71,6 @@ class ContextMenuNativeDelegateImpl {
 
   content::WebContents* const web_contents_;
   content::ContextMenuParams* const context_menu_params_;
-  content::RenderFrameHost* const render_frame_host_;
 };
 
 #endif  // CHROME_BROWSER_ANDROID_CONTEXT_MENU_CONTEXT_MENU_NATIVE_DELEGATE_IMPL_H_

chrome/browser/contextmenu/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuNativeDelegateImpl.java

@@ -20,6 +20,8 @@ import org.chromium.content_public.browser.WebContents;
 class ContextMenuNativeDelegateImpl implements ContextMenuNativeDelegate {
     private static final int MAX_SHARE_DIMEN_PX = 2048;
 
+    private final RenderFrameHost mRenderFrameHost;
+
     private long mNativePtr;
 
     /**
@@ -45,8 +47,8 @@ class ContextMenuNativeDelegateImpl implements ContextMenuNativeDelegate {
 
     public ContextMenuNativeDelegateImpl(WebContents webContents, RenderFrameHost renderFrameHost,
             ContextMenuParams contextMenuParams) {
-        mNativePtr = ContextMenuNativeDelegateImplJni.get().init(
-                webContents, contextMenuParams, renderFrameHost);
+        mRenderFrameHost = renderFrameHost;
+        mNativePtr = ContextMenuNativeDelegateImplJni.get().init(webContents, contextMenuParams);
     }
 
     @Override
@@ -67,8 +69,8 @@ class ContextMenuNativeDelegateImpl implements ContextMenuNativeDelegate {
             imageRetrieveCallback.onResult(createImageCallbackResultForTesting());
         } else {
             ContextMenuNativeDelegateImplJni.get().retrieveImageForShare(mNativePtr,
-                    ContextMenuNativeDelegateImpl.this, imageRetrieveCallback, MAX_SHARE_DIMEN_PX,
-                    MAX_SHARE_DIMEN_PX, imageFormat);
+                    ContextMenuNativeDelegateImpl.this, mRenderFrameHost, imageRetrieveCallback,
+                    MAX_SHARE_DIMEN_PX, MAX_SHARE_DIMEN_PX, imageFormat);
         }
     }
 
@@ -77,8 +79,9 @@ class ContextMenuNativeDelegateImpl implements ContextMenuNativeDelegate {
             int maxWidthPx, int maxHeightPx, Callback<Bitmap> callback) {
         if (mNativePtr == 0) return;
 
-        ContextMenuNativeDelegateImplJni.get().retrieveImageForContextMenu(
-                mNativePtr, ContextMenuNativeDelegateImpl.this, callback, maxWidthPx, maxHeightPx);
+        ContextMenuNativeDelegateImplJni.get().retrieveImageForContextMenu(mNativePtr,
+                ContextMenuNativeDelegateImpl.this, mRenderFrameHost, callback, maxWidthPx,
+                maxHeightPx);
     }
 
     @Override
@@ -94,7 +97,7 @@ class ContextMenuNativeDelegateImpl implements ContextMenuNativeDelegate {
         if (mNativePtr == 0) return;
 
         ContextMenuNativeDelegateImplJni.get().searchForImage(
-                mNativePtr, ContextMenuNativeDelegateImpl.this);
+                mNativePtr, ContextMenuNativeDelegateImpl.this, mRenderFrameHost);
     }
 
     /**
@@ -124,17 +127,17 @@ class ContextMenuNativeDelegateImpl implements ContextMenuNativeDelegate {
 
     @NativeMethods
     interface Natives {
-        long init(WebContents webContents, ContextMenuParams contextMenuParams,
-                RenderFrameHost renderFrameHost);
+        long init(WebContents webContents, ContextMenuParams contextMenuParams);
         void retrieveImageForShare(long nativeContextMenuNativeDelegateImpl,
-                ContextMenuNativeDelegateImpl caller, Callback<ImageCallbackResult> callback,
-                int maxWidthPx, int maxHeightPx, @ContextMenuImageFormat int imageFormat);
+                ContextMenuNativeDelegateImpl caller, RenderFrameHost renderFrameHost,
+                Callback<ImageCallbackResult> callback, int maxWidthPx, int maxHeightPx,
+                @ContextMenuImageFormat int imageFormat);
         void retrieveImageForContextMenu(long nativeContextMenuNativeDelegateImpl,
-                ContextMenuNativeDelegateImpl caller, Callback<Bitmap> callback, int maxWidthPx,
-                int maxHeightPx);
+                ContextMenuNativeDelegateImpl caller, RenderFrameHost renderFrameHost,
+                Callback<Bitmap> callback, int maxWidthPx, int maxHeightPx);
         void startDownload(long nativeContextMenuNativeDelegateImpl,
                 ContextMenuNativeDelegateImpl caller, boolean isLink);
-        void searchForImage(
-                long nativeContextMenuNativeDelegateImpl, ContextMenuNativeDelegateImpl caller);
+        void searchForImage(long nativeContextMenuNativeDelegateImpl,
+                ContextMenuNativeDelegateImpl caller, RenderFrameHost renderFrameHost);
     }
 }