Rendering the image in the sandbox for security

chrome/browser/ui/android/context_menu_helper.cc

Index: chrome/browser/ui/android/context_menu_helper.cc
diff --git a/chrome/browser/ui/android/context_menu_helper.cc b/chrome/browser/ui/android/context_menu_helper.cc
index 0d677d1e7007e6dc353d104179763c2a70974cda..b42189787275673707ff578df430a95a71e8b282 100644
--- a/chrome/browser/ui/android/context_menu_helper.cc
+++ b/chrome/browser/ui/android/context_menu_helper.cc
@@ -13,6 +13,7 @@
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "chrome/browser/android/download/download_controller_base.h"
+#include "chrome/browser/image_decoder.h"
 #include "chrome/browser/ui/tab_contents/core_tab_helper.h"
 #include "chrome/common/thumbnail_capturer.mojom.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_headers.h"
@@ -22,8 +23,10 @@
 #include "content/public/common/context_menu_params.h"
 #include "jni/ContextMenuHelper_jni.h"
 #include "jni/ContextMenuParams_jni.h"
+#include "services/data_decoder/public/cpp/decode_image.h"

[Ted C, 2017/06/21 00:18:02]

why do you need this?

[Daniel Park, 2017/06/22 00:54:09]

Done.

 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "third_party/WebKit/public/web/WebContextMenuData.h"
+#include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/size.h"
 
@@ -39,13 +42,55 @@ const char kDataReductionProxyPassthroughHeader[] =
 
 namespace {
 
-void OnRetrieveImage(chrome::mojom::ThumbnailCapturerPtr thumbnail_capturer,
-                     const base::android::JavaRef<jobject>& jcallback,
-                     const std::vector<uint8_t>& thumbnail_data,
-                     const gfx::Size& original_size) {
+class ContextMenuHelperImageRequest : public ImageDecoder::ImageRequest {
+ public:
+  static void Create(const base::android::JavaRef<jobject>& jcallback,
+                     const std::vector<uint8_t>& thumbnail_data) {
+    new ContextMenuHelperImageRequest(jcallback, thumbnail_data);
+  }
+
+ protected:
+  void OnImageDecoded(const SkBitmap& decoded_image) override {
+    base::android::RunCallbackAndroid(jcallback_,
+                                      gfx::ConvertToJavaBitmap(&decoded_image));
+    delete this;

[gone, 2017/06/20 00:32:39]

Still need to figure out the right way to deal with this memory management, but
I'm not the right guy.  Might be worth asking Ted or someone else familiar with
native memory _stuff_.  dominickn@ is a solid choice.

[Daniel Park, 2017/06/21 00:01:23]

Acknowledged.

[dominickn, 2017/06/21 00:12:49]

This is fine. There's only one way to create this object (static factory
constructor), and it owns itself. Perhaps just add a comment above ::Create:

// This object owns itself. It will be deleted when image decoding finishes or
fails.

[Ted C, 2017/06/21 00:18:02]

The main thing is to look at who owns this request.  Passing to the decoder does
not take ownership (it uses a std::map of pointers and erase does not clear the
pointer reference, so we're good there).

In general, this looks OK to me.

+  }
+
+  void OnDecodeImageFailed() override {
+    base::android::ScopedJavaLocalRef<jobject> j_bitmap;
+    base::android::RunCallbackAndroid(jcallback_, j_bitmap);
+    delete this;
+  }
+
+ private:
+  ContextMenuHelperImageRequest(
+      const base::android::JavaRef<jobject>& jcallback,
+      const std::vector<uint8_t>& thumbnail_data)
+      : jcallback_(jcallback) {
+    ImageDecoder::Start(this, thumbnail_data);

[Ted C, 2017/06/21 00:18:02]

I wouldn't start this in the constructor.  I would actually suggest renaming
your static function above to Start as well, then that would create a new object
and call ImageDecoder::Start with it.

In general, I try not to have constructor's "do" anything.

[Daniel Park, 2017/06/22 00:54:09]

Done.

+  }
+
+  const base::android::ScopedJavaGlobalRef<jobject> jcallback_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(ContextMenuHelperImageRequest);
+};
+
+void OnRetrieveImageForShare(
+    chrome::mojom::ThumbnailCapturerPtr thumbnail_capturer,
+    const base::android::JavaRef<jobject>& jcallback,
+    const std::vector<uint8_t>& thumbnail_data,
+    const gfx::Size& original_size) {
   base::android::RunCallbackAndroid(jcallback, thumbnail_data);
 }
 
+void OnRetrieveImageForContextMenu(
+    chrome::mojom::ThumbnailCapturerPtr thumbnail_capturer,
+    const base::android::JavaRef<jobject>& jcallback,
+    const std::vector<uint8_t>& thumbnail_data,
+    const gfx::Size& original_size) {
+  ContextMenuHelperImageRequest::Create(jcallback, thumbnail_data);
+}
+
 }  // namespace
 
 ContextMenuHelper::ContextMenuHelper(content::WebContents* web_contents)
@@ -161,10 +206,32 @@ void ContextMenuHelper::SearchForImage(JNIEnv* env,
       render_frame_host, context_menu_params_.src_url);
 }
 
-void ContextMenuHelper::RetrieveImage(JNIEnv* env,
-                                      const JavaParamRef<jobject>& obj,
-                                      const JavaParamRef<jobject>& jcallback,
-                                      jint max_dimen_px) {
+void ContextMenuHelper::RetrieveImageForShare(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& jcallback,
+    jint max_dimen_px) {
+  content::RenderFrameHost* render_frame_host =
+      content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
+  if (!render_frame_host)
+    return;
+
+  chrome::mojom::ThumbnailCapturerPtr thumbnail_capturer;
+  render_frame_host->GetRemoteInterfaces()->GetInterface(&thumbnail_capturer);
+  // Bind the InterfacePtr into the callback so that it's kept alive until

[gone, 2017/06/20 00:32:39]

InterfacePtr?  You're not using one.

[Daniel Park, 2017/06/21 00:01:24]

Done.

[Ted C, 2017/06/21 00:18:02]

The InterfacePtr is hidden behind the auto*

+  // there's either a connection error or a response.
+  auto* thumbnail_capturer_proxy = thumbnail_capturer.get();
+  thumbnail_capturer_proxy->RequestThumbnailForContextNode(
+      0, gfx::Size(max_dimen_px, max_dimen_px),
+      base::Bind(&OnRetrieveImageForShare, base::Passed(&thumbnail_capturer),
+                 base::android::ScopedJavaGlobalRef<jobject>(env, jcallback)));
+}
+
+void ContextMenuHelper::RetrieveImageForContextMenu(

[Ted C, 2017/06/21 00:18:02]

In general, I think we should be able to create a helper function to combine the
bulk of this and above functions.  The function pointers are really the only
difference I see here.

Talking with dtrainor@ and I think we could do something like:

using ImageRetrieveCallback = base::Callback<void(
    chrome::mojom::ThumbnailCapturerPtr,
    const base::android::JavaRef<jobject>&,
    const std::vector<uint8_t>&,
    const gfx::Size&)>;

void ContextMenuHelper::RetrieveImageForShare(
    JNIEnv* env,
    const JavaParamRef<jobject>& obj,
    const JavaParamRef<jobject>& jcallback,
    jint max_dimen_px) {
  RetrieveImageInternal(base::Bind(&OnRetrieveImageForShare), jcallback,
max_dimen_px);
}

void ContextMenuHelper::RetrieveImageInternal(
    const ImageRetrieveCallback& retrieve_callback, jcallback, max_dimen_px) {
  ...
  ..
  .
  thumbnail_capturer_proxy->RequestThumbnailForContextNode(
      0, gfx::Size(max_dimen_px, max_dimen_px),
      base::Bind(retrieve_callback, base::Passed(....)));
      
}

[Daniel Park, 2017/06/22 00:54:09]

Done.

+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jobject>& jcallback,
+    jint max_dimen_px) {
   content::RenderFrameHost* render_frame_host =
       content::RenderFrameHost::FromID(render_process_id_, render_frame_id_);
   if (!render_frame_host)
@@ -177,7 +244,8 @@ void ContextMenuHelper::RetrieveImage(JNIEnv* env,
   auto* thumbnail_capturer_proxy = thumbnail_capturer.get();
   thumbnail_capturer_proxy->RequestThumbnailForContextNode(
       0, gfx::Size(max_dimen_px, max_dimen_px),
-      base::Bind(&OnRetrieveImage, base::Passed(&thumbnail_capturer),
+      base::Bind(&OnRetrieveImageForContextMenu,
+                 base::Passed(&thumbnail_capturer),
                  base::android::ScopedJavaGlobalRef<jobject>(env, jcallback)));
 }