Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2015433003:
Implement MediaMetadata artwork in content (Closed)
|
Created:
4 years, 6 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 5 months ago Reviewers:
jochen (gone - plz use gerrit), mlamouri (slow - plz ping), dcheng, Mike West, DaleCurtis, palmer CC:
avayvod+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, haraken, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-manifest_chromium.org, mlamouri+watch-media_chromium.org, nasko+codewatch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement MediaMetadata artwork in content
This CL implements MediaMetadata artwork in the content layer,
and completes the propagation of MediaMetadata from blink to the
browser process.
BUG=616411
Committed: https://crrev.com/5d2a9140bd85b681a0290719da33569db66d06cc
Cr-Commit-Position: refs/heads/master@{#404125}
Patch Set 1 : #Patch Set 2 : fixed tests #
Total comments: 4
Patch Set 3 : addressed Mounir's comments (not ready for review) #Patch Set 4 : Addressed Mounir's comments #Patch Set 5 : rebasing onto common icon sizes parser #Patch Set 6 : fixed nits, added TODOs #
Total comments: 8
Patch Set 7 : adding sanitize check for artwork #
Total comments: 14
Patch Set 8 : addressed palmer's comments #
Total comments: 9
Patch Set 9 : addressed Mounir's comments #
Total comments: 10
Patch Set 10 : addressed Mounir's comments #
Total comments: 12
Patch Set 11 : addressed reviewers' comments #
Total comments: 11
Patch Set 12 : addressed Dale's comments #
Total comments: 3
Patch Set 13 : moving metadata sanitizing into one place, not ready for review yet #Patch Set 14 : fixed lint issues #
Total comments: 16
Patch Set 15 : addressed dcheng's comments #
Total comments: 10
Patch Set 16 : addressed dcheng's comments #
Total comments: 2
Patch Set 17 : fixed nits #Messages
Total messages: 65 (20 generated)
Patchset #1 (id:1) has been deleted
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
Description was changed from ========== implement MediaMetadata artwork in content Implement MediaSession artwork in Blink BUG= ========== to ========== implement MediaMetadata artwork in content BUG= ==========
Description was changed from ========== implement MediaMetadata artwork in content BUG= ========== to ========== Implement MediaMetadata artwork in content BUG=616411 ==========
It looks good but I think we shouldn't split the media session callbacks (mediasessionstatechanged and metadatachanged) because we will end up sending a callback to say that a media session has started without metadata then just after send the metadata. This would be a common use case and we should make sure the API works well with it and it will not. Though, there is clearly a memory issue so I would recommend that we send a null metadata if there are no changes in metadata in order to save memory. WDYT? https://codereview.chromium.org/2015433003/diff/40001/content/browser/media/a... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/40001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.cc:54: MediaSession::Get(contents)->SetMetadata(metadata); Better to leave this as-is. It should be plumbed when the API will be plumbed. There was some work in that direction but it stalled. https://codereview.chromium.org/2015433003/diff/40001/content/renderer/manife... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/2015433003/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.h:27: std::vector<gfx::Size> ParseIconSizesHTML(const base::string16& sizes_str16); Not a big fan of re-using the parsing algorithm of the Manifest but let see how the other CL goes. Worse case, we can land with a TODO and switch to the Blink API when available.
Description was changed from ========== Implement MediaMetadata artwork in content BUG=616411 ========== to ========== Implement MediaMetadata artwork in content This CL implements MediaMetadata artwork in the content layer, and completes the propagation of MediaMetadata from blink to the browser process. BUG=616411 ==========
Description was changed from ========== Implement MediaMetadata artwork in content This CL implements MediaMetadata artwork in the content layer, and completes the propagation of MediaMetadata from blink to the browser process. BUG=616411 ========== to ========== Implement MediaMetadata artwork in content This CL implements MediaMetadata artwork in the content layer, and completes the propagation of MediaMetadata from blink to the browser process. BUG=616411 ==========
PTAL
> It looks good but I think we shouldn't split the media session callbacks > (mediasessionstatechanged and metadatachanged) because we will end up sending a > callback to say that a media session has started without metadata then just > after send the metadata. This would be a common use case and we should make sure > the API works well with it and it will not. Though, there is clearly a memory > issue so I would recommend that we send a null metadata if there are no changes > in metadata in order to save memory. WDYT? Reverted the split of mediaSessionStateChanged and mediaMetadataChanged(). We can iterate later. https://codereview.chromium.org/2015433003/diff/40001/content/browser/media/a... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/40001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.cc:54: MediaSession::Get(contents)->SetMetadata(metadata); On 2016/06/10 10:54:58, Mounir Lamouri (slow) wrote: > Better to leave this as-is. It should be plumbed when the API will be plumbed. > There was some work in that direction but it stalled. Acknowledged. https://codereview.chromium.org/2015433003/diff/40001/content/renderer/manife... File content/renderer/manifest/manifest_parser.h (right): https://codereview.chromium.org/2015433003/diff/40001/content/renderer/manife... content/renderer/manifest/manifest_parser.h:27: std::vector<gfx::Size> ParseIconSizesHTML(const base::string16& sizes_str16); On 2016/06/10 10:54:58, Mounir Lamouri (slow) wrote: > Not a big fan of re-using the parsing algorithm of the Manifest but let see how > the other CL goes. Worse case, we can land with a TODO and switch to the Blink > API when available. Rebased onto the common parser in Blink.
zqzhang@chromium.org changed reviewers: + palmer@chromium.org, sievers@chromium.org
Adding reviewers +palmer to look at media_session_messages_android.h +siever to review contents/
https://codereview.chromium.org/2015433003/diff/120001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/120001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:42: // TODO(zqzhang): security checking for artwork? What is the nature of the artwork? To what extent do we trust it? (For example, is it a JPEG that we parse and render in the browser process?) To what extent can we establish its trustworthiness? (For example, if for some reason we had to render it in the browser, perhaps we could first transform it to a simpler, safer format or otherwise normalize it in a utility process. Or something.) https://codereview.chromium.org/2015433003/diff/120001/content/browser/media/... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2015433003/diff/120001/content/browser/media/... content/browser/media/session/media_session.cc:63: static_cast<WebContentsImpl*>(web_contents())->OnMediaSessionStateChanged(); What guarantees that this cast is safe/correct? https://codereview.chromium.org/2015433003/diff/120001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/120001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:76: // TODO(zqzhang): apply sanity checks on artwork? Yes, please. :) For example, is the src valid? (What is it supposed to be? A URL? If so, are there aspects of the URL you can check for, such as is it parseable, does it use an approved scheme (e.g. not telnet: or mailto:), et c.?) Similarly, are its sizes reasonable? A rectangle that is 2 billion x 2 billion pixels is probably questionable, for example. That kind of thing. https://codereview.chromium.org/2015433003/diff/120001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/2015433003/diff/120001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:49: artwork.src = GURL(base::string16(web_artwork.src)); Right, so it's a URL. Let's check that basic GURL sanity holds, such as is_valid(), IsStandard(), SchemeIsHTTPOrHTTPS(), et c.
Patchset #7 (id:140001) has been deleted
PTAL, addressed palmer's comments. https://codereview.chromium.org/2015433003/diff/120001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/120001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:42: // TODO(zqzhang): security checking for artwork? On 2016/06/14 22:06:31, palmer wrote: > What is the nature of the artwork? > > To what extent do we trust it? (For example, is it a JPEG that we parse and > render in the browser process?) > > To what extent can we establish its trustworthiness? (For example, if for some > reason we had to render it in the browser, perhaps we could first transform it > to a simpler, safer format or otherwise normalize it in a utility process. Or > something.) See the other reply for details. https://codereview.chromium.org/2015433003/diff/120001/content/browser/media/... File content/browser/media/session/media_session.cc (right): https://codereview.chromium.org/2015433003/diff/120001/content/browser/media/... content/browser/media/session/media_session.cc:63: static_cast<WebContentsImpl*>(web_contents())->OnMediaSessionStateChanged(); On 2016/06/14 22:06:31, palmer wrote: > What guarantees that this cast is safe/correct? WebContentsImpl is the only subclass of WebContents, and MediaSession is valid as long as WebContents lives, so this cast is safe. Another example is MediaSession::UpdateWebContents(). https://codereview.chromium.org/2015433003/diff/120001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/120001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:76: // TODO(zqzhang): apply sanity checks on artwork? On 2016/06/14 22:06:31, palmer wrote: > Yes, please. :) For example, is the src valid? (What is it supposed to be? A > URL? If so, are there aspects of the URL you can check for, such as is it > parseable, does it use an approved scheme (e.g. not telnet: or mailto:), et c.?) > > Similarly, are its sizes reasonable? A rectangle that is 2 billion x 2 billion > pixels is probably questionable, for example. That kind of thing. Now I'm doing sanity checks (is_valid(), IsStandard(), SchemeIsHttpOrHttps(), SchemeIsFile()). For type, I'm just truncating the string if it's too long. For sizes, I'm removing the sizes that are negative or too large. https://codereview.chromium.org/2015433003/diff/120001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/2015433003/diff/120001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:49: artwork.src = GURL(base::string16(web_artwork.src)); On 2016/06/14 22:06:32, palmer wrote: > Right, so it's a URL. Let's check that basic GURL sanity holds, such as > is_valid(), IsStandard(), SchemeIsHTTPOrHTTPS(), et c. Done. See the other reply for details.
https://codereview.chromium.org/2015433003/diff/160001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2015433003/diff/160001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:321: // seprate the state change and Metadata update. It's a good idea to attach a crbug link to all TODOs. https://codereview.chromium.org/2015433003/diff/160001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/160001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:42: for (const MediaMetadata::Artwork& artwork : insecure_metadata.artwork) { Nit: using auto here and possibly on line 43 would make this code easier to read. https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... File content/public/common/media_metadata.cc (right): https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... content/public/common/media_metadata.cc:31: !artwork.src.SchemeIsFile())) { Is there any chance that allowing file: URLs here could enable media metadata authors to induce the calling process to read a file that the calling process can read but which the metadata author cannot? And then, is there some way that the calling process might accidentally leak or reveal information in/about that file to the metadata author in some way? For example, what if web content could say that a media object's artwork is file:///etc/passwd (or file:///data/app-private/...), and then the browser process opened the file, and gave a descriptor or the raw bytes to the web content? If that scenario is plausible, we should not allow file: URLs, or at least not all file URLs. If the scenario is implausible, can you explain what stops it from happening? Thanks! https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... content/public/common/media_metadata.cc:39: artwork.type.string().substr(0, kMaxIPCStringLength), Is the range of valid types really the set of strings containing 0 to 4096 arbitrary byte values? https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... content/public/common/media_metadata.h:20: // TODO(zqzhang): move |Manifest::Icon| to a common place. It's good to have a crbug link. https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... content/public/common/media_metadata.h:42: std::vector<Artwork> artwork; Nit: Since this is a vector, I might call it |artworks| or |artwork_list| or something like that. https://codereview.chromium.org/2015433003/diff/160001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/160001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:77: for (const MediaMetadata::Artwork& artwork : metadata.artwork) { Nit: Can use auto here too, to clarify the code.
Done. Address palmer's comments :) https://codereview.chromium.org/2015433003/diff/160001/content/browser/androi... File content/browser/android/web_contents_observer_proxy.cc (right): https://codereview.chromium.org/2015433003/diff/160001/content/browser/androi... content/browser/android/web_contents_observer_proxy.cc:321: // seprate the state change and Metadata update. On 2016/06/20 21:34:20, palmer wrote: > It's a good idea to attach a crbug link to all TODOs. Done. https://codereview.chromium.org/2015433003/diff/160001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/160001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:42: for (const MediaMetadata::Artwork& artwork : insecure_metadata.artwork) { On 2016/06/20 21:34:21, palmer wrote: > Nit: using auto here and possibly on line 43 would make this code easier to > read. Done. https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... File content/public/common/media_metadata.cc (right): https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... content/public/common/media_metadata.cc:31: !artwork.src.SchemeIsFile())) { On 2016/06/20 21:34:21, palmer wrote: > Is there any chance that allowing file: URLs here could enable media metadata > authors to induce the calling process to read a file that the calling process > can read but which the metadata author cannot? > > And then, is there some way that the calling process might accidentally leak or > reveal information in/about that file to the metadata author in some way? > > For example, what if web content could say that a media object's artwork is > file:///etc/passwd (or file:///data/app-private/...), and then the browser > process opened the file, and gave a descriptor or the raw bytes to the web > content? > > If that scenario is plausible, we should not allow file: URLs, or at least not > all file URLs. If the scenario is implausible, can you explain what stops it > from happening? Thanks! The image is fetched via WebContents::DownloadImage(), which basically passes the URL to the renderer, and eventually the request is initiated in blink, where regular security checks should apply I think? https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... content/public/common/media_metadata.cc:39: artwork.type.string().substr(0, kMaxIPCStringLength), On 2016/06/20 21:34:21, palmer wrote: > Is the range of valid types really the set of strings containing 0 to 4096 > arbitrary byte values? Hmm, we actually allow any string here (see Manifest::Icon), but the size limit for MIME types actually 127*2+1 (RFC 4288). Fixed. https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... content/public/common/media_metadata.h:20: // TODO(zqzhang): move |Manifest::Icon| to a common place. On 2016/06/20 21:34:21, palmer wrote: > It's good to have a crbug link. Done. https://codereview.chromium.org/2015433003/diff/160001/content/public/common/... content/public/common/media_metadata.h:42: std::vector<Artwork> artwork; On 2016/06/20 21:34:21, palmer wrote: > Nit: Since this is a vector, I might call it |artworks| or |artwork_list| or > something like that. Yes. The naming here is confusing, which comes from the MediaMetadata spec. We are addressing the naming spec now. We'll update the code (TBR should be okay?) once the spec has been fixed. https://codereview.chromium.org/2015433003/diff/160001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/160001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:77: for (const MediaMetadata::Artwork& artwork : metadata.artwork) { On 2016/06/20 21:34:21, palmer wrote: > Nit: Can use auto here too, to clarify the code. Done.
https://codereview.chromium.org/2015433003/diff/180001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/180001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:57: MediaSession::Get(contents)->SetMetadata(metadata); I guess you did that to test your change. Though, we should get the MediaSession that matches |session_id| ideally. It's probably better to keep this change out so we don't have strange bugs very hard to identify when the implementation of the API goes forward. https://codereview.chromium.org/2015433003/diff/180001/content/common/android... File content/common/android/media_metadata_android.cc (right): https://codereview.chromium.org/2015433003/diff/180001/content/common/android... content/common/android/media_metadata_android.cc:37: env, j_src.obj(), j_type.obj()); Is there any way you can pass the sizes in there? Something similar to std::vector<std::pair<int,int>>? Maybe with arrays of arrays where the second array is of size 2? If you can do that, you could just have addArtwork() which would make all of this simpler (and less JNI calls). https://codereview.chromium.org/2015433003/diff/180001/content/public/common/... File content/public/common/media_metadata.cc (right): https://codereview.chromium.org/2015433003/diff/180001/content/public/common/... content/public/common/media_metadata.cc:46: size.height() >= 0 && size.height() <= kMaxIconSize; }); palmer@ why do we need to check this? The sizes here are pure hints to the UA. It sounds strange to filter out from the IPC messages images with sizes that are too large instead of letting the final user make that decision. We don't auto-fetch the image and the image can any way have a very different size than the advertised one. In other words, an image can be advertised as 1x1 pixels and end up being HD. https://codereview.chromium.org/2015433003/diff/180001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/180001/content/public/common/... content/public/common/media_metadata.h:50: // Maximum size of Artwork. The renderer process should remove the sizes in nit: add empty line https://codereview.chromium.org/2015433003/diff/180001/content/public/common/... content/public/common/media_metadata.h:54: // Maximum type length of Artwork, which conforms to RFc 4288 nit: add empty line + s/RFc/RFC/
Addressed Mounir's comments. PTAL https://codereview.chromium.org/2015433003/diff/180001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/180001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:57: MediaSession::Get(contents)->SetMetadata(metadata); On 2016/06/21 13:19:46, Mounir Lamouri (slow) wrote: > I guess you did that to test your change. Though, we should get the MediaSession > that matches |session_id| ideally. It's probably better to keep this change out > so we don't have strange bugs very hard to identify when the implementation of > the API goes forward. Done. https://codereview.chromium.org/2015433003/diff/180001/content/common/android... File content/common/android/media_metadata_android.cc (right): https://codereview.chromium.org/2015433003/diff/180001/content/common/android... content/common/android/media_metadata_android.cc:37: env, j_src.obj(), j_type.obj()); On 2016/06/21 13:19:46, Mounir Lamouri (slow) wrote: > Is there any way you can pass the sizes in there? Something similar to > std::vector<std::pair<int,int>>? Maybe with arrays of arrays where the second > array is of size 2? If you can do that, you could just have addArtwork() which > would make all of this simpler (and less JNI calls). Now I flatten the std::vector<std::pair<int,int>> into std::vector<int>, convert it into jintArray, and pass it to Java. https://codereview.chromium.org/2015433003/diff/180001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/180001/content/public/common/... content/public/common/media_metadata.h:50: // Maximum size of Artwork. The renderer process should remove the sizes in On 2016/06/21 13:19:46, Mounir Lamouri (slow) wrote: > nit: add empty line Done. https://codereview.chromium.org/2015433003/diff/180001/content/public/common/... content/public/common/media_metadata.h:54: // Maximum type length of Artwork, which conforms to RFc 4288 On 2016/06/21 13:19:46, Mounir Lamouri (slow) wrote: > nit: add empty line + s/RFc/RFC/ Done.
""" > If that scenario is plausible, we should not allow file: URLs, or at least not > all file URLs. If the scenario is implausible, can you explain what stops it > from happening? Thanks! The image is fetched via WebContents::DownloadImage(), which basically passes the URL to the renderer, and eventually the request is initiated in blink, where regular security checks should apply I think? """ |WebContentsImpl::DownloadImage| is in content/browser/web_contents/web_contents_impl.cc, and so would seem to happen on the browser process' authority. That suggests that indeed, it could be possible for a renderer to get access to a file: URL that it should be able to. (Test it to be sure.) """ content/public/common/media_metadata.cc:46: size.height() >= 0 && size.height() <= kMaxIconSize; }); palmer@ why do we need to check this? The sizes here are pure hints to the UA. It sounds strange to filter out from the IPC messages images with sizes that are too large instead of letting the final user make that decision. We don't auto-fetch the image and the image can any way have a very different size than the advertised one. In other words, an image can be advertised as 1x1 pixels and end up being HD. """ It's true, the ultimate relying party that needs for these values to be sane needs to do the check for itself. Doing the check in the untrustworthy code is merely a politeness. But if it does lead to incorrectness then it can be removed, since the relying party should do its check.
On 2016/06/22 at 22:43:18, palmer wrote: > """ > > If that scenario is plausible, we should not allow file: URLs, or at least not > > all file URLs. If the scenario is implausible, can you explain what stops it > > from happening? Thanks! > > The image is fetched via WebContents::DownloadImage(), which basically passes > the URL to the renderer, and eventually the request is initiated in blink, where > regular security checks should apply I think? > """ > > |WebContentsImpl::DownloadImage| is in content/browser/web_contents/web_contents_impl.cc, and so would seem to happen on the browser process' authority. That suggests that indeed, it could be possible for a renderer to get access to a file: URL that it should be able to. (Test it to be sure.) WebContentsImpl::DownloadImage ends up here: https://cs.chromium.org/chromium/src/content/renderer/image_downloader/image_... Adding tests sound reasonable, though, I'm not sure how extensively we should test the behaviour of another component (ie. Image Downloader). > """ > content/public/common/media_metadata.cc:46: size.height() >= 0 && size.height() > <= kMaxIconSize; }); > palmer@ why do we need to check this? The sizes here are pure hints to the UA. > It sounds strange to filter out from the IPC messages images with sizes that are > too large instead of letting the final user make that decision. We don't > auto-fetch the image and the image can any way have a very different size than > the advertised one. In other words, an image can be advertised as 1x1 pixels and > end up being HD. > """ > > It's true, the ultimate relying party that needs for these values to be sane needs to do the check for itself. Doing the check in the untrustworthy code is merely a politeness. But if it does lead to incorrectness then it can be removed, since the relying party should do its check. Great!
lgtm with comments applied (inc. not checking for the image size) https://codereview.chromium.org/2015433003/diff/200001/content/common/android... File content/common/android/media_metadata_android.cc (right): https://codereview.chromium.org/2015433003/diff/200001/content/common/android... content/common/android/media_metadata_android.cc:15: std::vector<int> GetFlattenedSizeArray(const std::vector<gfx::Size>& sizes) { style: empty lines around namespace {} https://codereview.chromium.org/2015433003/diff/200001/content/common/android... content/common/android/media_metadata_android.cc:24: } nit: } // anonymous namespace https://codereview.chromium.org/2015433003/diff/200001/content/common/android... content/common/android/media_metadata_android.cc:56: env, j_metadata.obj(), j_artwork.obj()); It sounds like we could merge addArtwork and createArtwork, right? Could we just have addArtwork and pass all the info so the Java implementation creates it and adds it. It would save one JNI call :) https://codereview.chromium.org/2015433003/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java (right): https://codereview.chromium.org/2015433003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:48: public String getSrc() { nit: @NonNull maybe? https://codereview.chromium.org/2015433003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:174: for (int i = 0; i < flattenedSizes.length - 1; i += 2) { nit: add an assert that length is even? and/or, you can check that `(i+1) < flattenedSizes.length`.
Patchset #10 (id:220001) has been deleted
zqzhang@chromium.org changed reviewers: + jochen@chromium.org
PTAL. Addressed Mounir's comments. +jochen@ to look at content/ since sievers's review queue is a bit overwhelmed. On 2016/06/22 22:43:18, palmer wrote: > """ > > If that scenario is plausible, we should not allow file: URLs, or at least not > > all file URLs. If the scenario is implausible, can you explain what stops it > > from happening? Thanks! > > The image is fetched via WebContents::DownloadImage(), which basically passes > the URL to the renderer, and eventually the request is initiated in blink, where > regular security checks should apply I think? > """ > > |WebContentsImpl::DownloadImage| is in > content/browser/web_contents/web_contents_impl.cc, and so would seem to happen > on the browser process' authority. That suggests that indeed, it could be > possible for a renderer to get access to a file: URL that it should be able to. > (Test it to be sure.) Did tested this locally, downloadImage() does block fetching file:/// scheme URLs when the frame is HTTP/HTTPS scheme. The fetch code eventually reaches `MixedContentChecker::shouldBlockFetch()`, where the check is done. We could write tests for this, but since it's a different module, so not put the tests in this CL. https://codereview.chromium.org/2015433003/diff/200001/content/common/android... File content/common/android/media_metadata_android.cc (right): https://codereview.chromium.org/2015433003/diff/200001/content/common/android... content/common/android/media_metadata_android.cc:15: std::vector<int> GetFlattenedSizeArray(const std::vector<gfx::Size>& sizes) { On 2016/06/23 12:52:28, Mounir Lamouri (slow) wrote: > style: empty lines around namespace {} Done. https://codereview.chromium.org/2015433003/diff/200001/content/common/android... content/common/android/media_metadata_android.cc:24: } On 2016/06/23 12:52:29, Mounir Lamouri (slow) wrote: > nit: > } // anonymous namespace Done. https://codereview.chromium.org/2015433003/diff/200001/content/common/android... content/common/android/media_metadata_android.cc:56: env, j_metadata.obj(), j_artwork.obj()); On 2016/06/23 12:52:28, Mounir Lamouri (slow) wrote: > It sounds like we could merge addArtwork and createArtwork, right? Could we just > have addArtwork and pass all the info so the Java implementation creates it and > adds it. It would save one JNI call :) Done. https://codereview.chromium.org/2015433003/diff/200001/content/public/android... File content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java (right): https://codereview.chromium.org/2015433003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:48: public String getSrc() { On 2016/06/23 12:52:29, Mounir Lamouri (slow) wrote: > nit: @NonNull maybe? Done. https://codereview.chromium.org/2015433003/diff/200001/content/public/android... content/public/android/java/src/org/chromium/content_public/common/MediaMetadata.java:174: for (int i = 0; i < flattenedSizes.length - 1; i += 2) { On 2016/06/23 12:52:29, Mounir Lamouri (slow) wrote: > nit: add an assert that length is even? > > and/or, you can check that `(i+1) < flattenedSizes.length`. Done.
the CL description says that something got completed - so where are the tests? This should be reviewed by somebody in media/OWNERS and somebody that owns the java bits btw, I'm not a good reviewer for either https://codereview.chromium.org/2015433003/diff/240001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/240001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:44: MediaMetadata::SanitizeArtwork(artwork); should we kill the renderer if the artwork isn't sane at this point? https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... File content/public/common/media_metadata.cc (right): https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... content/public/common/media_metadata.cc:35: sanitized_artwork.src = artwork.src; should we put some length limit on the url? https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... content/public/common/media_metadata.cc:43: return sanitized_artwork; can you avoid creating a copy of the artwork if it's actually sane? https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... content/public/common/media_metadata.h:32: static base::Optional<Artwork> SanitizeArtwork(const Artwork& artwork); why is this in public? It's only used from with content
zqzhang@chromium.org changed reviewers: + dalecurtis@chromium.org - sievers@chromium.org
+dalecurtis@ to review media-related changes.
Seems like a lot of size parsing, unpacking, and repacking. Is there a more efficient route? https://codereview.chromium.org/2015433003/diff/240001/content/common/android... File content/common/android/media_metadata_android.cc (right): https://codereview.chromium.org/2015433003/diff/240001/content/common/android... content/common/android/media_metadata_android.cc:50: ScopedJavaLocalRef<jintArray> j_sizes( How many sizes do you expect there to be? Should you instead pass a jintArray output into GetFlattenedSizeArray(). Or if there are only a couple sizes, why bother with an array at all?
Patchset #11 (id:260001) has been deleted
Patchset #12 (id:300001) has been deleted
Patchset #11 (id:280001) has been deleted
PTAL. Addressed/replied jochen@ and dalecurtis@'s comments. mlamouri@, can you PTAL at the added tests? > the CL description says that something got completed - so where are the tests? Thanks, the tests are added, and caught several bugs in the CL :) > This should be reviewed by somebody in media/OWNERS and somebody that owns the > java bits btw, I'm not a good reviewer for either > OK, the problem is that the CL is quite media-related, but many files need approval from content/ owners. So probably I should come back to you for when the media/ reviewers are happy. https://codereview.chromium.org/2015433003/diff/240001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/240001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:44: MediaMetadata::SanitizeArtwork(artwork); On 2016/06/27 08:12:40, jochen wrote: > should we kill the renderer if the artwork isn't sane at this point? Yes, the logic is already in line 49. https://codereview.chromium.org/2015433003/diff/240001/content/common/android... File content/common/android/media_metadata_android.cc (right): https://codereview.chromium.org/2015433003/diff/240001/content/common/android... content/common/android/media_metadata_android.cc:50: ScopedJavaLocalRef<jintArray> j_sizes( On 2016/06/27 17:49:02, DaleCurtis wrote: > How many sizes do you expect there to be? Should you instead pass a jintArray > output into GetFlattenedSizeArray(). Or if there are only a couple sizes, why > bother with an array at all? Actually the array could be of any length. Did you mean letting GetFlattenedSizeArray() return a jintArray directly (i.e. put the ToJavaIntArray conversion inside GetFlattenedSizeArray)? About your concern for parsing, packing and unpacking: We parse only once from string to vector<gfx::Size>, and then we need to plumb it to be accessed in chrome Java. So the IPC calls are unavoidable, and packing/unpacking is for passing sizes through JNI. I think most of them are unavoidable. Do you have better ideas? https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... File content/public/common/media_metadata.cc (right): https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... content/public/common/media_metadata.cc:35: sanitized_artwork.src = artwork.src; On 2016/06/27 08:12:40, jochen wrote: > should we put some length limit on the url? Limiting to 4K, maybe palmer@ can have more comments? https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... content/public/common/media_metadata.cc:43: return sanitized_artwork; On 2016/06/27 08:12:40, jochen wrote: > can you avoid creating a copy of the artwork if it's actually sane? Done. Early-returning if it's sane. https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/240001/content/public/common/... content/public/common/media_metadata.h:32: static base::Optional<Artwork> SanitizeArtwork(const Artwork& artwork); On 2016/06/27 08:12:40, jochen wrote: > why is this in public? It's only used from with content The MediaMetadata class is used both in content/ and chrome/. Did you mean moving this method to somewhere else in content/common/?
https://codereview.chromium.org/2015433003/diff/240001/content/common/android... File content/common/android/media_metadata_android.cc (right): https://codereview.chromium.org/2015433003/diff/240001/content/common/android... content/common/android/media_metadata_android.cc:50: ScopedJavaLocalRef<jintArray> j_sizes( On 2016/06/28 at 13:57:00, Zhiqiang Zhang wrote: > On 2016/06/27 17:49:02, DaleCurtis wrote: > > How many sizes do you expect there to be? Should you instead pass a jintArray > > output into GetFlattenedSizeArray(). Or if there are only a couple sizes, why > > bother with an array at all? > > Actually the array could be of any length. Did you mean letting GetFlattenedSizeArray() return a jintArray directly (i.e. put the ToJavaIntArray conversion inside GetFlattenedSizeArray)? > > About your concern for parsing, packing and unpacking: > We parse only once from string to vector<gfx::Size>, and then we need to plumb it to be accessed in chrome Java. So the IPC calls are unavoidable, and packing/unpacking is for passing sizes through JNI. I think most of them are unavoidable. Do you have better ideas? Do you really mean any length or is there a cap? If not there should be :) Do you have a doc or some more details on exactly what these sizes are for? If you only parse once it doesn't really matter unless the array is going to be large. It wasn't clear to me that this is only done once though.
https://codereview.chromium.org/2015433003/diff/240001/content/common/android... File content/common/android/media_metadata_android.cc (right): https://codereview.chromium.org/2015433003/diff/240001/content/common/android... content/common/android/media_metadata_android.cc:50: ScopedJavaLocalRef<jintArray> j_sizes( On 2016/06/28 19:03:30, DaleCurtis wrote: > On 2016/06/28 at 13:57:00, Zhiqiang Zhang wrote: > > On 2016/06/27 17:49:02, DaleCurtis wrote: > > > How many sizes do you expect there to be? Should you instead pass a > jintArray > > > output into GetFlattenedSizeArray(). Or if there are only a couple sizes, > why > > > bother with an array at all? > > > > Actually the array could be of any length. Did you mean letting > GetFlattenedSizeArray() return a jintArray directly (i.e. put the ToJavaIntArray > conversion inside GetFlattenedSizeArray)? > > > > About your concern for parsing, packing and unpacking: > > We parse only once from string to vector<gfx::Size>, and then we need to plumb > it to be accessed in chrome Java. So the IPC calls are unavoidable, and > packing/unpacking is for passing sizes through JNI. I think most of them are > unavoidable. Do you have better ideas? > > Do you really mean any length or is there a cap? If not there should be :) Do > you have a doc or some more details on exactly what these sizes are for? If you > only parse once it doesn't really matter unless the array is going to be large. > It wasn't clear to me that this is only done once though. The length does not have limit in Blink, but we will shrink the vector if it's too large in sanitizing. Sorry I don't have that doc yet. I'll write one tomorrow. Should've done this earlier to reduce your reviewing effort.
On 2016/06/28 19:03:30, DaleCurtis wrote: > Do you have a doc or some more details on exactly what these sizes are for? If you > only parse once it doesn't really matter unless the array is going to be large. > It wasn't clear to me that this is only done once though. Just finished the doc, check here (shared with chromium access): https://drive.google.com/open?id=16eyzKZlyF6EU5V4usLFV56FLSXXJNjAtVrk5G19lhK8 Hope it will help you do the review.
Thanks for the doc! https://codereview.chromium.org/2015433003/diff/320001/content/browser/media/... File content/browser/media/android/browser_media_session_manager_browsertest.cc (right): https://codereview.chromium.org/2015433003/diff/320001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:17: #if defined(OS_ANDROID) Should we just not include this file for non-android android? https://codereview.chromium.org/2015433003/diff/320001/content/public/common/... File content/public/common/media_metadata.cc (right): https://codereview.chromium.org/2015433003/diff/320001/content/public/common/... content/public/common/media_metadata.cc:40: base::Optional<MediaMetadata::Artwork> MediaMetadata::SanitizeArtwork( What's the point of sanitizing the artwork? Why not just fail if invalid options are used? https://codereview.chromium.org/2015433003/diff/320001/content/public/common/... content/public/common/media_metadata.cc:50: artwork.sizes.size() <= kMaxNumberOfArtworkSizes) Needs {} https://codereview.chromium.org/2015433003/diff/320001/content/public/common/... content/public/common/media_metadata.cc:60: for (size_t i = 0; {} https://codereview.chromium.org/2015433003/diff/320001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/320001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:83: if (ipc_metadata.artwork.size() >= MediaMetadata::kMaxNumberOfArtworkImages) Is this necessary on the renderer side? I.e. it seems to assume an untrusted browser?
PTAL. Addressed/replied Dale's comments. https://codereview.chromium.org/2015433003/diff/320001/content/browser/media/... File content/browser/media/android/browser_media_session_manager_browsertest.cc (right): https://codereview.chromium.org/2015433003/diff/320001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:17: #if defined(OS_ANDROID) On 2016/06/29 17:20:20, DaleCurtis wrote: > Should we just not include this file for non-android android? Oops, files with "android" in the pathnames are excluded on desktop. See build/filename_rules.gypi Removed this macro https://codereview.chromium.org/2015433003/diff/320001/content/public/common/... File content/public/common/media_metadata.cc (right): https://codereview.chromium.org/2015433003/diff/320001/content/public/common/... content/public/common/media_metadata.cc:40: base::Optional<MediaMetadata::Artwork> MediaMetadata::SanitizeArtwork( On 2016/06/29 17:20:20, DaleCurtis wrote: > What's the point of sanitizing the artwork? Why not just fail if invalid options > are used? The artwork come from blink, and can be anything, so we sanitize it before sending to the browser through IPC. Sometimes it may be legal but improper to be sent through IPC (e.g. src is too long), so I think we should not fail. https://codereview.chromium.org/2015433003/diff/320001/content/public/common/... content/public/common/media_metadata.cc:50: artwork.sizes.size() <= kMaxNumberOfArtworkSizes) On 2016/06/29 17:20:20, DaleCurtis wrote: > Needs {} Done. https://codereview.chromium.org/2015433003/diff/320001/content/public/common/... content/public/common/media_metadata.cc:60: for (size_t i = 0; On 2016/06/29 17:20:20, DaleCurtis wrote: > {} Done. https://codereview.chromium.org/2015433003/diff/320001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/320001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:83: if (ipc_metadata.artwork.size() >= MediaMetadata::kMaxNumberOfArtworkImages) On 2016/06/29 17:20:20, DaleCurtis wrote: > Is this necessary on the renderer side? I.e. it seems to assume an untrusted > browser? I think we should. Otherwise the page may set a super long list of artwork, and we don't want to send a message that long through IPC. https://codereview.chromium.org/2015433003/diff/340001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/340001/content/public/common/... content/public/common/media_metadata.h:34: static base::Optional<Artwork> SanitizeArtwork(const Artwork& artwork); For jochen@, copying previous comments here: On 2016/06/27 08:12:40, jochen wrote: > why is this in public? It's only used from with content The MediaMetadata class is used both in content/ and chrome/. Did you mean moving this method to somewhere else in content/common/? BTW, I think it's better to move all MediaMetadata sanitizing in one place. Now some part is in SanitizeArtwork(), and some part is in RendererMediaSessionManager/BrowserMediaSessionManager. Also we could have a method for checking MediaMetadata is OK before sanitizing it so we can avoid making another copy. Does this sound good to you?
On 2016/06/23 12:44:47, Mounir Lamouri wrote: > On 2016/06/22 at 22:43:18, palmer wrote: > > """ > > > If that scenario is plausible, we should not allow file: URLs, or at least > not > > > all file URLs. If the scenario is implausible, can you explain what stops it > > > from happening? Thanks! > > > > The image is fetched via WebContents::DownloadImage(), which basically passes > > the URL to the renderer, and eventually the request is initiated in blink, > where > > regular security checks should apply I think? > > """ > > > > |WebContentsImpl::DownloadImage| is in > content/browser/web_contents/web_contents_impl.cc, and so would seem to happen > on the browser process' authority. That suggests that indeed, it could be > possible for a renderer to get access to a file: URL that it should be able to. > (Test it to be sure.) > > WebContentsImpl::DownloadImage ends up here: > https://cs.chromium.org/chromium/src/content/renderer/image_downloader/image_... > > Adding tests sound reasonable, though, I'm not sure how extensively we should > test the behaviour of another component (ie. Image Downloader). I think it's reasonable to make sure it meets your needs; 1 test would be sufficient here.
media/ lgtm though I think you probably want to take a strong stance and forbid invalid metadata (i.e. soft-fail renderer side if sanity fails, and if it still fails on browser side nuke the renderer as jochen@ suggests) rather than truncating it in unexpected ways (unless that is defined by the spec somewhere). https://codereview.chromium.org/2015433003/diff/320001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/2015433003/diff/320001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:83: if (ipc_metadata.artwork.size() >= MediaMetadata::kMaxNumberOfArtworkImages) On 2016/06/29 at 19:16:13, Zhiqiang Zhang wrote: > On 2016/06/29 17:20:20, DaleCurtis wrote: > > Is this necessary on the renderer side? I.e. it seems to assume an untrusted > > browser? > > I think we should. Otherwise the page may set a super long list of artwork, and we don't want to send a message that long through IPC. Ah, this is sending to the browser, not receiving. Gotcha.
On 2016/07/01 at 00:01:04, dalecurtis wrote: > media/ lgtm though I think you probably want to take a strong stance and forbid invalid metadata (i.e. soft-fail renderer side if sanity fails, and if it still fails on browser side nuke the renderer as jochen@ suggests) rather than truncating it in unexpected ways (unless that is defined by the spec somewhere). Not sure what you mean by "soft-fail" but I think if we clean out things in the renderer, it's fine to kill the renderer when the browser process doesn't receive clean messages.
https://codereview.chromium.org/2015433003/diff/340001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/340001/content/public/common/... content/public/common/media_metadata.h:34: static base::Optional<Artwork> SanitizeArtwork(const Artwork& artwork); On 2016/06/29 at 19:16:14, Zhiqiang Zhang wrote: > For jochen@, copying previous comments here: > > On 2016/06/27 08:12:40, jochen wrote: > > why is this in public? It's only used from with content > > The MediaMetadata class is used both in content/ and chrome/. Did you mean > moving this method to somewhere else in content/common/? > > BTW, I think it's better to move all MediaMetadata sanitizing in one place. Now some part is in SanitizeArtwork(), and some part is in RendererMediaSessionManager/BrowserMediaSessionManager. Also we could have a method for checking MediaMetadata is OK before sanitizing it so we can avoid making another copy. Does this sound good to you? unifying this sounds good, yes
On 2016/07/01 at 10:47:33, mlamouri wrote: > On 2016/07/01 at 00:01:04, dalecurtis wrote: > > media/ lgtm though I think you probably want to take a strong stance and forbid invalid metadata (i.e. soft-fail renderer side if sanity fails, and if it still fails on browser side nuke the renderer as jochen@ suggests) rather than truncating it in unexpected ways (unless that is defined by the spec somewhere). > > Not sure what you mean by "soft-fail" but I think if we clean out things in the renderer, it's fine to kill the renderer when the browser process doesn't receive clean messages. I meant: to return some exception or other page-visible error that tells the page what went wrong.
On 2016/07/01 17:28:38, DaleCurtis wrote: > On 2016/07/01 at 10:47:33, mlamouri wrote: > > On 2016/07/01 at 00:01:04, dalecurtis wrote: > > > media/ lgtm though I think you probably want to take a strong stance and > forbid invalid metadata (i.e. soft-fail renderer side if sanity fails, and if it > still fails on browser side nuke the renderer as jochen@ suggests) rather than > truncating it in unexpected ways (unless that is defined by the spec somewhere). > > > > Not sure what you mean by "soft-fail" but I think if we clean out things in > the renderer, it's fine to kill the renderer when the browser process doesn't > receive clean messages. > > I meant: to return some exception or other page-visible error that tells the > page what went wrong. I got your point. Maybe we could print console warnings when we metadata has problems. Will add a TODO for this :)
A warning is okay, but is still more work than just throwing an error :)
PTAL. * Moved artwork sanity check and sanitize methods to content/common/media/ * Wrote some tests for WebContents::DownloadImage() in a separate CL (left comments for palmer@) https://codereview.chromium.org/2015433003/diff/340001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/340001/content/public/common/... content/public/common/media_metadata.h:34: static base::Optional<Artwork> SanitizeArtwork(const Artwork& artwork); On 2016/07/01 14:18:56, jochen wrote: > On 2016/06/29 at 19:16:14, Zhiqiang Zhang wrote: > > For jochen@, copying previous comments here: > > > > On 2016/06/27 08:12:40, jochen wrote: > > > why is this in public? It's only used from with content > > > > The MediaMetadata class is used both in content/ and chrome/. Did you mean > > moving this method to somewhere else in content/common/? > > > > BTW, I think it's better to move all MediaMetadata sanitizing in one place. > Now some part is in SanitizeArtwork(), and some part is in > RendererMediaSessionManager/BrowserMediaSessionManager. Also we could have a > method for checking MediaMetadata is OK before sanitizing it so we can avoid > making another copy. Does this sound good to you? > unifying this sounds good, yes Done. See content/common/media/media_metadata_sanitizer.h https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... File content/common/media/media_metadata_sanitizer.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:36: if (!src.SchemeIsHTTPOrHTTPS() && src.scheme() != url::kDataScheme) palmer@, actually I found that WebContents::DownloadImage() does not support file urls. Since it sends real HTTP requests. I wrote some tests in https://codereview.chromium.org/2113893005/. Also, DownloadImage() supports data URIs so increased the artwork src limit. Could we land this CL first and land the DownloadImage() tests later?
zqzhang@chromium.org changed reviewers: + dcheng@chromium.org
Hi dcheng, can you look at the IPC messages since palmer is OOO? I've addressed all palmer's comments.
mlamouri@chromium.org changed reviewers: + mkwst@chromium.org
+mkwst@ for EMEA IPC review
https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... File content/browser/media/android/browser_media_session_manager_browsertest.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:42: " src: 'http://foo.com/bar.jpg', type: 'image/jpeg', sizes: 'any'" Can we add an explicit test here for trying to load, say, a file: URL? https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... File content/browser/media/android/media_web_contents_observer_android.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... content/browser/media/android/media_web_contents_observer_android.cc:84: RenderFrameHost* render_frame_host, BrowserMediaSessionManager* manager) { Nit: please pass |manager| through as a std::unique_ptr. https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... File content/common/media/media_metadata_sanitizer.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:21: const size_t kMaxArtworkSrcLength = 64 * 1024; FWIW, there's also a kMaxURLChars (which is 2MB). I don't know what a reasonable limit for media metadata is though, so if 64KB is a reasonable upper bound, that seems fine with me. https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:34: if (!src.is_valid() || !src.IsStandard()) The standard scheme check seems redundant in conjunction with the HTTPOrHTTPS check below. https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:103: if (!CheckArtworkSrcSanity(artwork.src)) CheckArtworkSanity already does this check. https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:109: if (sanitized_metadata.artwork.size() > kMaxNumberOfArtworkImages) This check should be ==: otherwise, you can create sanitized media metadata that won't pass the sanity check. https://codereview.chromium.org/2015433003/diff/380001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:55: std::back_inserter(artwork.sizes)); It's probably a bit more common to see this written as: artwork.sizes.insert(artwork.sizes.end(), web_sizes.begin(), web_sizes.end()); Otherwise, you actually need to include two headers (iterator and algorithm)
lgtm
Patchset #15 (id:400001) has been deleted
PTAL, addressed dcheng's comments w/ replies. https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... File content/browser/media/android/browser_media_session_manager_browsertest.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:42: " src: 'http://foo.com/bar.jpg', type: 'image/jpeg', sizes: 'any'" On 2016/07/05 02:36:55, dcheng wrote: > Can we add an explicit test here for trying to load, say, a file: URL? Done. https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... File content/browser/media/android/media_web_contents_observer_android.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... content/browser/media/android/media_web_contents_observer_android.cc:84: RenderFrameHost* render_frame_host, BrowserMediaSessionManager* manager) { On 2016/07/05 02:36:55, dcheng wrote: > Nit: please pass |manager| through as a std::unique_ptr. Done. Though I think we are putting too much magic code here for test. We need base::WrapUnique on the call site, and need std::forward in this method since media_session_managers_.set() uses copy ctor which is removed in std::unique_ptr. https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... File content/common/media/media_metadata_sanitizer.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:21: const size_t kMaxArtworkSrcLength = 64 * 1024; On 2016/07/05 02:36:55, dcheng wrote: > FWIW, there's also a kMaxURLChars (which is 2MB). I don't know what a reasonable > limit for media metadata is though, so if 64KB is a reasonable upper bound, that > seems fine with me. Let's use kMaxURLChars. https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:34: if (!src.is_valid() || !src.IsStandard()) On 2016/07/05 02:36:55, dcheng wrote: > The standard scheme check seems redundant in conjunction with the HTTPOrHTTPS > check below. Done. https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:103: if (!CheckArtworkSrcSanity(artwork.src)) On 2016/07/05 02:36:55, dcheng wrote: > CheckArtworkSanity already does this check. This check is needed here. If artwork has a good src but bad other fields (super long type/sizes), we will do sanitize it. If artwork has a bad src, then we just abandon it. https://codereview.chromium.org/2015433003/diff/380001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:109: if (sanitized_metadata.artwork.size() > kMaxNumberOfArtworkImages) On 2016/07/05 02:36:55, dcheng wrote: > This check should be ==: otherwise, you can create sanitized media metadata that > won't pass the sanity check. Done. https://codereview.chromium.org/2015433003/diff/380001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:55: std::back_inserter(artwork.sizes)); On 2016/07/05 02:36:55, dcheng wrote: > It's probably a bit more common to see this written as: > artwork.sizes.insert(artwork.sizes.end(), web_sizes.begin(), web_sizes.end()); > > Otherwise, you actually need to include two headers (iterator and algorithm) Done.
https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... File content/browser/media/android/media_web_contents_observer_android.cc (right): https://codereview.chromium.org/2015433003/diff/380001/content/browser/media/... content/browser/media/android/media_web_contents_observer_android.cc:84: RenderFrameHost* render_frame_host, BrowserMediaSessionManager* manager) { On 2016/07/05 15:30:30, Zhiqiang Zhang wrote: > On 2016/07/05 02:36:55, dcheng wrote: > > Nit: please pass |manager| through as a std::unique_ptr. > > Done. Though I think we are putting too much magic code here for test. We need > base::WrapUnique on the call site, and need std::forward in this method since > media_session_managers_.set() uses copy ctor which is removed in > std::unique_ptr. If a pointer param is transferring ownership, it should be passed a unique_ptr, regardless of whether or not it's in a test. The tradeoff is a bit more typing to be explicit about ownership, but it turns out to be quite helpful in seeing how ownership flows through a system. https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... File content/browser/media/android/browser_media_session_manager_browsertest.cc (right): https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:107: base::WrapUnique(browser_media_session_manager_); A slightly shorter way to write this: std::unique_ptr<BrowserMediaSessionManager> manager(new MockBrowserMediaSessionManager(...)); browser_media_session_manager_ = manager.get(); MWCOA::FromWebContents(...)->SetMediaSessionManagerForTest(..., std::move(manager)); https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:111: web_contents_->GetMainFrame(), manager_ptr); std::move(manager_ptr) (and make the function param pass by value) https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:204: EXPECT_CALL(*browser_media_session_manager_, OnSetMetadata(_, Ne(expected))) I'm not sure I understand these expectations. Why do we both expect that we'll call it once with the expected data and that we'll never call it with not-expected data? https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... File content/browser/media/android/media_web_contents_observer_android.cc (right): https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... content/browser/media/android/media_web_contents_observer_android.cc:90: std::forward<std::unique_ptr<BrowserMediaSessionManager>>(manager)); This should just be std::move(manager) (once the arg is passed by value) https://codereview.chromium.org/2015433003/diff/420001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/2015433003/diff/420001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:7: #include <iterator> Nit: this include is no longer needed.
PTAL. Addressed dcheng's comments. https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... File content/browser/media/android/browser_media_session_manager_browsertest.cc (right): https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:107: base::WrapUnique(browser_media_session_manager_); On 2016/07/06 02:34:15, dcheng wrote: > A slightly shorter way to write this: > > std::unique_ptr<BrowserMediaSessionManager> manager(new > MockBrowserMediaSessionManager(...)); > browser_media_session_manager_ = manager.get(); > MWCOA::FromWebContents(...)->SetMediaSessionManagerForTest(..., > std::move(manager)); Done for all the std::unique_ptr-related comments :) https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:111: web_contents_->GetMainFrame(), manager_ptr); On 2016/07/06 02:34:15, dcheng wrote: > std::move(manager_ptr) (and make the function param pass by value) Done. https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... content/browser/media/android/browser_media_session_manager_browsertest.cc:204: EXPECT_CALL(*browser_media_session_manager_, OnSetMetadata(_, Ne(expected))) On 2016/07/06 02:34:15, dcheng wrote: > I'm not sure I understand these expectations. Why do we both expect that we'll > call it once with the expected data and that we'll never call it with > not-expected data? Yeah, this expect indeed don't make sense. Removed. https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... File content/browser/media/android/media_web_contents_observer_android.cc (right): https://codereview.chromium.org/2015433003/diff/420001/content/browser/media/... content/browser/media/android/media_web_contents_observer_android.cc:90: std::forward<std::unique_ptr<BrowserMediaSessionManager>>(manager)); On 2016/07/06 02:34:15, dcheng wrote: > This should just be std::move(manager) (once the arg is passed by value) Done. https://codereview.chromium.org/2015433003/diff/420001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/2015433003/diff/420001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:7: #include <iterator> On 2016/07/06 02:34:15, dcheng wrote: > Nit: this include is no longer needed. Done.
ipc lgtm with comments addressed https://codereview.chromium.org/2015433003/diff/440001/content/common/media/m... File content/common/media/media_metadata_sanitizer.cc (right): https://codereview.chromium.org/2015433003/diff/440001/content/common/media/m... content/common/media/media_metadata_sanitizer.cc:65: if (sanitized_artwork.sizes.size() > kMaxNumberOfArtworkSizes) Ditto: this should be == instead of > https://codereview.chromium.org/2015433003/diff/440001/content/public/common/... File content/public/common/media_metadata.h (right): https://codereview.chromium.org/2015433003/diff/440001/content/public/common/... content/public/common/media_metadata.h:10: #include "base/optional.h" Nit: remove unused include
\o/
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, dalecurtis@chromium.org, jochen@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2015433003/#ps460001 (title: "fixed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement MediaMetadata artwork in content This CL implements MediaMetadata artwork in the content layer, and completes the propagation of MediaMetadata from blink to the browser process. BUG=616411 ========== to ========== Implement MediaMetadata artwork in content This CL implements MediaMetadata artwork in the content layer, and completes the propagation of MediaMetadata from blink to the browser process. BUG=616411 Committed: https://crrev.com/5d2a9140bd85b681a0290719da33569db66d06cc Cr-Commit-Position: refs/heads/master@{#404125} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/5d2a9140bd85b681a0290719da33569db66d06cc Cr-Commit-Position: refs/heads/master@{#404125}
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Failed to apply the patch. |
