Bug 61015 - Support crossorigin property for images
: Support crossorigin property for images
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Enhancement
Assigned To: Adam Barth
:
Depends on: 61209 61223 61318
Blocks: 62257
  Show dependency treegraph
 
Reported: 2011-05-17 18:56 PDT by Kenneth Russell
Modified: 2011-06-07 19:26 PDT (History)
14 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2011-05-17 18:56:39 PDT
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 Kenneth Russell 2011-05-18 11:25:51 PDT
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 Adam Barth 2011-05-18 17:01:35 PDT
I suspect this will be difficult to implement, but I look forward to your patch.
Comment 3 Adam Barth 2011-05-20 12:39:31 PDT
I've started looking at this.
Comment 4 Kenneth Russell 2011-05-23 16:13:59 PDT
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 Alexey Proskuryakov 2011-05-23 16:22:25 PDT
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 Adam Barth 2011-05-23 16:39:25 PDT
> 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 Adam Barth 2011-05-23 16:41:53 PDT
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 Adam Barth 2011-05-25 12:56:18 PDT
Created attachment 94842 [details]
Work in progress
Comment 9 Adam Barth 2011-05-25 14:45:38 PDT
Created attachment 94864 [details]
Work in progress
Comment 10 Adam Barth 2011-05-25 15:25:46 PDT
Created attachment 94870 [details]
Work in progress
Comment 11 Adam Barth 2011-05-25 16:26:23 PDT
Audio and Video will be a separate patch.
Comment 12 Adam Barth 2011-05-25 16:29:41 PDT
Created attachment 94880 [details]
Patch
Comment 13 Eric Seidel 2011-05-26 07:27:13 PDT
Comment on attachment 94880 [details]
Patch

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 Adam Barth 2011-05-26 10:08:18 PDT
(In reply to comment #13)
> (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?

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 Adam Barth 2011-05-26 11:35:23 PDT
Created attachment 95011 [details]
Patch
Comment 16 Eric Seidel 2011-05-26 12:07:17 PDT
Comment on attachment 95011 [details]
Patch

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 WebKit Review Bot 2011-05-26 12:11:03 PDT
Comment on attachment 95011 [details]
Patch

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 WebKit Review Bot 2011-05-26 12:11:07 PDT
Created attachment 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 Darin Adler 2011-05-26 12:14:51 PDT
Comment on attachment 95011 [details]
Patch

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 Eric Seidel 2011-05-26 12:19:01 PDT
(In reply to comment #19)
> (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.

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 Adam Barth 2011-05-26 13:20:13 PDT
> 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 Adam Barth 2011-05-26 13:54:41 PDT
> 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 Adam Barth 2011-05-26 13:57:18 PDT
Created attachment 95032 [details]
Patch for landing
Comment 24 Darin Adler 2011-05-26 16:23:28 PDT
Comment on attachment 95011 [details]
Patch

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 Adam Barth 2011-05-26 16:31:36 PDT
(In reply to comment #24)
> (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?

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 Adam Barth 2011-05-26 16:46:56 PDT
Created attachment 95078 [details]
Patch for landing
Comment 27 WebKit Commit Bot 2011-05-26 23:12:58 PDT
Comment on attachment 95078 [details]
Patch for landing

Clearing flags on attachment: 95078

Committed r87473: <http://trac.webkit.org/changeset/87473>
Comment 28 WebKit Commit Bot 2011-05-26 23:13:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Darin Fisher (:fishd, Google) 2011-05-30 22:02:00 PDT
Why crossOrigin and not webkitCrossOrigin?  Are we already certain this will become a standard?
Comment 30 Ian 'Hixie' Hickson 2011-05-30 22:36:46 PDT
Once this is shipped, if the standard changes incompatibly I'll use a different name.
Comment 31 Adam Barth 2011-05-30 22:48:02 PDT
(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.