Check origin before providing initData in encrypted event

Check origin before providing initData in encrypted event

BUG=418233
TEST=new tests pass

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189735

[jrummell, 2015-02-03 21:28:22]

jrummell@chromium.org changed reviewers:
+ ddorwin@chromium.org, sandersd@chromium.org

[jrummell, 2015-02-03 21:28:22]

jrummell@chromium.org changed required reviewers:
+ ddorwin@chromium.org

[jrummell, 2015-02-03 21:28:23]

PTAL.

[sandersd (OOO until July 31), 2015-02-03 22:31:59]

https://codereview.chromium.org/893123004/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right):

https://codereview.chromium.org/893123004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:51: static bool
canAccessData(HTMLMediaElement& element)
I would prefer that there is only one copy of this code. I think the best plan
is to rename this something more serious sounding (basically add Origin to the
method name), then replace HTMLVideoElement::wouldTaintOrigin() calls with calls
to this method.

[jrummell, 2015-02-04 23:28:37]

Updated to use common method.

https://codereview.chromium.org/893123004/diff/1/Source/modules/encryptedmedi...
File Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp (right):

https://codereview.chromium.org/893123004/diff/1/Source/modules/encryptedmedi...
Source/modules/encryptedmedia/HTMLMediaElementEncryptedMedia.cpp:51: static bool
canAccessData(HTMLMediaElement& element)
On 2015/02/03 22:31:59, sandersd wrote:
> I would prefer that there is only one copy of this code. I think the best plan
> is to rename this something more serious sounding (basically add Origin to the
> method name), then replace HTMLVideoElement::wouldTaintOrigin() calls with
calls
> to this method.

Done.

[sandersd (OOO until July 31), 2015-02-04 23:38:24]

lgtm

[ddorwin, 2015-02-04 23:56:50]

ddorwin@chromium.org changed reviewers:
+ philipj@opera.com

[ddorwin, 2015-02-04 23:56:51]

+philpj for origin and CORS advice (and eventual OWNERS approval).

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1569: return hasSingleSecurityOrigin() &&
((webMediaPlayer() && webMediaPlayer()->didPassCORSAccessCheck()) ||
!origin->taintsCanvas(currentSrc()));
taintsCanvas() has a special exception that is not spec-compliant and perhaps
not appropriate for initData:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

We might need to a) call canRequest() instead (???) and b) keep the logic that
was in HVE.

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1569: return hasSingleSecurityOrigin() &&
((webMediaPlayer() && webMediaPlayer()->didPassCORSAccessCheck()) ||
!origin->taintsCanvas(currentSrc()));
It's not clear what didPassCORSAccessCheck() means. It appears to just mean that
there was a value (from crossorigin attribute?) Perhaps we only get a loader
object if the CORS check then passed. We should double-check this logic, and
perhaps update related comments.

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:285: bool
canAccessSrcFromOrigin(SecurityOrigin*) const;
"CurrentSrc" to match the attribute (and impl)?

It probably reads better as canOriginAccessCurrentSrc(). WDYT?

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLVid...
File Source/core/html/HTMLVideoElement.cpp (right):

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:319: return
!canAccessSrcFromOrigin(destinationSecurityOrigin);
I'm not sure the two uses are equivalent. See earlier comment.

[ddorwin, 2015-02-05 00:10:31]

s/philpj/philipj/

Also, we should have layout tests. I think we can check it just by changing the
port like
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Going further, we could actually test with the crossorigin attribute. But most
importantly, we need to make sure data is not provided when cross-origin.

[sandersd (OOO until July 31), 2015-02-05 00:32:29]

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1569: return hasSingleSecurityOrigin() &&
((webMediaPlayer() && webMediaPlayer()->didPassCORSAccessCheck()) ||
!origin->taintsCanvas(currentSrc()));
On 2015/02/04 23:56:51, ddorwin wrote:
> taintsCanvas() has a special exception that is not spec-compliant and perhaps
> not appropriate for initData:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> We might need to a) call canRequest() instead (???) and b) keep the logic that
> was in HVE.

My reading of the code is that it is canRequest() is not spec-compliant and that
the workaround is to fix that. Certainly if you have a data URL you can parse
the initData yourself.

https://codereview.chromium.org/893123004/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1569: return hasSingleSecurityOrigin() &&
((webMediaPlayer() && webMediaPlayer()->didPassCORSAccessCheck()) ||
!origin->taintsCanvas(currentSrc()));
On 2015/02/04 23:56:51, ddorwin wrote:
> It's not clear what didPassCORSAccessCheck() means. It appears to just mean
that
> there was a value (from crossorigin attribute?) Perhaps we only get a loader
> object if the CORS check then passed. We should double-check this logic, and
> perhaps update related comments.

The name is bad, the meaning is that there was a crossorigin attribute and
*also* that it loaded successfully (which should be obvious if we get an
encrypted event, I suppose).

[philipj_slow, 2015-02-05 02:40:54]

A quick primer in how EME works could help me. I thought EME was basically
always used together with MSE, but this looks like it's dealing with a plain
HTTP (or something?) src that is encrypted?

The wording of the spec, using "media data" is a bit unlike the wording for
canvas so I'm not sure what the intended behavior is. Compare:
https://html.spec.whatwg.org/#the-image-argument-is-not-origin-clean

"an HTMLImageElement or HTMLVideoElement whose origin is not the same as the
origin specified by the entry settings object"

It seems to me like this is what should apply in this case as well. This is
conceptually simple, but there's a known problem on Android:
https://code.google.com/p/chromium/issues/detail?id=334204
https://code.google.com/p/chromium/issues/detail?id=358198

You'll have to decide if you want the same hole or not. I don't want it :)

[jrummell, 2015-02-05 21:40:31]

New patchsets have been uploaded after l-g-t-m from sandersd@chromium.org

[ddorwin, 2015-02-05 22:38:31]

On 2015/02/05 02:40:54, philipj_UTC7 wrote:
> A quick primer in how EME works could help me. I thought EME was basically
> always used together with MSE, but this looks like it's dealing with a plain
> HTTP (or something?) src that is encrypted?

Although most content providers using EME also use MSE, EME is technically
independent of the source. Thus, the spec and implementation should work with
both traditional .src= and MSE. Some implementations (i.e. Chrome on Android for
now - crbug.com/455905; some CE devices) only support EME with MSE. This is
often due to use of different demuxers and/or pipelines depending on the source.
This should hopefully disappear over time.

This CL should apply the same protections for both .src= and MSE sources.
Presumably, the same-origin checks already in the code are working for both
.src= and MSE sources or we'd have a bug in canvas tainting with MSE. Is that
right?

The goal of this CL is to ensure that initData (metadata from the container) is
not returned to the application if the media data came from a different
non-CORS-same-origin origin. This should apply for both <video
src="other-origin"> and MSE where the buffers were obtained via XHRs to another
origin.

> The wording of the spec, using "media data" is a bit unlike the wording for
> canvas so I'm not sure what the intended behavior is. Compare:
> https://html.spec.whatwg.org/#the-image-argument-is-not-origin-clean
> 
> "an HTMLImageElement or HTMLVideoElement whose origin is not the same as the
> origin specified by the entry settings object"

That language is unclear to me: In "HTMLVideoElement whose origin", is the
HTMLVideoElement's origin the origin of the source or of the document that
contains the element? If the latter, this could probably be clearer and be
updated to accommodate MSE. In addition, origin-clean does not appear to allow
CORS-same-origin content. Is that intentional?

The existing EME spec text "If the media data is CORS-same-origin and not mixed
content, run the following steps:" is similar to the text "If the media data is
CORS-same-origin, run the steps..." at
https://html.spec.whatwg.org/#found-a-media-resource-specific-timed-track (with
the addition of a mixed content check). The purpose of the EME step is the same
as the note at that URL.

If there are problems with the EME spec text or you have suggestions on how to
improve it, please let me know or file a spec bug.

> It seems to me like this is what should apply in this case as well. This is
What specifically should apply?

> conceptually simple, but there's a known problem on Android:
> https://code.google.com/p/chromium/issues/detail?id=334204
> https://code.google.com/p/chromium/issues/detail?id=358198
> 
> You'll have to decide if you want the same hole or not. I don't want it :)

Those bugs are related to the Android MediaPlayer, which is used for .src= URLs
in Chromium (crbug.com/401795). Since EME is currently only supported with MSE
on Android (crbug.com/455905), this should not be an issue.

[philipj_slow, 2015-02-06 16:05:32]

On 2015/02/05 22:38:31, ddorwin wrote:
> On 2015/02/05 02:40:54, philipj_UTC7 wrote:
> > A quick primer in how EME works could help me. I thought EME was basically
> > always used together with MSE, but this looks like it's dealing with a plain
> > HTTP (or something?) src that is encrypted?
> 
> Although most content providers using EME also use MSE, EME is technically
> independent of the source. Thus, the spec and implementation should work with
> both traditional .src= and MSE. Some implementations (i.e. Chrome on Android
for
> now - crbug.com/455905; some CE devices) only support EME with MSE. This is
> often due to use of different demuxers and/or pipelines depending on the
source.
> This should hopefully disappear over time.
> 
> This CL should apply the same protections for both .src= and MSE sources.
> Presumably, the same-origin checks already in the code are working for both
> .src= and MSE sources or we'd have a bug in canvas tainting with MSE. Is that
> right?

So my assumption here was that a MediaSource could never taint a canvas because
in order to get the data into a SourceBuffer you already need byte-level access
to the resource, pushing the problem elsewhere. Fortunately, the code seems to
agree:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

> The goal of this CL is to ensure that initData (metadata from the container)
is
> not returned to the application if the media data came from a different
> non-CORS-same-origin origin. This should apply for both <video
> src="other-origin"> and MSE where the buffers were obtained via XHRs to
another
> origin.

I understand, let's put MSE aside and treat the case of <video
src="other-origin"> which I didn't know could be use with EME until now.

> > The wording of the spec, using "media data" is a bit unlike the wording for
> > canvas so I'm not sure what the intended behavior is. Compare:
> > https://html.spec.whatwg.org/#the-image-argument-is-not-origin-clean
> > 
> > "an HTMLImageElement or HTMLVideoElement whose origin is not the same as the
> > origin specified by the entry settings object"
> 
> That language is unclear to me: In "HTMLVideoElement whose origin", is the
> HTMLVideoElement's origin the origin of the source or of the document that
> contains the element? If the latter, this could probably be clearer and be
> updated to accommodate MSE. In addition, origin-clean does not appear to allow
> CORS-same-origin content. Is that intentional?

Following the "origin" definition of that spec text leads to this:
For audio and video elements
If the media data is CORS-cross-origin
The origin is a globally unique identifier assigned when the media data is
fetched.
If the media data is CORS-same-origin
The origin is an alias to the origin of the media element's node document.
Media elements do not have an effective script origin.

Does that make sense?

> The existing EME spec text "If the media data is CORS-same-origin and not
mixed
> content, run the following steps:" is similar to the text "If the media data
is
> CORS-same-origin, run the steps..." at
> https://html.spec.whatwg.org/#found-a-media-resource-specific-timed-track
(with
> the addition of a mixed content check). The purpose of the EME step is the
same
> as the note at that URL.
> 
> If there are problems with the EME spec text or you have suggestions on how to
> improve it, please let me know or file a spec bug.

Hmm, I hadn't noticed that the HTML spec phrased this in two different ways. It
seems intentional, and there's a bit that says "The resource obtained in this
fashion, if any, contains the media data. It can be CORS-same-origin or
CORS-cross-origin; this affects whether subtitles referenced in the media data
are exposed in the API and, for video elements, whether a canvas gets tainted
when the video is drawn on it."

So the EME spec does use the right words and would fall into the same bucket as
for canvas tainting and exposing in-band track. Good!

> > It seems to me like this is what should apply in this case as well. This is
> What specifically should apply?

I mean that the conditions should be the same, and per spec they are. You said
that "taintsCanvas() has a special exception that is not spec-compliant and
perhaps
not appropriate for initData." If I'm reading correctly the exception is for
painting data: URIs to canvas. Would you like for data: URIs to fail the origin
check and not provide initData? I don't think it matters much as such, but
there's some comfort in using the same code path for all origin checks related
done for media elements.

> > conceptually simple, but there's a known problem on Android:
> > https://code.google.com/p/chromium/issues/detail?id=334204
> > https://code.google.com/p/chromium/issues/detail?id=358198
> > 
> > You'll have to decide if you want the same hole or not. I don't want it :)
> 
> Those bugs are related to the Android MediaPlayer, which is used for .src=
URLs
> in Chromium (crbug.com/401795). Since EME is currently only supported with MSE
> on Android (crbug.com/455905), this should not be an issue.

Phew!

[philipj_slow, 2015-02-06 16:17:57]

https://codereview.chromium.org/893123004/diff/40001/Source/core/html/HTMLVid...
File Source/core/html/HTMLVideoElement.cpp (left):

https://codereview.chromium.org/893123004/diff/40001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:319: return !hasSingleSecurityOrigin() ||
(!(webMediaPlayer() && webMediaPlayer()->didPassCORSAccessCheck()) &&
destinationSecurityOrigin->taintsCanvas(currentSrc()));
OK, so my confusion is starting to thaw and I see that this all pretty
straight-forward, you are trying to use the exact same rule in both places, so
we have no problem! Too bad that CanvasImageSource is on HTMLVideo element so
that these can't be consolidated. 

This all LGTM, but would the name mediaDataIsCorsSameOrigin(origin) be accurate?
I like naming things like in the spec where possible :)

[philipj_slow, 2015-02-06 16:18:56]

I guess s/Cors/CORS/ to match naming conventions.

[jrummell, 2015-02-06 19:07:40]

New patchsets have been uploaded after l-g-t-m from philipj@opera.com

[jrummell, 2015-02-06 19:08:35]

Updated.

https://codereview.chromium.org/893123004/diff/40001/Source/core/html/HTMLVid...
File Source/core/html/HTMLVideoElement.cpp (left):

https://codereview.chromium.org/893123004/diff/40001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:319: return !hasSingleSecurityOrigin() ||
(!(webMediaPlayer() && webMediaPlayer()->didPassCORSAccessCheck()) &&
destinationSecurityOrigin->taintsCanvas(currentSrc()));
On 2015/02/06 16:17:57, philipj_UTC7 wrote:
> OK, so my confusion is starting to thaw and I see that this all pretty
> straight-forward, you are trying to use the exact same rule in both places, so
> we have no problem! Too bad that CanvasImageSource is on HTMLVideo element so
> that these can't be consolidated. 
> 
> This all LGTM, but would the name mediaDataIsCorsSameOrigin(origin) be
accurate?
> I like naming things like in the spec where possible :)

Done.

[ddorwin, 2015-02-06 20:08:10]

On 2015/02/06 16:05:32, philipj_UTC7 wrote:
> On 2015/02/05 22:38:31, ddorwin wrote:
> > On 2015/02/05 02:40:54, philipj_UTC7 wrote:
> > > A quick primer in how EME works could help me. I thought EME was basically
> > > always used together with MSE, but this looks like it's dealing with a
plain
> > > HTTP (or something?) src that is encrypted?
> > 
> > Although most content providers using EME also use MSE, EME is technically
> > independent of the source. Thus, the spec and implementation should work
with
> > both traditional .src= and MSE. Some implementations (i.e. Chrome on Android
> for
> > now - crbug.com/455905; some CE devices) only support EME with MSE. This is
> > often due to use of different demuxers and/or pipelines depending on the
> source.
> > This should hopefully disappear over time.
> > 
> > This CL should apply the same protections for both .src= and MSE sources.
> > Presumably, the same-origin checks already in the code are working for both
> > .src= and MSE sources or we'd have a bug in canvas tainting with MSE. Is
that
> > right?
> 
> So my assumption here was that a MediaSource could never taint a canvas
because
> in order to get the data into a SourceBuffer you already need byte-level
access
> to the resource, pushing the problem elsewhere. Fortunately, the code seems to
> agree:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

WebMediaPlayerMS is MediaStream (WebRTC), not Media Source.
WebMediaPlayerImpl/WebMediaPlayerAndroid is used for both .src= and MSE.

However, we do appear to return true for MSE (MSE has no data_source_):
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...

> > > It seems to me like this is what should apply in this case as well. This
is
> > What specifically should apply?
> 
> I mean that the conditions should be the same, and per spec they are. You said
> that "taintsCanvas() has a special exception that is not spec-compliant and
> perhaps
> not appropriate for initData." If I'm reading correctly the exception is for
> painting data: URIs to canvas. Would you like for data: URIs to fail the
origin
> check and not provide initData? I don't think it matters much as such, but
> there's some comfort in using the same code path for all origin checks related
> done for media elements.

I think I misread the comment originally. As I read it now, the spec (at least
in 2009) said that data URLs should not be unique and thus not taint.

I filed crbug.com/456242 to hopefully resolve whether this issue in the comment
and the function are still relevant.

[ddorwin, 2015-02-06 21:41:33]

LG overall

https://codereview.chromium.org/893123004/diff/40001/Source/core/html/HTMLVid...
File Source/core/html/HTMLVideoElement.cpp (left):

https://codereview.chromium.org/893123004/diff/40001/Source/core/html/HTMLVid...
Source/core/html/HTMLVideoElement.cpp:319: return !hasSingleSecurityOrigin() ||
(!(webMediaPlayer() && webMediaPlayer()->didPassCORSAccessCheck()) &&
destinationSecurityOrigin->taintsCanvas(currentSrc()));
On 2015/02/06 19:08:35, jrummell wrote:
> On 2015/02/06 16:17:57, philipj_UTC7 wrote:
> > OK, so my confusion is starting to thaw and I see that this all pretty
> > straight-forward, you are trying to use the exact same rule in both places,
so
> > we have no problem! Too bad that CanvasImageSource is on HTMLVideo element
so
> > that these can't be consolidated. 
> > 
> > This all LGTM, but would the name mediaDataIsCorsSameOrigin(origin) be
> accurate?
> > I like naming things like in the spec where possible :)
> 
> Done.

Does the new function really tell us that it is CORS-same-origin? See my other
comment in PS4 of HME.cpp.

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
File
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html
(right):

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html:1:
<!DOCTYPE html>
For full coverage of this complex issue, we should have tests:
 * where the crossorigin attribute is set
 * Where we switch back and forth between origins
 * for MSE

I'm okay with adding these later. Perhaps file a bug.

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html:29:
// The same decryption key is used by both the audio and
Is this really relevant? It's the fact that both tracks have initData that
causes the two.

ditto below

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html:45:
}, 'InitDataType returned when using same origin.');
It's not really the "Type" that is important here. I'd drop that. Probably
either "Initialization Data" or "initData".

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html:47:
async_test(function(test)
I wonder whether we should break this up into separate HTML files (-same-origin,
-different-origin).

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1567: bool
HTMLMediaElement::mediaDataIsCORSSameOrigin(SecurityOrigin* origin) const
Does the new function really tell us that it is CORS-same-origin? (As discussed
elsewhere) taintsCanvas() is basically canRequest(), which has more to do with
access than same-origin.

hasSingleSecurityOrigin() apparently tells us whether the origin in the src is
the same as the actual request (i.e. after redirect). If correct, it would be
nice to add a comment in WebMediaPlayer.h.

I think didPassCORSAccessCheck() means it was a successful CORS-enabled fetch
(vs. non-CORS-enabled or failed).

Then, taintsCanvas() must check that it is not a URL that requires CORS - that
is, same origin. However, it also does some other things, which I guess are
valid exceptions.

I suppose at a high level, it is equivalent to CORS-same-origin.

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:284: // Checks to see if |origin| is allowed
access to the current src.
The comment no longer matches the function name.

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:285: bool
mediaDataIsCORSSameOrigin(SecurityOrigin*) const;
isMediaDataCORSSameOrigin

[jrummell, 2015-02-07 00:12:26]

Updated.

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
File
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html
(right):

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html:1:
<!DOCTYPE html>
On 2015/02/06 21:41:33, ddorwin wrote:
> For full coverage of this complex issue, we should have tests:
>  * where the crossorigin attribute is set
>  * Where we switch back and forth between origins
>  * for MSE
> 
> I'm okay with adding these later. Perhaps file a bug.

Opened http://crbug.com/456358

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html:29:
// The same decryption key is used by both the audio and
On 2015/02/06 21:41:33, ddorwin wrote:
> Is this really relevant? It's the fact that both tracks have initData that
> causes the two.
> 
> ditto below

Done.

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html:45:
}, 'InitDataType returned when using same origin.');
On 2015/02/06 21:41:33, ddorwin wrote:
> It's not really the "Type" that is important here. I'd drop that. Probably
> either "Initialization Data" or "initData".

Done.

https://codereview.chromium.org/893123004/diff/60001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event.html:47:
async_test(function(test)
On 2015/02/06 21:41:33, ddorwin wrote:
> I wonder whether we should break this up into separate HTML files
(-same-origin,
> -different-origin).

Done.

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:1567: bool
HTMLMediaElement::mediaDataIsCORSSameOrigin(SecurityOrigin* origin) const
On 2015/02/06 21:41:33, ddorwin wrote:
> Does the new function really tell us that it is CORS-same-origin? (As
discussed
> elsewhere) taintsCanvas() is basically canRequest(), which has more to do with
> access than same-origin.
> 
> hasSingleSecurityOrigin() apparently tells us whether the origin in the src is
> the same as the actual request (i.e. after redirect). If correct, it would be
> nice to add a comment in WebMediaPlayer.h.
> 
> I think didPassCORSAccessCheck() means it was a successful CORS-enabled fetch
> (vs. non-CORS-enabled or failed).
> 
> Then, taintsCanvas() must check that it is not a URL that requires CORS - that
> is, same origin. However, it also does some other things, which I guess are
> valid exceptions.
> 
> I suppose at a high level, it is equivalent to CORS-same-origin.

Acknowledged. Added comments to indicate this, just so the next person looking
at it has somewhere to start.

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:284: // Checks to see if |origin| is allowed
access to the current src.
On 2015/02/06 21:41:33, ddorwin wrote:
> The comment no longer matches the function name.

Done.

https://codereview.chromium.org/893123004/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:285: bool
mediaDataIsCORSSameOrigin(SecurityOrigin*) const;
On 2015/02/06 21:41:33, ddorwin wrote:
> isMediaDataCORSSameOrigin

Done.

https://codereview.chromium.org/893123004/diff/80001/LayoutTests/http/tests/m...
File
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event-different-origin.html
(right):

https://codereview.chromium.org/893123004/diff/80001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event-different-origin.html:10:
<video id="testVideo2"></video>
Don't need both video elements -- I'll fix it.

[ddorwin, 2015-02-07 01:59:01]

LGTM % nit

https://codereview.chromium.org/893123004/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/893123004/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:284: // Checks to see if current media data
is CORS-same-origin as |origin|.
|origin| doesn't exist. perhaps "the specified origin."

[jrummell, 2015-02-07 02:11:02]

New patchsets have been uploaded after l-g-t-m from ddorwin@chromium.org

[jrummell, 2015-02-07 02:11:47]

Thanks for the reviews.

https://codereview.chromium.org/893123004/diff/80001/LayoutTests/http/tests/m...
File
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event-different-origin.html
(right):

https://codereview.chromium.org/893123004/diff/80001/LayoutTests/http/tests/m...
LayoutTests/http/tests/media/encrypted-media/encrypted-media-encrypted-event-different-origin.html:10:
<video id="testVideo2"></video>
On 2015/02/07 00:12:26, jrummell wrote:
> Don't need both video elements -- I'll fix it.

Done.

https://codereview.chromium.org/893123004/diff/80001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/893123004/diff/80001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:284: // Checks to see if current media data
is CORS-same-origin as |origin|.
On 2015/02/07 01:59:01, ddorwin wrote:
> |origin| doesn't exist. perhaps "the specified origin."

Done.

[jrummell, 2015-02-07 02:11:53]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2015-02-07 02:12:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/893123004/100001

[commit-bot: I haz the power, 2015-02-07 03:35:51]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189735