664299 - Add crossorigin attribute

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

A new attribute was recently added to the HTML spec: crossorigin:

http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.html#the-img-element

http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#cors-settings-attribute

Supporting it is needed for bug 662599.

Can you please explain me in detail (assume you're talking to a chimp) the steps that I need to follow to implement that? Or let me know if you plan to do that yourself, but that is a high priority for WebGL.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Note: WebKit patch here, including testcases:
https://bugs.webkit.org/show_bug.cgi?id=61015
https://bugs.webkit.org/attachment.cgi?id=95078

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You probably don't have to do anything to the parser.

You need to add "crossorigin" to nsIDOMHTMLImageElement.idl.

You need to add NS_IMPL_STRING_ATTR(nsHTMLImageElement, CrossOrigin, crossorigin) to nsHTMLImageElement.cpp.

You'll want to add a method to nsIImageLoadingContent to get the "CORS mode" for the load. The default would be "No CORS", nsHTMLImageElement would override it to return something based on the current attribute value.

When CORS mode is not "No CORS", nsImageLoadingContent::LoadImage needs to set up CORS when loading the image. See nsMediaChannelStream::OpenChannel for an example of this. You probably want to pass the CORS mode and the requesting principal to nsContentUtils::LoadImage and then into imgILoader::LoadImage.

The hardest part that I can give you the least advice about is integrating with the image cache. For image requests made with CORS, the incoming principal needs to become part of the cache key so that results of CORS loads don't get mixed up with results of non-CORS loads, and results of CORS loads with different principals don't get mixed up with each other.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #2)
> You need to add "crossorigin" to nsIDOMHTMLImageElement.idl.

crossOrigin

> You need to add NS_IMPL_STRING_ATTR(nsHTMLImageElement, CrossOrigin,
> crossorigin) to nsHTMLImageElement.cpp.

NS_IMPL_ENUM_ATTR_DEFAULT_VALUE

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Thanks a lot for the explanation, I'm slowly working my way through that.

One thing I don't understand is that at some point we're supposed to parse some HTTP header to see if we're allowed to use the image where cross-domain images would normally not be allowed. Where is that part supposed to happen? I don't see where in comment 2's outline it fits.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

nsCORSListenerProxy does all that for you. If it detects an improper attempt to use a cross-site resource it will cancel your channel with NS_ERROR_DOM_BAD_URI, and you would detect that in your OnStartRequest callback. See nsMediaChannelStream::OnStartRequest for an example.

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to comment #2)
> You probably don't have to do anything to the parser.

Confirmed. No need to touch the parser. (A tiny, tiny atom interning optimization could be done, but I can do it sometime later. No need to worry about it here.)

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

OK, I only got back to this today as I was taken by security bugs. Here's where I'm currently stuck:

(In reply to comment #2)
> When CORS mode is not "No CORS", nsImageLoadingContent::LoadImage needs to
> set up CORS when loading the image. See nsMediaChannelStream::OpenChannel
> for an example of this. You probably want to pass the CORS mode and the
> requesting principal to nsContentUtils::LoadImage and then into
> imgILoader::LoadImage.

The code in nsMediaChannelStream::OpenChannel is doing:
http://mxr.mozilla.org/mozilla-central/source/content/media/nsMediaStream.cpp#449

460       nsCORSListenerProxy* crossSiteListener =
461         new nsCORSListenerProxy(mListener,
462                                 element->NodePrincipal(),
463                                 mChannel,
464                                 PR_FALSE,
465                                 &rv);
466       listener = crossSiteListener;

so it's all about 'listeners'. But in nsImageLoadingContent::LoadImage, there's no 'listener'. However, there's one in LoadImageWithChannel.

Questions:
 1) What's a 'listener'?
 2) Do I only need to edit LoadImageWithChannel or do I really need to edit LoadImage?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The listener is the thing that receives callbacks from Necko when something happens to the channel.

You can probably get by with changing LoadImageWithChannel.

Comment 9

[Boris Zbarsky [:bzbarsky]]

LoadImageWithChannel is only used when an image is loaded directly in an iframe or toplevel window.  You can probably not worry about CORS stuff at all on that codepath for now.  Changing it won't affect <img src> loads.

The code where you have a channel to work with is inside imagelib, not in nsImageLoadingContent.  See the imgILoader::LoadImage call that nsImageLoadingContent makes....

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Guys, thanks for the help, but I'm still stuck. Good to know that I only need to edit LoadImage and not LoadImageWithChannel. But as far as I can see, the way to check if there is CORS approval is to use nsCORSListenerProxy, and this wants a Listener to work with. My problem is that there is no Listener around in nsImageLoadingContent::LoadImage, as far as I can see.

Comment 11

[Boris Zbarsky [:bzbarsky]]

See comment 9 paragraph 2.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 1: add the crossorigin attribute

Getting Joe to do Part 2 - actually do something useful in imagelib; and there will be a part 3 - webgl side changes

Comment 13

[Joe Drew (not getting mail)]

Attachment: part 0: support CORS in imagelib

This adds a load flag for the two CORS styles, and adds our CORS listener between our listener and the channel when necessary. It also makes sure that when we try to load from the image cache, we only allow an existing imgRequest to be used when both the CORS style is the same and the document principal is the same. (Otherwise, we reload from the network.)

Comment 14

[Joe Drew (not getting mail)]

Attachment: part 1: add the crossorigin attribute, and set the flag on LoadImage

This is an updated version of Benoit's patch that a) builds and b) sets the CORS load flags on LoadImage. I'm leaving it up to Benoit to request review.

Comment 15

[Joe Drew (not getting mail)]

Note: These patches are tested only to run. I have not tested whether the CORS stuff works at all. Angry Birds for the web is still broken, even in Chrome 14.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 543815 [details] [diff] [review]
part 0: support CORS in imagelib


>+// Returns true if this request is compatible with the given CORS mode on the
>+// given initial document URI, and false if the request may not be reused due
>+// to CORS.
>+static bool
>+ValidateCORS(imgRequest* request, CorsMode corsmode, nsIURI* initialDocumentURI)
>+{
>+  // If the entry's CORS mode doesn't match, or the CORS mode matches but the
>+  // document principal isn't the same, we can't use this request.
>+  if (request->GetCorsMode() != corsmode) {
>+    return false;
>+  } else if (request->GetCorsMode() != CORS_NONE) {
>+    nsCOMPtr<nsIScriptSecurityManager> secman = nsContentUtils::GetSecurityManager();
>+    nsCOMPtr<nsIPrincipal> docprincipal;
>+    secman->GetCodebasePrincipal(initialDocumentURI, getter_AddRefs(docprincipal));

Seems like you could just do:
nsContentUtils::GetSecurityManager()->GetCodebasePrincipal(initialDocumentURI, getter_AddRefs(docprincipal));

and do the same elsewhere.


>+    nsCOMPtr<nsIScriptSecurityManager> secman = nsContentUtils::GetSecurityManager();
>+    nsCOMPtr<nsIPrincipal> docprincipal;
>+    secman->GetCodebasePrincipal(aInitialDocumentURI, getter_AddRefs(docprincipal));
>+

Same here.

> 
>-    NS_ADDREF(pl);
>+    nsCOMPtr<nsIStreamListener> listener = pl;
>+
>+    if (corsmode != CORS_NONE) {
>+      if (NS_FAILED(rv)) {
>+        return NS_ERROR_FAILURE;
>+      }

Checking if we've failed here seems a little paranoid :)

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 543817 [details] [diff] [review]
part 1: add the crossorigin attribute, and set the flag on LoadImage


>+  /**
>+   * Used to get the CORS mode for the load.
>+   *
>+   * @return The CORS mode.
>+   */
>+  long getCORSMode();

I'd prefer this to be [noscript, notxpcom]
That should also let the implementation return the long instead of having to return the NS_RESULT.

Comment 18

[Joe Drew (not getting mail)]

Attachment: part 0: support CORS in imagelib, v2

Updated for review comments.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Why are we calling getCodebasePrincipal at all?  Do we not know what document (or at least what principal) that load is associated with at that point?  If so, can we just fix that?  All the callers have to know that info, and imagelib could do fix the longstanding data: image in canvas issue too if it just had the principal and stuck it on the channel as needed before opening it.... (which is a separate bug, but we still want the principal on this side of the imagelib API for that).

I'll look at the patch in detail tomorrow.

Comment 20

[Joe Drew (not getting mail)]

Attachment: part 0: pass the nsIPrincipal to loadImage

Good point. Let's pass the loading principal to loadImage.

Comment 21

[Joe Drew (not getting mail)]

Attachment: part 0.5: support CORS in imagelib v3

Building on part 0, now we can just use the nsIPrincipal passed in to loadImage to properly support CORS.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What caching policy does this implement? I.e. do we ever cache an image fetched from one of the three CORS modes such that it can be used in another CORS mode?

From a security perspective it might be safe to cache an image fetched using CORS_ANONYMOUS and CORS_USE_CREDENTIALS such that it's used for CORS_NONE requests, but I'm not sure that that's desirable from a behavioral point of view.

Comment 23

[Joe Drew (not getting mail)]

As currently implemented we only reuse cached elements with identical principals and CORS modes; this can be changed if we want. From a behavioural point of view I can't imagine using valid cached elements being a bad idea.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 544026 [details] [diff] [review]
part 0: pass the nsIPrincipal to loadImage

This seems ok, but please document in the various APIs involved that a null pointer is allowed and what it means.

And is there a reason GetLoadingPrincipal() can't just return the nsIPrincipal* without addrefing?

r=me with those nits.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 544027 [details] [diff] [review]
part 0.5: support CORS in imagelib v3

So this is relying on the fact that loadImage only takes the nsIRequest flags, so bits 16-31 are safe to reuse.  That needs to be documented somewhere.

The enum values there (both in the IDL and in the C++) need documenting too.

r=me with that; I didn't check the details of how you're using CORS here.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, as long as we separate by loading principal and CORS type we should be fine. There are lots of ways to screw up otherwise, with results ranging from DoS to leaking private data, so it's something I think we should avoid.

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 1: add the crossorigin attribute, and set the flag on LoadImage

This new version makes the crossOrigin attribute actually work: in particular, since it's an enum attribute, we had to implement nsHTMLImageElement::ParseAttribute

Comment 28

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: part 2: the mochitest

unfortunately I still get failures:

TEST-UNEXPECTED-FAIL |  | texImage2D on http://example.com/tests/content/canvas/test/webgl/crossorigin/image-allow-star.png with crossOrigin=anonymous: expected no error, got security error

TEST-UNEXPECTED-FAIL |  | texImage2D on http://example.com/tests/content/canvas/test/webgl/crossorigin/image-allow-credentials.png with crossOrigin=use-credentials: expected no error, got security error

can someone more competent than me please have a look?

Comment 29

[Boris Zbarsky [:bzbarsky]]

With the attached patches, the CORS mode is passed into imagelib, imagelib enforces the CORS restrictions, but the canvas same-origin checks are still there, no?  Those need to be changed to take into account the CORS info.  In particular, the canvas same-origin checks should only be performed if the image was loaded CORS_NONE, I believe.  Note that you have to get this information from the imgIRequest, not the element, because the CORS mode at load start may not match the current CORS mode.  (Do we need spec changes here to restart the load if @crossorigin changes, by the way?)

On a separate note, nsHTMLImageElement::GetCORSMode should use GetParsedAttr instead of what it does now; if the parsed value is an enum just return it, and otherwise return CORS_NONE.

Comment 30

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And can you please add a test for crossOrigin reflection, using content/html/content/test/reflect.js?

Comment 31

[Joe Drew (not getting mail)]

Attachment: part 0.5: support CORS in imagelib v4

In order to correctly allow cross-origin textures, we have to expose the CORS mode on imgIRequest. This patch does that by moving the CORS mode types into the idl file.

Comment 32

[Joe Drew (not getting mail)]

Attachment: part 0.5 v4 interdiff

This is an interdiff between v3 and v4 of part 0.5 for the easier reviewing.

Comment 33

[Joe Drew (not getting mail)]

Attachment: part 2: mochitest v2

It seems we had a misunderstanding about what "allow credentials" means in CORS. It doesn't mean that anonymous CORS isn't allowed, but rather that we can send along cookies and such.

Thus, I'm adjusting Benoit's test to allow anonymous loads of "allow credentials" CORS-using resources, and similarly to allow "allow credentials" loads of anonymous CORS-using resources.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 544902 [details] [diff] [review]
part 2: mochitest v2

Fantastic!

Comment 35

[Joe Drew (not getting mail)]

Attachment: part 1: crossorigin attribute v2

This implements the changes asked for by Boris. I've left the ParseAttribute body alone, though, presuming it's important.

I don't actually know who should review this; whoever does should probably also review the mochitest, though!

Comment 36

[Joe Drew (not getting mail)]

Attachment: part 1.5: relax the same-origin requirement in WebGL

This relaxes the same-origin requirement that we had in WebGL to also allow CORS-verified cross-origin images as textures. (We still disallow tainted canvases, though.)

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 544917 [details] [diff] [review]
part 1: crossorigin attribute v2

> I've left the ParseAttribute body alone, though, presuming it's important.

It is, yeah.  ;)

I'd really like to see some documentation for what these modes mean _somewhere_ here.  I don't know the CORS stuff well enough to suggest wording myself; maybe ping sicking?  The getter also needs documentation explaining that this is the CORS mode a load would use if you started one right now.... which is an odd thing to be exposing.  Why do we need this on the interface in the first place?

Assuming we do need this on the interface, you need to change the iid of the interface.  And is there a reason this is not a |readonly attribute long CORSMode| ?

Comment 38

[Boris Zbarsky [:bzbarsky]]

So if I read the CORS listener code correctly, the CORS_USE_CREDENTIALS case should fail the load if the server does not respond with "Access-Control-Allow-Credentials: true".

> and similarly to allow "allow credentials" loads of anonymous CORS-using
> resources.

Except those aren't allowed, per above.  What you did was change the test so it has two instances of this:

+    testTexture("http://example.com/tests/content/canvas/test/webgl/crossorigin/image-allow-credentials.png",
+                "use-credentials",
+                OK);

One of those should go and probably be replaced with an instance of this:

+    testTexture("http://example.com/tests/content/canvas/test/webgl/crossorigin/image-allow-star.png",
+                "use-credentials",
+                SECURITY_ERR);

It's also worth testing that using a bogus value of the crossorigin attribute is treated just like having it not set at all.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 544919 [details] [diff] [review]
part 1.5: relax the same-origin requirement in WebGL

I'd rather we explicitly set the mode to NONE if the getter throws instead of relying on it not touching the mode.

r=me with that.

Comment 40

[Joe Drew (not getting mail)]

(In reply to comment #37)
> I'd really like to see some documentation for what these modes mean
> _somewhere_ here.  I don't know the CORS stuff well enough to suggest
> wording myself; maybe ping sicking?

I was able to put something together from my readings of the HTML5 spec.

>  The getter also needs documentation
> explaining that this is the CORS mode a load would use if you started one
> right now.... which is an odd thing to be exposing.  Why do we need this on
> the interface in the first place?

As far as I can tell, it's required because nsHTMLImageElement uses nsImageLoadingContent to actually load images, and in order to have CORS passed to imgLoader, we need that information at the nsImageLoadingContent layer.
 
> Assuming we do need this on the interface, you need to change the iid of the
> interface.  And is there a reason this is not a |readonly attribute long
> CORSMode| ?

I can't explain it - I just inherited this code from Benoit. I'll change it. :)

Also, I'm going to change the uuid of nsIHTMLImageElement.

Comment 41

[Boris Zbarsky [:bzbarsky]]

> and in order to have CORS passed to imgLoader, we need that information at the > nsImageLoadingContent layer.

We need it on nsImageLoadingContent, sure.  So make it a virtual method on that class.  But it doesn't need to be on nsIImageLoadingContent at all, I think.

Comment 42

[Joe Drew (not getting mail)]

Attachment: part 1: crossorigin attribute v3

Very good! This implements those requested changes.

Comment 43

[Joe Drew (not getting mail)]

(In reply to comment #38)
> So if I read the CORS listener code correctly, the CORS_USE_CREDENTIALS case
> should fail the load if the server does not respond with
> "Access-Control-Allow-Credentials: true".

That matches my reading of the CORS spec too.

> What you did was change the test so it has two instances of this:

Yeah, I was probably sleep-deprived.

> It's also worth testing that using a bogus value of the crossorigin
> attribute is treated just like having it not set at all.

Will test.

Comment 44

[Joe Drew (not getting mail)]

Attachment: part 2: mochitest v3

Turns out that we get an error (as in, the image gets onerror and not onload) while loading images if we have Access-Control-Allow-Origin: * and ask for credentials with CORS, so we have to handle that too.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 545474 [details] [diff] [review]
part 1: crossorigin attribute v3

The nsAlertsIconListener.cpp change needs to be in the same patch as the interface change; otherwise we'll be pushing non-compiling changesets.

r=me with that fixed.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 545501 [details] [diff] [review]
part 2: mochitest v3

r=me

Comment 47

[Joe Drew (not getting mail)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5977286eda3a
http://hg.mozilla.org/integration/mozilla-inbound/rev/24b7b80bb3c3
http://hg.mozilla.org/integration/mozilla-inbound/rev/1dd4abff0da3
http://hg.mozilla.org/integration/mozilla-inbound/rev/1d535d70b54d
http://hg.mozilla.org/integration/mozilla-inbound/rev/255eaf3959f5

Comment 48

[Joe Drew (not getting mail)]

The dev doc we need here is that the HTML5 crossorigin attribute on images is now supported; links regarding it are in comment 1.

Comment 49

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #30)
> And can you please add a test for crossOrigin reflection, using
> content/html/content/test/reflect.js?

Comment 50

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/5977286eda3a
http://hg.mozilla.org/mozilla-central/rev/24b7b80bb3c3
http://hg.mozilla.org/mozilla-central/rev/1dd4abff0da3
http://hg.mozilla.org/mozilla-central/rev/1d535d70b54d
http://hg.mozilla.org/mozilla-central/rev/255eaf3959f5

Comment 51

[Joe Drew (not getting mail)]

Attachment: test that crossOrigin is reflected properly

(In reply to comment #49)
> (In reply to comment #30)
> > And can you please add a test for crossOrigin reflection, using
> > content/html/content/test/reflect.js?

Sorry, totally forgot.

Comment 52

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 546269 [details] [diff] [review]
test that crossOrigin is reflected properly

>Bug 664299 - Test that the crossOrigin attribute is probably reflected into JS. r=Ms2ger

"is properly reflected"

>--- a/content/html/content/test/Makefile.in
>+++ b/content/html/content/test/Makefile.in
>@@ -271,12 +271,13 @@ _TEST_FILES = \
> 		test_bug586786.html \
> 		test_bug649134.html \
> 		test_bug658746.html \
> 		test_bug659596.html \
> 		test_bug659743.xml \
> 		test_bug660663.html \
> 		test_bug666666.html \
> 		test_restore_from_parser_fragment.html \
>+		test_bug664299.html \

This one should go between 660663 and 666666.

r=me with that

Comment 53

[Joe Drew (not getting mail)]

(In reply to comment #52)
> >--- a/content/html/content/test/Makefile.in
> >+++ b/content/html/content/test/Makefile.in
> >@@ -271,12 +271,13 @@ _TEST_FILES = \
> > 		test_bug586786.html \
> > 		test_bug649134.html \
> > 		test_bug658746.html \
> > 		test_bug659596.html \
> > 		test_bug659743.xml \
> > 		test_bug660663.html \
> > 		test_bug666666.html \
> > 		test_restore_from_parser_fragment.html \
> >+		test_bug664299.html \
> 
> This one should go between 660663 and 666666.

Note that the list is not in sorted order in general, though the quoted section is.

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I know; I'd still like to keep that section sorted.

Comment 55

[Joe Drew (not getting mail)]

The reflection test is inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/86dd8454de0b

Comment 56

[Joe Drew (not getting mail)]

And now the reflection test's been merged to m-c: http://hg.mozilla.org/mozilla-central/rev/86dd8454de0b

Comment 57

[Cedric Vivier [:cedricv]]

Any luck to have this in Firefox 6?

Comment 58

[Boris Zbarsky [:bzbarsky]]

Feature freeze for Firefox 6 was May 24.  So without a time machine, no.  Also probably not going to be in 7 (feature freeze was July 5).

Comment 59

[Benoit Jacob [:bjacob] (mostly away)]

Thanks a bunch for driving this. I would like very much this to go into Firefox 7, but at the same time I understand that it's branched already. A possible reason to make an exception for this is that it helps our relation with web developers to offer as soon as possible this way to get their sites to resume working in Firefox. I know that major web developers, including at Google, are very interested in us shipping this feature ASAP. Anyway I'm not the one deciding on this :)

Comment 60

[Boris Zbarsky [:bzbarsky]]

Benoit, if you think some set of patches here should be landing on Aurora, please nominate them accordingly.

Comment 61

[Benoit Jacob [:bjacob] (mostly away)]

Nominated.

Comment 62

[Joe Drew (not getting mail)]

This needs to go in with the patches in bug 671906 or not at all; that bug unbroke this.

Comment 63

[LegNeato]

During triage today we decided to let this go through the normal process rather than pushing it into Aurora. The decision hurts and we know it isn't ideal from a web developer standpoint but we think it's the right one.

Comment 64

[Jeff Muizelaar [:jrmuizel]]

Some notes on add-on compatibility:
- the interfaces that changed are: imgILoader and imgIRequest and nsIDOMHTMLImageElement

- nsIDOMHTMLImageElement has an added attribute
 - this shouldn't hurt script addons

- imgIRequest has an added attribute
 - this shouldn't hurt script addons
 - this interface is used by addons like firebug, a request viewer, an image saver. I don't know if there are a lot of binary addons that would use it.

- imgILoader changes the signature of LoadImage
 - Two addons on AMO use imgILoader, one use is commented out and the other does not use LoadImage

Comment 65

[LegNeato]

Have we checked if CoolIris or stuff like HPSmartWebPrinting use these interfaces?

Comment 66

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #65)
> Have we checked if CoolIris or stuff like HPSmartWebPrinting use these
> interfaces?

How do we do that?

Comment 67

[chris hofmann]

fligtar and jorge may still have the Cooliris contacts, and maybe kev has HP contacts

Comment 68

[Jorge Villalobos [:jorgev] (he/him)]

Adding some known Cooliris contacts to the CC list.

Comment 69

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

removed sec-review flag as we are comfortable with CORS usage here

Comment 70

[Jeff]

I will check the Cooliris code.  This is will be changing in FF/Gecko 8.0?

Comment 71

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Jeff from comment #70)
> I will check the Cooliris code.  This is will be changing in FF/Gecko 8.0?

Yes.

Comment 72

[Jeff]

We are not currently using imgILoader, imgIRequest or nsIDOMHTMLImageElement.  Thanks for the heads up.  If you'd like a complete list of interfaces we do use, I can provide that.  :)

Comment 73

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff from comment #72)
> We are not currently using imgILoader, imgIRequest or
> nsIDOMHTMLImageElement.  Thanks for the heads up.  If you'd like a complete
> list of interfaces we do use, I can provide that.  :)

That would be great.

Comment 74

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Need to schedule this for a full secteam review who should I talk to make this happen

Comment 75

[Joe Drew (not getting mail)]

Benoit Jacob or me.

Comment 76

[Jean-Yves Perrier [:teoli]]

We have the following documentation for this:
CORS-enabled image definition: https://developer.mozilla.org/en/CORS_Enabled_Image
CORS-enabled settings attribute definition: https://developer.mozilla.org/en/HTML/CORS_settings_attributes
The crossOrigin attribute on the HTMLImageElement page: https://developer.mozilla.org/en/HTML/CORS_settings_attributes
The crossorigin attribut on the <img> element: https://developer.mozilla.org/En/HTML/Element/Img

And an entry on Firefox 8 for developers: https://developer.mozilla.org/en/firefox_8_for_developers

I think it is enough to change the keyword to dev-doc-complete, though we should, in the future, revisit all CORS documentation to make it more coherent and understandable (lots of Web Developers have problems understanding CORS)

(Most of this doc was written by Sheppy, and Paul Irish; I just did the <img> HTML attribute crossorigin bit)

Comment 77

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Benoit: since joedrew is away for a while do you want to just schedule this and get it done or do we need him for the review too?

Comment 78

[Benoit Jacob [:bjacob] (mostly away)]

Joe would have been a better person since, contrary to me, he understand the ImageLib side of things, but given that this is for Firefox 8, I suppose we need to schedule this ASAP.

How do we go about scheduling this? I don't know what day works for you. Let me make a random suggestion: how about this Thursday morning? 10 AM east coast time?

Comment 79

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Generally you pick a date from https://mail.mozilla.com/home/[e-mail]/Security%20Review.html, lucky for you this Thu at 10am is still open. I will schedule it.

Comment 80

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Benoit: sorry that review slot is 10am Pacific, not eastern does that still work for you? 10am EST would be 7am Pacific and noone will attend that

Comment 81

[Benoit Jacob [:bjacob] (mostly away)]

Oh right, sorry. 10 am Pacific works for me.

Comment 82

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

SecReview scheduled for 20-Oct-2011 at 10am PST

Comment 83

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Notes from review: https://wiki.mozilla.org/Security/Reviews/crossoriginAttribute

Comment 84

[Ioana (away)]

The test that verifies the fix for this bug (test_bug664299.html) is passing on all OS:

Firefox 8.0 (beta):
https://tbpl.mozilla.org/php/getParsedLog.php?id=7158461&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=7160589&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=7158593&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=7158600&full=1&branch=mozilla-beta

Firefox 9.0 (aurora):
https://tbpl.mozilla.org/php/getParsedLog.php?id=7155263&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7155779&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7156339&full=1&branch=mozilla-aurora
https://tbpl.mozilla.org/php/getParsedLog.php?id=7156332&full=1&branch=mozilla-aurora

Firefox 10.0 (central):
https://tbpl.mozilla.org/php/getParsedLog.php?id=7148007&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7150553&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7148795&full=1&branch=services-central
https://tbpl.mozilla.org/php/getParsedLog.php?id=7148858&full=1&branch=services-central