Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2948583002:
Photo Picker dialog: Convert to AIDL for easier security review. (Closed)
DescriptionPhoto Picker dialog: Convert to AIDL for easier security review.
BUG=656015
Review-Url: https://codereview.chromium.org/2948583002
Cr-Commit-Position: refs/heads/master@{#481229}
Committed: https://chromium.googlesource.com/chromium/src/+/38cde60a4b6cd9c61a668d70a14d37cd88bd5cc5
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address feedback from Robert #Patch Set 3 : Adding to owners file #
Total comments: 6
Patch Set 4 : Address feedback from Theresa #
Total comments: 5
Patch Set 5 : Nits addressed #Patch Set 6 : Add comment #Messages
Total messages: 39 (24 generated)
The CQ bit was checked by finnur@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...
finnur@chromium.org changed reviewers: + rsesek@chromium.org, twellington@chromium.org
Theresa, mind taking a look? Robert, please review to see if this is as you requested. This is my first foray into AIDL, so there might be something glaringly obvious that needs addressing. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Overall looks good. This is already covered by unit tests, right? https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:48: Log.e(TAG, "Service has unexpectedly disconnected"); This will be called if you ever unbindService(), which should be done to clean up the process when it's no longer needed. https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceListener.aidl (right): https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceListener.aidl:7: interface IDecoderServiceListener { "Callback" would be a more typical name. https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceListener.aidl:8: oneway void onDecodeImageDone(in Bundle payload); nit: indent
The CQ bit was checked by finnur@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...
Theresa, PTAL. Robert: yes, there are unit tests for specific things and an end-to-end test covering everything from showing the dialog to selecting photos and making sure they are returned. Also note: I've dropped the Impl suffix change -- it wasn't buying us anything and just complicating the review and the reviewer requirements. https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:48: Log.e(TAG, "Service has unexpectedly disconnected"); I do call unbindService but I do not see this function being called as a result. The onServiceDisconnected in the Android sample... https://developer.android.com/guide/components/aidl.html ... also has an equivalent Log line. https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceListener.aidl (right): https://codereview.chromium.org/2948583002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceListener.aidl:8: oneway void onDecodeImageDone(in Bundle payload); On 2017/06/20 02:09:47, Robert Sesek wrote: > nit: indent Done.
finnur@chromium.org changed reviewers: + agrieve@chromium.org
Andrew, mind taking the BUILD.gn change?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by finnur@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...
Looks good overall https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:251: Message payload = Message.obtain(); Is payload used anymore? https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderService.aidl (right): https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderService.aidl:11: interface IDecoderService { Does this need documentation? https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceCallback.aidl (right): https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceCallback.aidl:7: interface IDecoderServiceCallback { Same question for this interface. I'm not sure what the standard is for .aidl documentation.
The CQ bit was checked by finnur@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...
All addressed, PTAL. https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:251: Message payload = Message.obtain(); No. Good catch. https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderService.aidl (right): https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderService.aidl:11: interface IDecoderService { On 2017/06/20 15:11:56, Theresa wrote: > Does this need documentation? Done. https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceCallback.aidl (right): https://codereview.chromium.org/2948583002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/IDecoderServiceCallback.aidl:7: interface IDecoderServiceCallback { Some of the .aidl in the tree already indicate a precedent for the same documentation standard as Java code.
One more question about threading; sorry for not catching it on the first pass. https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:253: mIRemoteService.decodeImage(bundle, this); Does the service automatically run this on a different thread than the UI thread? Also, would it make sense for the DecoderService to be responsible for calling onDecodeImageDone on the correct (UI) thread? The asymmetry of making the request on the UI thread but receiving it not on the UI thread is a bit confusing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/20 16:53:12, Theresa wrote: > One more question about threading; sorry for not catching it on the first pass. > > https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java > (right): > > https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:253: > mIRemoteService.decodeImage(bundle, this); > Does the service automatically run this on a different thread than the UI > thread? > > Also, would it make sense for the DecoderService to be responsible for calling > onDecodeImageDone on the correct (UI) thread? The asymmetry of making the > request on the UI thread but receiving it not on the UI thread is a bit > confusing. BUILD lgtm
LGTM. Thank you for doing this! https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:110: private Context mContext; final?
Thanks all for your review. Theresa, answer below. https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:253: mIRemoteService.decodeImage(bundle, this); No, no problem. This is exactly the kind of thing that one would expect and hope are brought up during code review. The later we catch this kind of issues, the more expensive it is to fix them. > Does the service automatically run this on a different thread than the UI thread? That is what I experienced. The callback is on some arbitrary thread and jumping to the UI thread on the callback was necessary to be able to touch UI objects without exceptions. This doc touches upon threading issues: https://developer.android.com/guide/components/aidl.html
The CQ bit was checked by finnur@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...
Nits addressed. Theresa, just a friendly reminder: you had given a looks good overall stamp but not the formal approval one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm % one last nigt https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:253: mIRemoteService.decodeImage(bundle, this); On 2017/06/20 20:55:05, Finnur wrote: > No, no problem. This is exactly the kind of thing that one would expect and hope > are brought up during code review. The later we catch this kind of issues, the > more expensive it is to fix them. > > > Does the service automatically run this on a different thread than the UI > thread? > > That is what I experienced. The callback is on some arbitrary thread and jumping > to the UI thread on the callback was necessary to be able to touch UI objects > without exceptions. > > This doc touches upon threading issues: > https://developer.android.com/guide/components/aidl.html Thanks for the link, that is very helpful. So this would fall under "Calls from a remote process are dispatched from a thread pool the platform maintains inside of your own process. You must be prepared for incoming calls from unknown threads, with multiple calls happening at the same time. In other words, an implementation of an AIDL interface must be completely thread-safe." Correct? Will you please add a short comment to #onDecodeImageDone() explaining that the AIDL callback may happen on any thread?
Updated. Thanks for the review, checking in now. https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java (right): https://codereview.chromium.org/2948583002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderServiceHost.java:253: mIRemoteService.decodeImage(bundle, this); Yes. Done.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, agrieve@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2948583002/#ps100001 (title: "Add comment")
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": 100001, "attempt_start_ts": 1498058438822210,
"parent_rev": "164abed19ba6d4882dc3b66028bcbfd24399b177", "commit_rev":
"38cde60a4b6cd9c61a668d70a14d37cd88bd5cc5"}
Message was sent while issue was closed.
Description was changed from ========== Photo Picker dialog: Convert to AIDL for easier security review. BUG=656015 ========== to ========== Photo Picker dialog: Convert to AIDL for easier security review. BUG=656015 Review-Url: https://codereview.chromium.org/2948583002 Cr-Commit-Position: refs/heads/master@{#481229} Committed: https://chromium.googlesource.com/chromium/src/+/38cde60a4b6cd9c61a668d70a14d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/38cde60a4b6cd9c61a668d70a14d... |
