Last Comment Bug 682299 - Implement crossOrigin attribute for <video> element
: Implement crossOrigin attribute for <video> element
Status: RESOLVED FIXED
[popcorn.js][sec-assigned:dveditz]
: dev-doc-complete
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla12
Assigned To: Jon Buckley
:
Mentors:
: 655823 (view as bug list)
Depends on: 837153 875194
Blocks: 676913 703566 760338
  Show dependency treegraph
 
Reported: 2011-08-26 08:26 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2013-05-22 21:17 PDT (History)
19 users (show)
dveditz: sec‑review? (dveditz)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add CORS support to <video> tag (7.00 KB, patch)
2011-12-16 22:41 PST, Jon Buckley
bzbarsky: feedback+
Details | Diff | Review
1 Move all CORS-related attributes and enumerations to nsGenericHTMLElement (24.26 KB, patch)
2012-01-08 22:15 PST, Jon Buckley
bzbarsky: review+
Details | Diff | Review
2 Implement CORS support for the <video> tag (15.64 KB, patch)
2012-01-08 22:17 PST, Jon Buckley
no flags Details | Diff | Review
1 Move all CORS-related attributes and enumerations to nsGenericHTMLElement (24.26 KB, patch)
2012-01-08 22:19 PST, Jon Buckley
no flags Details | Diff | Review
3 Report a console error message when a crossorigin video fails to load (3.70 KB, patch)
2012-01-08 22:25 PST, Jon Buckley
roc: review-
Details | Diff | Review
4 Tests (8.21 KB, patch)
2012-01-08 22:26 PST, Jon Buckley
no flags Details | Diff | Review
4 Tests (10.79 KB, patch)
2012-01-08 22:32 PST, Jon Buckley
no flags Details | Diff | Review
1v2 Move CORS-related attributes and enumerations to nsGenericHTMLElement (11.49 KB, patch)
2012-01-13 12:10 PST, Jon Buckley
jon: review+
Details | Diff | Review
3v2 Report a console error message when a crossorigin video fails to load (2.56 KB, patch)
2012-01-13 13:36 PST, Jon Buckley
no flags Details | Diff | Review
2v2 Implement CORS support for the <video> tag (19.37 KB, patch)
2012-01-23 14:36 PST, Jon Buckley
roc: review+
Details | Diff | Review
3v3 Report a console error message when a crossorigin video fails to load (3.89 KB, patch)
2012-01-23 14:42 PST, Jon Buckley
jonas: review-
Details | Diff | Review
4v2 CORS <video> tests WIP (8.29 KB, patch)
2012-01-23 14:44 PST, Jon Buckley
roc: review+
Details | Diff | Review
4v3 CORS <video> tests (9.02 KB, patch)
2012-01-23 19:17 PST, Jon Buckley
jon: review+
Details | Diff | Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-08-26 08:26:33 PDT
In order to allow for cross-domain <video> elements to be usable as WebGL textures, what's needed is support for the crossOrigin attribute on <video> elements.

See bug 664299 for the equivalent for <img> elements.

This will offer a path for some of Mozilla's own video/WebGL demos to resume working, see bug 676913.
Comment 1 Lari Temmes 2011-10-26 23:53:05 PDT
I vote for this. CORS attribute for media should be implemented. Spec for attribute is descripted here http://www.whatwg.org/specs/web-apps/current-work/#attr-media-crossorigin .
Comment 2 Boris Zbarsky [:bz] 2011-11-06 18:24:42 PST
*** Bug 655823 has been marked as a duplicate of this bug. ***
Comment 3 Jon Buckley 2011-12-05 17:39:30 PST
I'd like to take this bug as it's blocking some cool stuff that we want to do in Popcorn Maker
Comment 4 Jon Buckley 2011-12-16 22:41:52 PST
Created attachment 582492 [details] [diff] [review]
Add CORS support to <video> tag

Here's a patch that adds CORS support to the <video> tag. I am currently hosting some basic tests on my site at http://jbuckley.ca:8000/ . They drawImage the video into a canvas, then attempt to getImageData from the same canvas. The cors-enabled tests have CORS enabled on the server side, the cors-disabled tests do not.

One improvement that I need to make is adding an error message for when CORS is requested on a server that does not support CORS; If you run test-cors-disabled-anon.html or test-cors-disabled-credentials.html with this patch applied, you get a "Video format for MIME type is not supported" message.
Comment 5 Chris Pearce (:cpearce) 2011-12-18 14:22:29 PST
Comment on attachment 582492 [details] [diff] [review]
Add CORS support to <video> tag

Review of attachment 582492 [details] [diff] [review]:
-----------------------------------------------------------------

bz (or maybe roc) really needs to be the one giving you feedback/review on this.

Does the mochitest content/media/test/test_access_control.html still pass with this patch? That test at least needs to be changed to not set the "enforce_same_site_origin" pref, since that's not used anymore.

It would be great if you could use nsContentUtils::ReportToConsole() to add logging upon load failure due to CORS to help web developers understand why their resources aren't loading.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1518,5 @@
>      if (ParseImageAttribute(aAttribute, aValue, aResult)) {
>        return true;
>      }
> +    if (aAttribute == nsGkAtoms::crossorigin) {
> +      return aResult.ParseEnumValue(aValue, kCrossOriginTable, false);

Does this handle the "invalid value default" of "anonymous"? Or maybe that needs to be in GetCORSMode()?

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +56,5 @@
>  #undef GetCurrentTime
>  #endif
>  %}
>  
> +[scriptable, uuid(6733a409-fab3-45e1-af23-9af8c361bdfd)]

You need to rev the uuids on all classes that extend this one too, so nsIDOMHTMLVideoElement and nsIDOMHTMLAudioElement.
Comment 6 Boris Zbarsky [:bz] 2011-12-18 22:52:47 PST
Comment on attachment 582492 [details] [diff] [review]
Add CORS support to <video> tag

We should probably move that attribute enum table up to nsGenericHTMLElement and rename it to kCORSAttributeTable or something.  And perhaps move the CORS_* values up to nsGenericHTMLElement as well...

Furthermore, we should reconcile the table used here and the one used in nsHTMLImageElement.  I believe the one used there is correct and the ones used here are wrong.

The vestiges of the "media.enforce_same_site_origin" should go away.

The code in SurfaceFromElement is wrong.  It's looking at the CORS mode of the element at draw time, but what's important is the CORS mode at load time.  In other words, like the image case, the CORS mode looked at in SurfaceFromElement needs to be associated with the video data, not the element.  Consider the scenario where a cross-origin video is loaded without a "crossorigin" attribute, then the page sets the attribute and calls drawImage immediately.  The patch attached to this bug wouldn't taint the canvas in that case.  There should probably be a test for this case in the patch, by the way, as well as tests for the case when we really shouldn't taint the canvas.

LookupMediaElementURITable needs to be changed to look at the CORS mode; there are actually comments in there pointing that out.

I have no idea whether the media cache needs to be taught about CORS mode somehow... roc is probably the right man for that.

Apart from the issues above, this seems to be on the right track.  I have no idea whether this makes for feedback+ or feedback-.  ;)
Comment 7 Jon Buckley 2012-01-08 22:15:18 PST
Created attachment 586881 [details] [diff] [review]
1 Move all CORS-related attributes and enumerations to nsGenericHTMLElement
Comment 8 Jon Buckley 2012-01-08 22:17:20 PST
Created attachment 586883 [details] [diff] [review]
2 Implement CORS support for the <video> tag
Comment 9 Jon Buckley 2012-01-08 22:19:11 PST
Created attachment 586885 [details] [diff] [review]
1 Move all CORS-related attributes and enumerations to nsGenericHTMLElement

Oops, this patch wasn't meant to be made obsolete..., this is the first one that needs to be applied
Comment 10 Jon Buckley 2012-01-08 22:25:39 PST
Created attachment 586886 [details] [diff] [review]
3 Report a console error message when a crossorigin video fails to load

roc reviewed bug 712134, which is the reason for the change of reviewer here
Comment 11 Jon Buckley 2012-01-08 22:26:44 PST
Created attachment 586887 [details] [diff] [review]
4 Tests
Comment 12 Jon Buckley 2012-01-08 22:32:56 PST
Created attachment 586888 [details] [diff] [review]
4 Tests

Now includes the video files
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-08 22:38:06 PST
Comment on attachment 586886 [details] [diff] [review]
3 Report a console error message when a crossorigin video fails to load

Review of attachment 586886 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +328,5 @@
> +    nsAutoString src;
> +    element->GetCurrentSrc(src);
> +    const PRUnichar *params[] = { src.get() };
> +    element->ReportLoadError("MediaLoadCORSError", params, ArrayLength(params));
> +    return NS_BINDING_ABORTED;

How do you know for sure this was a CORS-related error? Seems like you need some additional checking here.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-08 22:39:04 PST
Comment on attachment 586886 [details] [diff] [review]
3 Report a console error message when a crossorigin video fails to load

Review of attachment 586886 [details] [diff] [review]:
-----------------------------------------------------------------

oops, I meant to r- that
Comment 15 Boris Zbarsky [:bz] 2012-01-09 12:38:22 PST
Comment on attachment 586881 [details] [diff] [review]
1 Move all CORS-related attributes and enumerations to nsGenericHTMLElement

s/so the/so they/

The "order matters here" comment should remain.

The imagelib changes here don't look necessary.  Please undo those.

r=me with those fixed.
Comment 16 Boris Zbarsky [:bz] 2012-01-09 12:43:17 PST
Comment on attachment 586883 [details] [diff] [review]
2 Implement CORS support for the <video> tag

Once LoadResource is entered, does that guarantee that whatever was loaded before is no longer available?

Is LookupMediaElementURITable guaranteed to be called after LoadResource on both elements involved?

It'd be good to have someone more familiar than I with the media code review this...
Comment 17 Boris Zbarsky [:bz] 2012-01-09 12:43:51 PST
Comment on attachment 586888 [details] [diff] [review]
4 Tests

Likewise, this would be better reviewed by someone who knows the media code better and knows what edge cases need testing.
Comment 18 Jon Buckley 2012-01-13 12:10:20 PST
Created attachment 588489 [details] [diff] [review]
1v2 Move CORS-related attributes and enumerations to nsGenericHTMLElement
Comment 19 Jon Buckley 2012-01-13 13:36:29 PST
Created attachment 588513 [details] [diff] [review]
3v2 Report a console error message when a crossorigin video fails to load

Added an explicit check for NS_ERROR_DOM_BAD_URI, which is the error returned by the nsCORSListenerProxy when the CORS check fails.

roc, would you be able to review my #2 and #4 patches in this bug, and if not, who would you recommend assigning to?
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-13 13:45:48 PST
I can review 2 and 4.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 01:43:43 PST
Unfortunately I don't think NS_ERROR_DOM_BAD_URI is only caused by CORS. I think we actually need a new "CORS failed" error code.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 03:17:32 PST
(or some other way to check that CORS failed)
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 03:22:12 PST
Comment on attachment 586883 [details] [diff] [review]
2 Implement CORS support for the <video> tag

Review of attachment 586883 [details] [diff] [review]:
-----------------------------------------------------------------

Basically looks great. Just need answers to those questions.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +72,5 @@
>      CANPLAY_MAYBE,
>      CANPLAY_YES
>    };
>  
> +  nsGenericHTMLElement::CORSMode GetCORSMode();

nsGenericHTMLElement prefix should not be needed here, or anywhere else you've used it.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +944,5 @@
> +  // By default, it's nsGenericHTMLElement::CORS_NONE
> +  const nsAttrValue* value = GetParsedAttr(nsGkAtoms::crossorigin);
> +  if (value) {
> +    NS_ASSERTION(value->Type() == nsAttrValue::eEnum,
> +                 "Why is this not an enum value?");

Is this safe? What happens if you set crossOrigin to some random string? We shouldn't assert due to an author error like that, it's not a browser bug.

@@ +945,5 @@
> +  const nsAttrValue* value = GetParsedAttr(nsGkAtoms::crossorigin);
> +  if (value) {
> +    NS_ASSERTION(value->Type() == nsAttrValue::eEnum,
> +                 "Why is this not an enum value?");
> +    mCORSMode = nsGenericHTMLElement::CORSMode(value->GetEnumValue());

Shouldn't we set mCORSMode to something when the attribute is not present?

@@ +1534,5 @@
>  
> +NS_IMPL_STRING_ATTR(nsHTMLMediaElement, Crossorigin, crossorigin)
> +
> +nsGenericHTMLElement::CORSMode nsHTMLMediaElement::GetCORSMode() {
> +  return mCORSMode;

just inline this in the header file.
Comment 24 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 03:47:42 PST
Comment on attachment 586888 [details] [diff] [review]
4 Tests

Review of attachment 586888 [details] [diff] [review]:
-----------------------------------------------------------------

We try to write our tests to be backend independent, so they work for people who turn off particular backends. So we don't like to hardcode WebM dependencies for example. Instead, load manifest.js and search for (e.g.) the first element of gSmallTests that is supported via canPlayType.

You still need to send the right headers, so what I'd do is instead of adding new resource files, write a .sjs module based on one of the existing ones (referer.sjs is pretty simple) that takes a resource name, MIME type, and CORS response headers as HTTP query parameters and returns the resource data with the specified CORS headers. Then you can easily load any resource and have the load get the CORS response headers you want.
Comment 25 Boris Zbarsky [:bz] 2012-01-14 08:42:20 PST
> Is this safe? What happens if you set crossOrigin to some random string?

Then it'll end up with the value passed as aDefaultValue to ParseEnumValue, if one is passed.  If one is not passed, it would end up as a non-enum-type attrvalue.  In this case we pass a default value, so this code is fine.

> Shouldn't we set mCORSMode to something when the attribute is not present?

Constructor sets it to CORS_NONE, I think.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 15:28:35 PST
(In reply to Boris Zbarsky (:bz) from comment #25)
> Constructor sets it to CORS_NONE, I think.

I think that means there's a bug if you have <video crossorigin="...">, do a load, then remove the crossorigin attribute and do another load --- the previous CORS mode will be used.
Comment 27 Boris Zbarsky [:bz] 2012-01-14 18:02:50 PST
Er, yes.  LoadResource should set mCORSMode to CORS_NONE if !value.  Good catch; we should at this to the tests.

I'm still not sure why we can't just store the CORS mode on the actual load as opposed to on the element, though.  Or do we not have an object representing the load around?
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 18:36:41 PST
I guess storing it in MediaLoadListener might be a little cleaner.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 18:37:27 PST
But the media element doesn't hold a reference to MediaLoadListener directly so that'd be a bit annoying.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-14 18:37:49 PST
So I say just fix the bug but leave mCORSMode there.
Comment 31 Jon Buckley 2012-01-15 09:19:12 PST
I think the reason that the CORS proxy returns NS_ERROR_DOM_BAD_URI is that the spec requires it. From http://dvcs.w3.org/hg/cors/raw-file/tip/Overview.html#simple-cross-origin-request-0 :

> Perform a resource sharing check. If it returns fail, apply the network error 
> steps. Otherwise, if it returns pass, terminate this algorithm and set the
> cross-origin request status to success. Do not actually terminate the request.

In Section 8.3 (http://dvcs.w3.org/hg/cors/raw-file/tip/Overview.html#cors-api-specification-response) the spec says that the network error condition should, "Handle analogous to requests where some kind of error occured. Ensure not the reveal any further information about the request."

This sounds like we shouldn't be adding an additional error code specifically for CORS errors; we might accidentally leak information about the request.

We also do a similar type of CORS error check in http://mxr.mozilla.org/mozilla-central/source/content/media/nsMediaStream.cpp#167
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-16 16:35:12 PST
(In reply to Jon Buckley [:jbuck] from comment #31)
> This sounds like we shouldn't be adding an additional error code
> specifically for CORS errors; we might accidentally leak information about
> the request.

OK, but I don't think Web apps can get access to the specific nsresult here.

Currently with your patch I think we could fail to load for some reason other than a CORS error, but report to Web developers that it was because of a CORS error. That would be bad.
Comment 33 Jon Buckley 2012-01-17 18:41:06 PST
Alright, I'll implement a different error code for CORS errors.

One thing I've noticed with this patch is that the entire video file needs to load before the actual CORS check can happen, because nsCrossSiteListenerProxy::OnStartRequest() only fires when the entire file has been loaded. If I'm looking at the network request in Firebug, I can see that the first request has the CORS headers. Is there a way to check the CORS headers on the first request, rather than waiting? It's really only noticeable with video files because they're usually much larger than images or fonts.
Comment 34 Jonas Sicking (:sicking) 2012-01-17 21:31:03 PST
Why do we need a different error code for CORS errors? The idea is that CORS errors should be indistinguishable from network errors so that you can't use CORS requests to scan for servers behind firewalls.
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-18 18:00:11 PST
(In reply to Jonas Sicking (:sicking) On leave waiting for visa from comment #34)
> Why do we need a different error code for CORS errors?

So that we can give Web developers a different message for CORS-related errors.

> The idea is that CORS
> errors should be indistinguishable from network errors so that you can't use
> CORS requests to scan for servers behind firewalls.

Indeed, the different error code must not be exposed to Web apps.

(In reply to Jon Buckley [:jbuck] from comment #33)
> One thing I've noticed with this patch is that the entire video file needs
> to load before the actual CORS check can happen, because
> nsCrossSiteListenerProxy::OnStartRequest() only fires when the entire file
> has been loaded.

... It does? That doesn't sound right.
Comment 36 Jon Buckley 2012-01-19 14:06:44 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> > One thing I've noticed with this patch is that the entire video file needs
> > to load before the actual CORS check can happen, because
> > nsCrossSiteListenerProxy::OnStartRequest() only fires when the entire file
> > has been loaded.
> 
> ... It does? That doesn't sound right.

My apologies, it only does that if the server doesn't support byte range requests, like the python SimpleHTTPServer. If you use a web server that supports byte range requests, it works as expected and fails instantly.
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-19 14:38:03 PST
That still seems wrong. The channel is supposed to fire nsCrossSiteListenerProxy::OnStartRequest as we start receiving content (i.e. all headers have been received).
Comment 38 Jon Buckley 2012-01-23 14:31:40 PST
With the python SimpleHTTPServer, video file loading acts strangely. They need to wait for the first bit of the file to load (782 KB according to Firebug) and on the console it shows that the browser has cancelled the connection (error: [Errno 32] Broken pipe). This occurs whether or not crossorigin is set. Now that might actually be a bug, I'm not sure. But my initial thought "CrossSiteListenerProxy is slowing things down" was wrong; it's just the python SimpleHTTPServer acting strangely.
Comment 39 Jon Buckley 2012-01-23 14:36:52 PST
Created attachment 590878 [details] [diff] [review]
2v2 Implement CORS support for the <video> tag

Fixs the mCORSMode bug
Comment 40 Jon Buckley 2012-01-23 14:42:48 PST
Created attachment 590882 [details] [diff] [review]
3v3 Report a console error message when a crossorigin video fails to load

Fixes an inconsistent behaviour in nsCrossSiteListenerProxy, where the wrong error would be reported if crossorigin='use-credentials' and the HTTP request failed (ie 404). A request that set crossorigin='anonymous' and 404'd would correctly print a 404 message to the web console.

I'm not sure if this is the correct behaviour; However, it does mean that subscribers of the nsCrossSiteListenerProxy can determine from the combination of status and http response status whether an error was a CORS error, or an HTTP error without implementing a new CORS error code.
Comment 41 Jon Buckley 2012-01-23 14:44:20 PST
Created attachment 590883 [details] [diff] [review]
4v2 CORS <video> tests WIP

Rewrote the tests to use sjs and existing video files. Still need to add tests for fiddling with CORS modes.
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-23 15:13:57 PST
(In reply to Jon Buckley [:jbuck] from comment #38)
> With the python SimpleHTTPServer, video file loading acts strangely. They
> need to wait for the first bit of the file to load (782 KB according to
> Firebug) and on the console it shows that the browser has cancelled the
> connection (error: [Errno 32] Broken pipe). This occurs whether or not
> crossorigin is set. Now that might actually be a bug, I'm not sure. But my
> initial thought "CrossSiteListenerProxy is slowing things down" was wrong;
> it's just the python SimpleHTTPServer acting strangely.

Are you testing without preload or autoplay? If you just write <video src="...">, we'll load enough to get the first frame and then stop loading.
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-23 15:25:22 PST
Comment on attachment 590882 [details] [diff] [review]
3v3 Report a console error message when a crossorigin video fails to load

Review of attachment 590882 [details] [diff] [review]:
-----------------------------------------------------------------

Requesting additional review from Jonas; I'm not sure whether the change to nsCrossSiteListenerProxy is secure. Maybe a caller that's not careful might now leak information about whether a cross-origin request succeeded or not?

Maybe it would actually be better to return the same error code here but provide a separate method on nsCrossSiteListenerProxy to check whether the request failed due to CORS issues or HTTP issues?
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-23 15:27:41 PST
You can land patches 1, 2 and 4 now I guess, if that helps.
Comment 45 Jon Buckley 2012-01-23 19:07:31 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> Are you testing without preload or autoplay? If you just write <video
> src="...">, we'll load enough to get the first frame and then stop loading.

Yeah, that works as expected. Sorry for the confusion I caused!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> Requesting additional review from Jonas; I'm not sure whether the change to
> nsCrossSiteListenerProxy is secure. Maybe a caller that's not careful might
> now leak information about whether a cross-origin request succeeded or not?
> 
> Maybe it would actually be better to return the same error code here but
> provide a separate method on nsCrossSiteListenerProxy to check whether the
> request failed due to CORS issues or HTTP issues?

Definitely possible that we might leak information, right now it's just inconsistent. HTTP response errors should cause the CORS proxy to always fail or always succeed.

And with some testing on the try servers, patch #3 makes several cross origin XHR tests fail. I'm leaning towards making it fail on HTTP response errors now...

--

Landing the meat of the patches now will definitely help. I'm going to run them through the try server first, just to make sure everything works properly without patch #3.
Comment 46 Jon Buckley 2012-01-23 19:17:42 PST
Created attachment 590978 [details] [diff] [review]
4v3 CORS <video> tests

Additional canvas tainting checks
Comment 49 Ian Melven :imelven 2012-02-06 11:10:32 PST
This has already landed but marking sec-review-needed after discussing with Curtis since it seems like a good idea for someone from the security assurance team to review what was implemented here :)
Comment 50 Jonas Sicking (:sicking) 2012-02-07 16:55:47 PST
Comment on attachment 590882 [details] [diff] [review]
3v3 Report a console error message when a crossorigin video fails to load

Review of attachment 590882 [details] [diff] [review]:
-----------------------------------------------------------------

The change to the CORS code seems wrong. Please renominate if I'm interpreting things wrongly.

::: content/base/src/nsCrossSiteListenerProxy.cpp
@@ +552,5 @@
>    // Test that things worked on a HTTP level
>    nsCOMPtr<nsIHttpChannel> http = do_QueryInterface(aRequest);
>    NS_ENSURE_TRUE(http, NS_ERROR_DOM_BAD_URI);
> +  bool succeeded;
> +  if (http && NS_SUCCEEDED(http->GetRequestSucceeded(&succeeded)) && !succeeded) {

I don't understand this change. It'll make us completely skip any security checks for non-200 responses. I.e. any 404 page will be automatically granted access to, no matter what the CORS headers say.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +330,5 @@
> +      element->GetCurrentSrc(src);
> +      const PRUnichar *params[] = { src.get() };
> +      element->ReportLoadError("MediaLoadCORSError", params, ArrayLength(params));
> +    }
> +    return NS_BINDING_ABORTED;

Is it intentional that you now no longer propagate the error code which came from the request, even for non-CORS related errors?
Comment 51 Paul Rouget [:paul] 2012-05-21 05:23:34 PDT
Any update on this?
Comment 52 Jon Buckley 2012-05-31 17:58:03 PDT
Hm. Well, I thoroughly dropped the ball on completing the last bit of this ticket.

The core feature itself, allowing people to use the crossorigin attribute on <video> elements, does work in Firefox 12. I think the best thing to do now would be mark this ticket as resolved fixed, and open a new dependent bug on "Displaying an error message in the console when a CORS failure occurs".

Does that sound right?
Comment 53 Boris Zbarsky [:bz] 2012-05-31 18:06:15 PDT
That sounds like an excellent idea.

Note You need to log in before you can comment on or make changes to this bug.