Bug 61015 - Support crossorigin property for images
: Support crossorigin property for images
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Enhancement
Assigned To:
:
:
: 61209 61223 61318
: 62257
  Show dependency treegraph
 
Reported: 2011-05-17 18:56 PST by
Modified: 2011-06-07 19:26 PST (History)


Attachments
Work in progress (5.76 KB, patch)
2011-05-25 12:56 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Work in progress (8.41 KB, patch)
2011-05-25 14:45 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Work in progress (24.86 KB, patch)
2011-05-25 15:25 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.62 KB, patch)
2011-05-25 16:29 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (28.87 KB, patch)
2011-05-26 11:35 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (161.34 KB, application/zip)
2011-05-26 12:11 PST, WebKit Review Bot
no flags Details
Patch for landing (28.86 KB, patch)
2011-05-26 13:57 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (28.85 KB, patch)
2011-05-26 16:46 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2011-05-17 18:56:39 PST
Based on feedback from the WebGL working group, the WHATWG has added a cross-origin property to image, video and audio elements allowing CORS requests to be issued for their download. This was needed because the WebGL specification will need to be updated to ban the use of cross-origin media for security reasons.

Support for this new property is needed to allow WebGL applications using cross-origin media to continue to work.

Related links:

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
------- Comment #1 From 2011-05-18 11:25:51 PST -------
Note that a bug was just filed about the naming of the new attribute. This would be trivial to change so shouldn't block implementation of the core feature.

http://www.w3.org/Bugs/Public/show_bug.cgi?id=12679
------- Comment #2 From 2011-05-18 17:01:35 PST -------
I suspect this will be difficult to implement, but I look forward to your patch.
------- Comment #3 From 2011-05-20 12:39:31 PST -------
I've started looking at this.
------- Comment #4 From 2011-05-23 16:13:59 PST -------
Note that the name of the new attribute was in fact just changed per http://www.w3.org/Bugs/Public/show_bug.cgi?id=12679 .
------- Comment #5 From 2011-05-23 16:22:25 PST -------
For those of us following along at home, what's the purpose of this content attribute? How does it affect the request (which is presumably still a "simple" one)?
------- Comment #6 From 2011-05-23 16:39:25 PST -------
> For those of us following along at home, what's the purpose of this content attribute? How does it affect the request (which is presumably still a "simple" one)?

The attribute has two effects:

1) The request becomes a CORS request, so it gets an outgoing Origin header.  I'll have to read the spec, but the image load might be blocked if CORS says no.

2) The attribute lets you control whether to make an anonymous request (as in CORS), so you can strip cookies / HTTP auth from the request if you like.

Apparently we can't just always use non-anonymous CORS for image loads because some servers get sad when you send them unexpected Origin headers.
------- Comment #7 From 2011-05-23 16:41:53 PST -------
I forgot the mention that the point of the attribute is to let you draw images onto 2D and 3D canvases and read back the results.  In the case of 3D canvas, my understanding is that a thumbs up for CORS is going to be required for drawing the image at all.
------- Comment #8 From 2011-05-25 12:56:18 PST -------
Created an attachment (id=94842) [details]
Work in progress
------- Comment #9 From 2011-05-25 14:45:38 PST -------
Created an attachment (id=94864) [details]
Work in progress
------- Comment #10 From 2011-05-25 15:25:46 PST -------
Created an attachment (id=94870) [details]
Work in progress
------- Comment #11 From 2011-05-25 16:26:23 PST -------
Audio and Video will be a separate patch.
------- Comment #12 From 2011-05-25 16:29:41 PST -------
Created an attachment (id=94880) [details]
Patch
------- Comment #13 From 2011-05-26 07:27:13 PST -------
(From update of attachment 94880 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=94880&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:409
> +SecurityOrigin* HTMLCanvasElement::securityOrigin() const

Are we changing this because SecurityOrigin* can sometimes be NULL?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1584
> +    bool originClean = false;
> +    if (cachedImage->image()->hasSingleSecurityOrigin()) {
> +        originClean = cachedImage->passesAccessControlCheck(canvas()->securityOrigin())
> +            || !canvas()->securityOrigin()->taintsCanvas(cachedImage->response().url());
> +    }

I would have just made this a helper.  isOriginClean(canvas(), cachedImage); ten you can use early return, etc.

> Source/WebCore/loader/ImageLoader.cpp:168
> +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);

Where is this free-function defined?  (Why is it free?)  And should we just be passing the crossOriginMode instead?  I wonder if this whole block shouldn't be a separate function in ImageLoader, and then this call is just:

updateAccessControlsForCrossOriginMode(crossOriginMode)

> Source/WebCore/loader/cache/CachedResource.cpp:179
> +    return WebCore::passesAccessControlCheck(m_response, resourceRequest().allowCookies(), securityOrigin, errorDescription);

Do you need/want the WebCore:: here?  Should this function take a String* or StringImpl* since this is an optional return?
------- Comment #14 From 2011-05-26 10:08:18 PST -------
(In reply to comment #13)
> (From update of attachment 94880 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94880&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.cpp:409
> > +SecurityOrigin* HTMLCanvasElement::securityOrigin() const
> 
> Are we changing this because SecurityOrigin* can sometimes be NULL?

No, but SecurityOrigin is a pointer-type not a reference-type everywhere else in the code.  This code is just randomly different.

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1584
> > +    bool originClean = false;
> > +    if (cachedImage->image()->hasSingleSecurityOrigin()) {
> > +        originClean = cachedImage->passesAccessControlCheck(canvas()->securityOrigin())
> > +            || !canvas()->securityOrigin()->taintsCanvas(cachedImage->response().url());
> > +    }
> 
> I would have just made this a helper.  isOriginClean(canvas(), cachedImage); ten you can use early return, etc.

Ok.

> > Source/WebCore/loader/ImageLoader.cpp:168
> > +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
> 
> Where is this free-function defined?

CrossOriginAccessControl.h

> (Why is it free?)

Because it has no associated state.

> And should we just be passing the crossOriginMode instead?

Nope.  updateRequestForAccessControl is a lower-level primitive shared by all CORS APIs.  crossOriginMode is specific to this API.

> I wonder if this whole block shouldn't be a separate function in ImageLoader, and then this call is just:
> 
> updateAccessControlsForCrossOriginMode(crossOriginMode)

It's eventually going to be shared by Images, Audio, and Video.  My plan was to move it somewhere else when implementing those APIs since the correct place will be more obvious then.

> > Source/WebCore/loader/cache/CachedResource.cpp:179
> > +    return WebCore::passesAccessControlCheck(m_response, resourceRequest().allowCookies(), securityOrigin, errorDescription);
> 
> Do you need/want the WebCore:: here?

It's needed to call the correct function because the free function and the member function have the same name.

> Should this function take a String* or StringImpl* since this is an optional return?

Dunno.  I'm not sure it matters that much.  I guess we could pass 0, which might be slightly better.  Seems like an issue for another patch.
------- Comment #15 From 2011-05-26 11:35:23 PST -------
Created an attachment (id=95011) [details]
Patch
------- Comment #16 From 2011-05-26 12:07:17 PST -------
(From update of attachment 95011 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review

OK.  I didn't review the tests particularly closely.  If you need me to, I'm happy to take another look.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:92
> +static bool isOriginClean(CachedImage* cachedImage, SecurityOrigin* securityOrigin)

OMG, so much nicer, thank you.

> Source/WebCore/loader/ImageLoader.cpp:168
> +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);

I really dislike these being free functions.  We should consider namspacing them in a class at some point.
------- Comment #17 From 2011-05-26 12:11:03 PST -------
(From update of attachment 95011 [details])
Attachment 95011 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8739244

New failing tests:
canvas/philip/tests/2d.pattern.paint.norepeat.outside.html
canvas/philip/tests/2d.pattern.paint.repeatx.outside.html
canvas/philip/tests/2d.composite.globalAlpha.imagepattern.html
canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html
canvas/philip/tests/2d.pattern.paint.norepeat.coord1.html
canvas/philip/tests/2d.pattern.paint.repeat.basic.html
canvas/philip/tests/2d.pattern.paint.repeat.coord1.html
canvas/philip/tests/2d.pattern.modify.image1.html
canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html
canvas/philip/tests/2d.pattern.animated.gif.html
canvas/philip/tests/2d.pattern.paint.repeat.outside.html
canvas/philip/tests/2d.pattern.modify.image2.html
canvas/philip/tests/2d.pattern.paint.norepeat.coord2.html
canvas/philip/tests/2d.pattern.paint.orientation.image.html
canvas/philip/tests/2d.pattern.paint.norepeat.basic.html
canvas/philip/tests/2d.pattern.paint.repeat.coord3.html
canvas/philip/tests/2d.pattern.paint.repeat.coord2.html
canvas/philip/tests/2d.pattern.paint.repeatx.basic.html
canvas/philip/tests/2d.pattern.crosscanvas.html
canvas/philip/tests/2d.pattern.basic.image.html
------- Comment #18 From 2011-05-26 12:11:07 PST -------
Created an attachment (id=95017) [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #19 From 2011-05-26 12:14:51 PST -------
(From update of attachment 95011 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review

>> Source/WebCore/loader/ImageLoader.cpp:168
>> +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
> 
> I really dislike these being free functions.  We should consider namspacing them in a class at some point.

I guess beauty is in the eye of the beholder. I like them as free functions.
------- Comment #20 From 2011-05-26 12:19:01 PST -------
(In reply to comment #19)
> (From update of attachment 95011 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review
> 
> >> Source/WebCore/loader/ImageLoader.cpp:168
> >> +            updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
> > 
> > I really dislike these being free functions.  We should consider namspacing them in a class at some point.
> 
> I guess beauty is in the eye of the beholder. I like them as free functions.

So true. :)  I find it confusing that he has to WebCore:: namespace them in one place.  We have very few free functions in WebCore.  But I admit to not having looked at these in great detail.
------- Comment #21 From 2011-05-26 13:20:13 PST -------
> I find it confusing that he has to WebCore:: namespace them in one place.

I can tweak some of the names so they don't collide.  Then we won't need the WebCore prefix.
------- Comment #22 From 2011-05-26 13:54:41 PST -------
> I didn't review the tests particularly closely.  If you need me to, I'm happy to take another look.

The tests are based on the existing image-canvas tainting tests, but with the expected results tweaked to match the CORS status of the request/image.
------- Comment #23 From 2011-05-26 13:57:18 PST -------
Created an attachment (id=95032) [details]
Patch for landing
------- Comment #24 From 2011-05-26 16:23:28 PST -------
(From update of attachment 95011 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review

> Source/WebCore/html/HTMLCanvasElement.h:118
> -    const SecurityOrigin& securityOrigin() const;
> +    SecurityOrigin* securityOrigin() const;

I kind of like the old signature. It’s kind of weak to offer a security origin object that you can mutate for a canvas. We don’t want a caller to get it and then call something like setDomainFromDOM on it, do we? What’s the rationale here?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:95
> +    if (cachedImage->image()->hasSingleSecurityOrigin())
> +        return false;

Is there a ! missing here?

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:73
> -    checkOrigin(KURL(KURL(), video->currentSrc()));
> +    // FIXME: HTMLVideoElement::currentSrc() should return a KURL.
> +    checkOrigin(KURL(ParsedURLString, video->currentSrc()));

Why change to this intermediate state? I am hoping we can get rid of ParsedURLString soon. Can’t we wait and not touch this call site until we fixed currentSrc to return a URL?

> Source/WebCore/loader/ImageLoader.cpp:164
> +        String crossOriginMode = m_element->getAttribute(HTMLNames::crossoriginAttr);

Could use fastGetAttribute. That’s always allowed for any attribute that is guaranteed to not be an SVG animated attribute or the style attribute. And it’s faster.

> Source/WebCore/loader/ImageLoader.cpp:167
> +            DEFINE_STATIC_LOCAL(String, useCredentials, ("use-credentials"));
> +            bool allowCredentials = equalIgnoringCase(crossOriginMode, useCredentials);

I’m not sure that having a static local string is a more efficient way to do the equality check. You’d think that an equality check with a const char* could be super fast.
------- Comment #25 From 2011-05-26 16:31:36 PST -------
(In reply to comment #24)
> (From update of attachment 95011 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review
> 
> > Source/WebCore/html/HTMLCanvasElement.h:118
> > -    const SecurityOrigin& securityOrigin() const;
> > +    SecurityOrigin* securityOrigin() const;
> 
> I kind of like the old signature. It’s kind of weak to offer a security origin object that you can mutate for a canvas. We don’t want a caller to get it and then call something like setDomainFromDOM on it, do we? What’s the rationale here?

The rationale is just consistency with everywhere else where we handle SecurityOrigin objects.  In this case, we want to pass the object to other functions.  There was already code that took the address of the reference, which seems somewhat goofy.

Maybe "SecurityOrigin const*" would be the best type?

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:95
> > +    if (cachedImage->image()->hasSingleSecurityOrigin())
> > +        return false;
> 
> Is there a ! missing here?

Yep.  I added it back in the final patch.  (It causes a massive number of test failures, thankfully.)

> > Source/WebCore/html/canvas/CanvasRenderingContext.cpp:73
> > -    checkOrigin(KURL(KURL(), video->currentSrc()));
> > +    // FIXME: HTMLVideoElement::currentSrc() should return a KURL.
> > +    checkOrigin(KURL(ParsedURLString, video->currentSrc()));
> 
> Why change to this intermediate state? I am hoping we can get rid of ParsedURLString soon. Can’t we wait and not touch this call site until we fixed currentSrc to return a URL?

I started down that path, but the scope was bigger than I wanted to include in this patch.  I'll do that in Bug 61578.

> > Source/WebCore/loader/ImageLoader.cpp:164
> > +        String crossOriginMode = m_element->getAttribute(HTMLNames::crossoriginAttr);
> 
> Could use fastGetAttribute. That’s always allowed for any attribute that is guaranteed to not be an SVG animated attribute or the style attribute. And it’s faster.

Thanks.  I wasn't familiar with the details of when you can use fastGetAttribute.  I'll fix that.

> > Source/WebCore/loader/ImageLoader.cpp:167
> > +            DEFINE_STATIC_LOCAL(String, useCredentials, ("use-credentials"));
> > +            bool allowCredentials = equalIgnoringCase(crossOriginMode, useCredentials);
> 
> I’m not sure that having a static local string is a more efficient way to do the equality check. You’d think that an equality check with a const char* could be super fast.

Will do.
------- Comment #26 From 2011-05-26 16:46:56 PST -------
Created an attachment (id=95078) [details]
Patch for landing
------- Comment #27 From 2011-05-26 23:12:58 PST -------
(From update of attachment 95078 [details])
Clearing flags on attachment: 95078

Committed r87473: <http://trac.webkit.org/changeset/87473>
------- Comment #28 From 2011-05-26 23:13:05 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #29 From 2011-05-30 22:02:00 PST -------
Why crossOrigin and not webkitCrossOrigin?  Are we already certain this will become a standard?
------- Comment #30 From 2011-05-30 22:36:46 PST -------
Once this is shipped, if the standard changes incompatibly I'll use a different name.
------- Comment #31 From 2011-05-30 22:48:02 PST -------
(In reply to comment #29)
> Why crossOrigin and not webkitCrossOrigin?  Are we already certain this will become a standard?

It's part of HTML5, and, as Ian says, attributes are easier to rename later.