Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2945903002:
Rendering the image in the sandbox for security (Closed)
DescriptionRendering the image in the sandbox for security
Handled case where image decode would fail because of malicious code
If malicious code is found, the sad tab image is displayed as the thumbnail in the context menu
Changed Callback<Type> from byte[] to Bitmap Changed native code to use the SkBitmap to a JavaBitmap
Added logic to decode image a second time for the context menu
BUG=734665
Review-Url: https://codereview.chromium.org/2945903002
Cr-Commit-Position: refs/heads/master@{#481991}
Committed: https://chromium.googlesource.com/chromium/src/+/d698fa98c4731a224995ef1c581ad42cab0820b2
Patch Set 1 #
Total comments: 19
Patch Set 2 : Comments #
Total comments: 3
Patch Set 3 : comments #
Messages
Total messages: 28 (17 generated)
The CQ bit was checked by danielpark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:285: mHeaderImageView.setImageResource(R.drawable.sad_tab); Any reason you picked sad tab? Is it in the mock? https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:56: delete this; 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. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:221: // Bind the InterfacePtr into the callback so that it's kept alive until InterfacePtr? You're not using one.
danielpark@chromium.org changed reviewers: + dominickn@chromium.org - dfalcantara@chromium.org, tedchoc@chromium.org, twellington@chromium.org
https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:285: mHeaderImageView.setImageResource(R.drawable.sad_tab); On 2017/06/20 00:32:39, dfalcantara wrote: > Any reason you picked sad tab? Is it in the mock? Acknowledged. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:56: delete this; On 2017/06/20 00:32:39, dfalcantara wrote: > 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. Acknowledged. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:221: // Bind the InterfacePtr into the callback so that it's kept alive until On 2017/06/20 00:32:39, dfalcantara wrote: > InterfacePtr? You're not using one. Done.
Memory management seems fine to me. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:56: delete this; 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.
dfalcantara@chromium.org changed reviewers: + dfalcantara@chromium.org
lgtm % comments
tedchoc@chromium.org changed reviewers: + tedchoc@chromium.org
https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:211: Callback<Bitmap> rawDataCallback = new Callback<Bitmap>() { let's rename rawData to bitmapCallback https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:285: mHeaderImageView.setImageResource(R.drawable.sad_tab); On 2017/06/21 00:01:23, Daniel Park wrote: > On 2017/06/20 00:32:39, dfalcantara wrote: > > Any reason you picked sad tab? Is it in the mock? > > Acknowledged. Nit, but what are you acknowledging? I would give a bit clearer answers to help other reviewers. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:26: #include "services/data_decoder/public/cpp/decode_image.h" why do you need this? https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:56: delete this; On 2017/06/20 00:32:39, dfalcantara wrote: > 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. 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. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:70: ImageDecoder::Start(this, thumbnail_data); 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. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:221: // Bind the InterfacePtr into the callback so that it's kept alive until On 2017/06/20 00:32:39, dfalcantara wrote: > InterfacePtr? You're not using one. The InterfacePtr is hidden behind the auto* https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:230: void ContextMenuHelper::RetrieveImageForContextMenu( 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(....))); }
danielpark@chromium.org changed reviewers: + twellington@chromium.org - dominickn@chromium.org
https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java (right): https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:211: Callback<Bitmap> rawDataCallback = new Callback<Bitmap>() { On 2017/06/21 00:18:02, Ted C wrote: > let's rename rawData to bitmapCallback Done. https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java (right): https://codereview.chromium.org/2945903002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuUi.java:285: mHeaderImageView.setImageResource(R.drawable.sad_tab); On 2017/06/21 00:18:02, Ted C wrote: > On 2017/06/21 00:01:23, Daniel Park wrote: > > On 2017/06/20 00:32:39, dfalcantara wrote: > > > Any reason you picked sad tab? Is it in the mock? > > > > Acknowledged. > > Nit, but what are you acknowledging? I would give a bit clearer answers to help > other reviewers. Got it. I talked with Dan offline about it, and the decision was to keep it as sad tab until I hear back from @hannahs. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... File chrome/browser/ui/android/context_menu_helper.cc (right): https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:26: #include "services/data_decoder/public/cpp/decode_image.h" On 2017/06/21 00:18:02, Ted C wrote: > why do you need this? Done. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:70: ImageDecoder::Start(this, thumbnail_data); On 2017/06/21 00:18:02, Ted C wrote: > 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. Done. https://codereview.chromium.org/2945903002/diff/1/chrome/browser/ui/android/c... chrome/browser/ui/android/context_menu_helper.cc:230: void ContextMenuHelper::RetrieveImageForContextMenu( On 2017/06/21 00:18:02, Ted C wrote: > 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(....))); > > } > Done.
The CQ bit was checked by danielpark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2945903002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/context_menu_helper.h (right): https://codereview.chromium.org/2945903002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/context_menu_helper.h:28: using ImageRetrieveCallback = base::Callback<void( Can this be in the private or protected sections or does it need to be public?
https://codereview.chromium.org/2945903002/diff/20001/chrome/browser/ui/andro... File chrome/browser/ui/android/context_menu_helper.h (right): https://codereview.chromium.org/2945903002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/context_menu_helper.h:28: using ImageRetrieveCallback = base::Callback<void( On 2017/06/22 22:33:13, Ted C wrote: > Can this be in the private or protected sections or does it need to be public? moved to protected. https://codereview.chromium.org/2945903002/diff/20001/chrome/browser/ui/andro... chrome/browser/ui/android/context_menu_helper.h:28: using ImageRetrieveCallback = base::Callback<void( On 2017/06/22 22:33:13, Ted C wrote: > Can this be in the private or protected sections or does it need to be public? Done.
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2945903002/#ps40001 (title: "comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498243461497860,
"parent_rev": "d5b40133c57d80b9368a832ee8ad497d34aab392", "commit_rev":
"d698fa98c4731a224995ef1c581ad42cab0820b2"}
Message was sent while issue was closed.
Description was changed from ========== Rendering the image in the sandbox for security Handled case where image decode would fail because of malicious code If malicious code is found, the sad tab image is displayed as the thumbnail in the context menu Changed Callback<Type> from byte[] to Bitmap Changed native code to use the SkBitmap to a JavaBitmap Added logic to decode image a second time for the context menu BUG=734665 ========== to ========== Rendering the image in the sandbox for security Handled case where image decode would fail because of malicious code If malicious code is found, the sad tab image is displayed as the thumbnail in the context menu Changed Callback<Type> from byte[] to Bitmap Changed native code to use the SkBitmap to a JavaBitmap Added logic to decode image a second time for the context menu BUG=734665 Review-Url: https://codereview.chromium.org/2945903002 Cr-Commit-Position: refs/heads/master@{#481991} Committed: https://chromium.googlesource.com/chromium/src/+/d698fa98c4731a224995ef1c581a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d698fa98c4731a224995ef1c581a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
