Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
408873004:
Fix for cross-origin video check for webgl on android (Closed)
|
Created:
6 years, 5 months ago by qinmin Modified:
6 years, 4 months ago Reviewers:
palmer, Ken Russell (switch to Gerrit), scherkus (not reviewing), dshwang, abarth-chromium CC:
chromium-reviews, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, jschuh, Robert Sesek, abarth-chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix for cross-origin video check for webgl
We landed a hack earlier to treat all video urls as cross-origin for an security issue.
That fix, however, breaks all webgl tests that has video in it.
This CL brings a proper fix to the original issue.
It uses a new API available in the android L preview System image.
For systems below K, the security impact is much less serious than the issue it caused.
As a result, we are reenabling video for webgl for systems <=K.
And the security issues on these systems should be resolved when devices upgrade.
BUG=358198
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288242
Patch Set 1 #
Total comments: 1
Patch Set 2 : pass allow_credentials to android mediaplayer #
Total comments: 2
Patch Set 3 : addressing kbr's comments #
Total comments: 15
Patch Set 4 : fix comments #Patch Set 5 : don't allow android mediaplayer to do any redirects after passing the final destination #Patch Set 6 : rebase #Patch Set 7 : rebase again after WMPA changes #Patch Set 8 : fix clang warnings #
Total comments: 2
Messages
Total messages: 51 (0 generated)
PTAL
I'd really like to see a version of this CL landed soon, but we need to make sure that the underlying issue will actually be fixed with the new API. https://codereview.chromium.org/408873004/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/408873004/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:153: headersMap.put("allow-cross-domain-redirect", "false"); Per our offline discussion I'm not sure that this new API is really sufficient. I think it may be necessary to teach Android's MediaPlayer about CORS-enabled fetches, and to be able to query the response headers to find out whether the server actually allowed access to the resource (through the Access-Control-Allow-Origin reponse header). Let's continue to discuss this issue offline.
lgtm https://codereview.chromium.org/408873004/diff/20001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/408873004/diff/20001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:234: // Check http://crbug.com/334204. We should retain this comment, and re-open issue 334204 and https://b.corp.google.com/issue?id=12573548, right?
Notes on this fix per offline discussion: 1) Need to rebase to top of tree. 2) For anonymous requests (no credentials), don't use the heavy hammer that was used before of always setting single_origin to false. This will allow at least some sites to use videoElement.crossOrigin = '' to successfully load their videos into WebGL. 3) Use the final redirected-to URL, not the initial URL. 4) Put back the security fix, modified, for pre-L releases.
On 2014/07/29 22:45:47, Ken Russell wrote: > Notes on this fix per offline discussion: > > 1) Need to rebase to top of tree. > 2) For anonymous requests (no credentials), don't use the heavy hammer that was > used before of always setting single_origin to false. This will allow at least > some sites to use videoElement.crossOrigin = '' to successfully load their > videos into WebGL. > 3) Use the final redirected-to URL, not the initial URL. > 4) Put back the security fix, modified, for pre-L releases. ... also some form of automated tests.
On 2014/07/29 23:15:49, scherkus wrote: > On 2014/07/29 22:45:47, Ken Russell wrote: > > Notes on this fix per offline discussion: > > > > 1) Need to rebase to top of tree. > > 2) For anonymous requests (no credentials), don't use the heavy hammer that > was > > used before of always setting single_origin to false. This will allow at least > > some sites to use videoElement.crossOrigin = '' to successfully load their > > videos into WebGL. > > 3) Use the final redirected-to URL, not the initial URL. > > 4) Put back the security fix, modified, for pre-L releases. > > ... also some form of automated tests. Yes. There are already some layout tests for cross-origin video in LayoutTests/http/tests/security/video-cross-origin-readback.html but we should triple-check that they're sufficient and working.
rebased to the top of the tree. Changed the CL according to suggestions 1-4 from kbr https://codereview.chromium.org/408873004/diff/20001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (left): https://codereview.chromium.org/408873004/diff/20001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:234: // Check http://crbug.com/334204. On 2014/07/22 19:19:47, Chromium Palmer wrote: > We should retain this comment, and re-open issue 334204 and > https://b.corp.google.com/issue?id=12573548, right? moved it to another place
This looks like a good step forward. Multiple tests are needed: - Manual testing with many video sites on L - Automated testing of video -> WebGL uploads, with cookies, with and without redirects, pre-L (should still fail) - Automated testing of video -> WebGL uploads without cookies, with and without redirects, server approves Access-Control-Allow-Origin, pre-L (should now work) - Automated testing of video -> WebGL uploads without cookies, with and without redirects, server denies Access-Control-Allow-Origin, pre-L (should still fail) - Automated testing of video -> WebGL uploads both with and without cookies, both with and without redirects, both approving and denying Access-Control-Allow-Origin, on L Some of these tests can be done against 2D canvas, seeing whether getImageData succeeds. Let's talk offline about putting these tests in place. https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... File content/renderer/media/android/media_info_loader.cc (right): https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:98: single_origin_ = url_.GetOrigin() == GURL(newRequest.url()).GetOrigin(); Are we 100% sure that this (preexisting) code computes the right thing? For example, if a redirect happens during an anonymous CORS fetch, and both the original and destination server respond to the Origin: header with the right Access-Control-Allow-Origin: header, the resulting redirect will be considered same-origin. https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:102: allow_stored_credentials_ = newRequest.allowStoredCredentials(); Are you 100% sure that these pieces of state are being updated correctly while following redirects? https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:199: allow_stored_credentials_); Are we 100% sure that these are the right pieces of information to give to the ready callback? https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:259: const GURL& fist_party_for_cookies, typo: first_party_for_cookies https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:546: // destination, we will honor the return value from "Only if the HTTP request was made without credentials, ..." https://codereview.chromium.org/408873004/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/408873004/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:149: // the security impact is incomparable to the webgl issue it causes. Eventually, the This comment is a little out of date now. Instead, say that for devices below K, if the HTTP request for the media was made anonymously (no cookies) then it may be considered same-origin. Note that if the server rejects the request (no "Access-Control-Allow-Origin: *" response header) we must not consider it same-origin. https://codereview.chromium.org/408873004/diff/40001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/408873004/diff/40001/media/base/android/media... media/base/android/media_player_bridge.cc:91: if (!allow_credentials_) { Please comment this block indicating why the short-circuit is OK.
Andrew, since you are the author of buffered_resource_loader.cc, would you please take a look at the questions in media_info_loader.cc? https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... File content/renderer/media/android/media_info_loader.cc (right): https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:98: single_origin_ = url_.GetOrigin() == GURL(newRequest.url()).GetOrigin(); hmm... I am not very familiar with the CORS stuff. Since this class is mostly cloned from buffered_resource_loader.cc, scherkus@, do you have any idea whether the implementation there and here is correct? Same for the questions below On 2014/07/30 20:44:05, Ken Russell wrote: > Are we 100% sure that this (preexisting) code computes the right thing? For > example, if a redirect happens during an anonymous CORS fetch, and both the > original and destination server respond to the Origin: header with the right > Access-Control-Allow-Origin: header, the resulting redirect will be considered > same-origin. https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:259: const GURL& fist_party_for_cookies, On 2014/07/30 20:44:05, Ken Russell wrote: > typo: first_party_for_cookies Done. https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.cc:546: // destination, we will honor the return value from On 2014/07/30 20:44:05, Ken Russell wrote: > "Only if the HTTP request was made without credentials, ..." Done. https://codereview.chromium.org/408873004/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java (right): https://codereview.chromium.org/408873004/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/MediaPlayerBridge.java:149: // the security impact is incomparable to the webgl issue it causes. Eventually, the On 2014/07/30 20:44:05, Ken Russell wrote: > This comment is a little out of date now. Instead, say that for devices below K, > if the HTTP request for the media was made anonymously (no cookies) then it may > be considered same-origin. Note that if the server rejects the request (no > "Access-Control-Allow-Origin: *" response header) we must not consider it > same-origin. Done. https://codereview.chromium.org/408873004/diff/40001/media/base/android/media... File media/base/android/media_player_bridge.cc (right): https://codereview.chromium.org/408873004/diff/40001/media/base/android/media... media/base/android/media_player_bridge.cc:91: if (!allow_credentials_) { On 2014/07/30 20:44:05, Ken Russell wrote: > Please comment this block indicating why the short-circuit is OK. Done.
It sounds from kbr's questions that this solution might be too restrictive in certain circumstances. If that turns out to be true, I'll take another look after that round of fixes. https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... File content/renderer/media/android/media_info_loader.h (right): https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/media_info_loader.h:51: // credentials needs to be sent to the final destination. Typo: "credentials need" https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/webmediaplayer_android.h:262: const GURL& fist_party_for_cookies, Make sure to fix this typo everywhere.
https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... File content/renderer/media/android/media_info_loader.cc (right): https://codereview.chromium.org/408873004/diff/40001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:98: single_origin_ = url_.GetOrigin() == GURL(newRequest.url()).GetOrigin(); On 2014/07/30 23:46:05, qinmin wrote: > hmm... I am not very familiar with the CORS stuff. Since this class is mostly > cloned from buffered_resource_loader.cc, scherkus@, do you have any idea whether > the implementation there and here is correct? Same for the questions below > > > On 2014/07/30 20:44:05, Ken Russell wrote: > > Are we 100% sure that this (preexisting) code computes the right thing? For > > example, if a redirect happens during an anonymous CORS fetch, and both the > > original and destination server respond to the Origin: header with the right > > Access-Control-Allow-Origin: header, the resulting redirect will be considered > > same-origin. > kbr: I think you're right. I thought that this was handled higher up in Blink (e.g., in HTMLVideoElement::willTaintOrigin() [1]), but it doesn't appear that's the case. I also did some digging around in the bug tracker and found http://crbug.com/390889 -- I think both this class and BufferedResourceLoader need fixes. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
ok, I was able to get the layout test running locally: before patching my CL, the following 3 video related tests are failing http/tests/security/video-cross-origin-readback.html http/tests/security/video-cross-origin-via-dom.html http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html With my patch, the first 2 tests passes as the user credential is not allowed. However, the last test still fails. But I think that is not related to this change.
Thanks for for working on this Min! Could you also please re-enable the disabled video tests in content/test/gpu/gpu_tests/webgl_conformance_expectations.py and check that they pass with this patch?
Min: please post your results of testing various video sites with this patch. Sami: Min and I just ran some of the affected WebGL conformance tests. While it definitely improves the situation -- parts of the video tests now pass -- there seem to be some issues with the supported codecs in his Chromium build. I think we should land this patch and test with official Canary builds, and then try to re-enable these tests. (Note that there are no bots on the waterfall which would allow the tests to be re-enabled yet.)
Here is a list of websites i tested with my patch, and i found no issues with any one of them. Youtube, vimeo, nytimes, cnn.com, espn.com, twitch.tv, kotaku.com, yahoo.com On 2014/08/06 21:46:50, Ken Russell wrote: > Min: please post your results of testing various video sites with this patch. > > Sami: Min and I just ran some of the affected WebGL conformance tests. While it > definitely improves the situation -- parts of the video tests now pass -- there > seem to be some issues with the supported codecs in his Chromium build. I think > we should land this patch and test with official Canary builds, and then try to > re-enable these tests. (Note that there are no bots on the waterfall which would > allow the tests to be re-enabled yet.)
LGTM There are further issues to be fixed in this area of the code -- like proper redirect handling -- but this CL definitely fixes the worst problem and provides a workaround for web sites using video and WebGL. Let's land this now.
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/408873004/70001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38260) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43468) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38327) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/408873004/90001
The CQ bit was unchecked by qinmin@chromium.org
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/408873004/110001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38640) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43841) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38647) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43848) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Needs to be rebased.
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/408873004/130001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/408873004/150001
Message was sent while issue was closed.
Change committed as 288242
Message was sent while issue was closed.
https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:625: kSDKVersionToSupportSecurityOriginCheck; This method allows SSO for systems >=L although the description said we are reenabling video for webgl for systems <=K. I guess we should replace >= to <, right? Video on WebGL still is in trouble on systems <= K. e.g. http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html
Message was sent while issue was closed.
On 2014/08/11 08:01:33, dshwang wrote: > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > content/renderer/media/android/webmediaplayer_android.cc:625: > kSDKVersionToSupportSecurityOriginCheck; > This method allows SSO for systems >=L although the description said we are > reenabling video for webgl for systems <=K. > I guess we should replace >= to <, right? > > Video on WebGL still is in trouble on systems <= K. e.g. > http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html for systems < K, if credentials are not allowed (e. g Access-Control-Allow-Credentials header), webgl is also enabled.
Message was sent while issue was closed.
On 2014/08/11 14:05:10, qinmin wrote: > On 2014/08/11 08:01:33, dshwang wrote: > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > content/renderer/media/android/webmediaplayer_android.cc:625: > > kSDKVersionToSupportSecurityOriginCheck; > > This method allows SSO for systems >=L although the description said we are > > reenabling video for webgl for systems <=K. > > I guess we should replace >= to <, right? > > > > Video on WebGL still is in trouble on systems <= K. e.g. > > http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html > > for systems < K, if credentials are not allowed (e. g > Access-Control-Allow-Credentials header), webgl is also enabled. That's not different from systems >= L also. That means that system < K is more restricted, though the description said less restricted, right? What system do Chromium Android bots use? L or K e.g. http://build.chromium.org/p/chromium.perf/builders/Android%20Nexus5%20Perf
Message was sent while issue was closed.
On 2014/08/11 15:53:06, dshwang wrote: > On 2014/08/11 14:05:10, qinmin wrote: > > On 2014/08/11 08:01:33, dshwang wrote: > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > content/renderer/media/android/webmediaplayer_android.cc:625: > > > kSDKVersionToSupportSecurityOriginCheck; > > > This method allows SSO for systems >=L although the description said we are > > > reenabling video for webgl for systems <=K. > > > I guess we should replace >= to <, right? > > > > > > Video on WebGL still is in trouble on systems <= K. e.g. > > > http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html > > > > for systems < K, if credentials are not allowed (e. g > > Access-Control-Allow-Credentials header), webgl is also enabled. > > That's not different from systems >= L also. That means that system < K is more > restricted, though the description said less restricted, right? > What system do Chromium Android bots use? L or K e.g. > http://build.chromium.org/p/chromium.perf/builders/Android%20Nexus5%20Perf all the bots are on K now
Message was sent while issue was closed.
On 2014/08/11 15:53:06, dshwang wrote: > On 2014/08/11 14:05:10, qinmin wrote: > > On 2014/08/11 08:01:33, dshwang wrote: > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > content/renderer/media/android/webmediaplayer_android.cc:625: > > > kSDKVersionToSupportSecurityOriginCheck; > > > This method allows SSO for systems >=L although the description said we are > > > reenabling video for webgl for systems <=K. > > > I guess we should replace >= to <, right? > > > > > > Video on WebGL still is in trouble on systems <= K. e.g. > > > http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html > > > > for systems < K, if credentials are not allowed (e. g > > Access-Control-Allow-Credentials header), webgl is also enabled. > > That's not different from systems >= L also. That means that system < K is more > restricted, though the description said less restricted, right? > What system do Chromium Android bots use? L or K e.g. > http://build.chromium.org/p/chromium.perf/builders/Android%20Nexus5%20Perf all the bots are on K now
Message was sent while issue was closed.
On 2014/08/15 03:07:30, qinmin wrote: > On 2014/08/11 15:53:06, dshwang wrote: > > On 2014/08/11 14:05:10, qinmin wrote: > > > On 2014/08/11 08:01:33, dshwang wrote: > > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > > content/renderer/media/android/webmediaplayer_android.cc:625: > > > > kSDKVersionToSupportSecurityOriginCheck; > > > > This method allows SSO for systems >=L although the description said we > are > > > > reenabling video for webgl for systems <=K. > > > > I guess we should replace >= to <, right? > > > > > > > > Video on WebGL still is in trouble on systems <= K. e.g. > > > > > http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html > > > > > > for systems < K, if credentials are not allowed (e. g > > > Access-Control-Allow-Credentials header), webgl is also enabled. > > > > That's not different from systems >= L also. That means that system < K is > more > > restricted, though the description said less restricted, right? > > What system do Chromium Android bots use? L or K e.g. > > http://build.chromium.org/p/chromium.perf/builders/Android%20Nexus5%20Perf > > all the bots are on K now or below K (but >= ICS)
Message was sent while issue was closed.
On 2014/08/15 03:08:10, qinmin wrote: > On 2014/08/15 03:07:30, qinmin wrote: > > On 2014/08/11 15:53:06, dshwang wrote: > > > On 2014/08/11 14:05:10, qinmin wrote: > > > > On 2014/08/11 08:01:33, dshwang wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > > > content/renderer/media/android/webmediaplayer_android.cc:625: > > > > > kSDKVersionToSupportSecurityOriginCheck; > > > > > This method allows SSO for systems >=L although the description said we > > are > > > > > reenabling video for webgl for systems <=K. > > > > > I guess we should replace >= to <, right? > > > > > > > > > > Video on WebGL still is in trouble on systems <= K. e.g. > > > > > > > http://browsertests.herokuapp.com/media/mdn_samples_webgl_sample8/index.html > > > > > > > > for systems < K, if credentials are not allowed (e. g > > > > Access-Control-Allow-Credentials header), webgl is also enabled. > > > > > > That's not different from systems >= L also. That means that system < K is > > more > > > restricted, though the description said less restricted, right? > > > What system do Chromium Android bots use? L or K e.g. > > > http://build.chromium.org/p/chromium.perf/builders/Android%20Nexus5%20Perf > > > > all the bots are on K now > > or below K (but >= ICS) Yes, the CL just make webGL less restricted on systems <K, so websites have a work around with the security issue.
Message was sent while issue was closed.
https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:625: kSDKVersionToSupportSecurityOriginCheck; > > Yes, the CL just make webGL less restricted on systems <K, so websites have a > work around with the security issue. This CL makes WebGL more restricted on systems <K, not less restricted, because this line; return system >= L as hasSingleSecurityOrigin So there is not any difference for system <= K compared to before-this-CL
Message was sent while issue was closed.
On 2014/08/15 09:25:56, dshwang wrote: > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > content/renderer/media/android/webmediaplayer_android.cc:625: > kSDKVersionToSupportSecurityOriginCheck; > > > > Yes, the CL just make webGL less restricted on systems <K, so websites have a > > work around with the security issue. > > This CL makes WebGL more restricted on systems <K, not less restricted, because > this line; return system >= L as hasSingleSecurityOrigin > So there is not any difference for system <= K compared to before-this-CL before this CL, video painting is totally disabled for webGL.
Message was sent while issue was closed.
On 2014/08/15 14:15:29, qinmin wrote: > On 2014/08/15 09:25:56, dshwang wrote: > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > content/renderer/media/android/webmediaplayer_android.cc:625: > > kSDKVersionToSupportSecurityOriginCheck; > > > > > > Yes, the CL just make webGL less restricted on systems <K, so websites have > a > > > work around with the security issue. > > > > This CL makes WebGL more restricted on systems <K, not less restricted, > because > > this line; return system >= L as hasSingleSecurityOrigin > > So there is not any difference for system <= K compared to before-this-CL > > before this CL, video painting is totally disabled for webGL. And with this CL, if allow_stored_credentials is false, we will return true for single origin check for devices <=K
Message was sent while issue was closed.
On 2014/08/15 14:17:26, qinmin wrote: > On 2014/08/15 14:15:29, qinmin wrote: > > On 2014/08/15 09:25:56, dshwang wrote: > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > content/renderer/media/android/webmediaplayer_android.cc:625: > > > kSDKVersionToSupportSecurityOriginCheck; > > > > > > > > Yes, the CL just make webGL less restricted on systems <K, so websites > have > > a > > > > work around with the security issue. > > > > > > This CL makes WebGL more restricted on systems <K, not less restricted, > > because > > > this line; return system >= L as hasSingleSecurityOrigin > > > So there is not any difference for system <= K compared to before-this-CL > > > > before this CL, video painting is totally disabled for webGL. > > And with this CL, if allow_stored_credentials is false, we will return true for > single origin check for devices <=K Ok, thank you for answering. I asked it because Android K is still in trouble to draw video on webgl. e.g. http://www.costoso.net/seau/hermitcrab.html
Message was sent while issue was closed.
On 2014/08/15 14:23:08, dshwang wrote: > On 2014/08/15 14:17:26, qinmin wrote: > > On 2014/08/15 14:15:29, qinmin wrote: > > > On 2014/08/15 09:25:56, dshwang wrote: > > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > > content/renderer/media/android/webmediaplayer_android.cc:625: > > > > kSDKVersionToSupportSecurityOriginCheck; > > > > > > > > > > Yes, the CL just make webGL less restricted on systems <K, so websites > > have > > > a > > > > > work around with the security issue. > > > > > > > > This CL makes WebGL more restricted on systems <K, not less restricted, > > > because > > > > this line; return system >= L as hasSingleSecurityOrigin > > > > So there is not any difference for system <= K compared to before-this-CL > > > > > > before this CL, video painting is totally disabled for webGL. > > > > And with this CL, if allow_stored_credentials is false, we will return true > for > > single origin check for devices <=K > > Ok, thank you for answering. > I asked it because Android K is still in trouble to draw video on webgl. e.g. > http://www.costoso.net/seau/hermitcrab.html Yes, some pages will still fail because they don't allow anonymous access. You can try some samples on the Khronos webgl conformance test page, or https://www.khronos.org/registry/webgl/sdk/tests/extra/tex-image-with-video-t...
Message was sent while issue was closed.
On 2014/08/15 15:17:11, qinmin wrote: > On 2014/08/15 14:23:08, dshwang wrote: > > On 2014/08/15 14:17:26, qinmin wrote: > > > On 2014/08/15 14:15:29, qinmin wrote: > > > > On 2014/08/15 09:25:56, dshwang wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > > > File content/renderer/media/android/webmediaplayer_android.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/408873004/diff/150001/content/renderer/media/... > > > > > content/renderer/media/android/webmediaplayer_android.cc:625: > > > > > kSDKVersionToSupportSecurityOriginCheck; > > > > > > > > > > > > Yes, the CL just make webGL less restricted on systems <K, so websites > > > have > > > > a > > > > > > work around with the security issue. > > > > > > > > > > This CL makes WebGL more restricted on systems <K, not less restricted, > > > > because > > > > > this line; return system >= L as hasSingleSecurityOrigin > > > > > So there is not any difference for system <= K compared to > before-this-CL > > > > > > > > before this CL, video painting is totally disabled for webGL. > > > > > > And with this CL, if allow_stored_credentials is false, we will return true > > for > > > single origin check for devices <=K > > > > Ok, thank you for answering. > > I asked it because Android K is still in trouble to draw video on webgl. e.g. > > http://www.costoso.net/seau/hermitcrab.html > > Yes, some pages will still fail because they don't allow anonymous access. > You can try some samples on the Khronos webgl conformance test page, or > https://www.khronos.org/registry/webgl/sdk/tests/extra/tex-image-with-video-t... The workaround to use crossOrigin="anonymous" in the video tag to re-enable video uploads to WebGL is just that, a workaround. The Chrome implementation still won't pass the WebGL conformance tests until the release after K, where the new code path is taken. |
