672014 - Image preloading needs to take crossorigin attribute into account

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Joe Drew (not getting mail)]

Right now, the HTML5 parser preloads image elements with no attributes set. Unfortunately, if you've specified the crossorigin attribute with the value anonymous or use-credentials, we can't do an attribute-free load, because we have to take CORS into account (sending certain headers, and checking what the server gives us back.)

What we do right now is immediately verify the image over the network (inserting a CORS listener proxy in the listener chain), because the CORS mode the image was loaded with (CORS_NONE) doesn't match the CORS mode requested by the nsHTMLImageElement (CORS_ anything else).

Comment 1

[Boris Zbarsky [:bzbarsky]]

So in particular, we need to either not preload images with @crossorigin or we need to pass the value of that attribute to the document's preloading code, which can then call the imagelib APIs the right way.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial fix

This needs ATTR_CROSSORIGIN added to the java code, etc... but other than that, does it look sane?

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Add the pre-interned attribute to the parser

Comment 4

[Henri Sivonen (:hsivonen)]

Comment on attachment 548363 [details] [diff] [review]
Partial fix

>-    inline void InitImage(const nsAString& aUrl) {
>+    inline void InitImage(const nsAString& aUrl,
>+                          const nsAString* aCrossOriginAttr) {

The inconsistency in the argument types isn't nice. Please make the new argument be of type const nsAString& instead of const nsAString* and move the pointer dereference to the caller (or pass EmptyString() where you are currently passing nsnull).

>   private:
>     eHtml5SpeculativeLoad mOpCode;
>     nsString mUrl;
>     nsString mCharset;
>     nsString mType;
>+    // XXXbz does it make sense to reuse one of the existing nsStrings
>+    // for this, or just not worry about it?
>+    nsString mCrossOriginAttr;

I don't know if worrying about it is truly worthwhile in terms of footprint and perf, but it seems to me it wouldn't hurt to have a field mCharsetOrCrossorigin whose semantics are given by mOpCode.

>         } else if (nsHtml5Atoms::video == aName) {
>           nsString* url = aAttributes->getValue(nsHtml5AttributeName::ATTR_POSTER);
>           if (url) {
>-            mSpeculativeLoadQueue.AppendElement()->InitImage(*url);
>+            mSpeculativeLoadQueue.AppendElement()->InitImage(*url, nsnull);

The spec has the crossorigin attribute on <video> and <audio>, too. I guess the above line makes sense if Gecko doesn't support crossorigin for video posterframes, yet, but if that's the case, shouldn't there be a follow-up bug about making video, audio and poster frame loads CORS aware?

>       case kNameSpaceID_SVG:
>         if (nsHtml5Atoms::image == aName) {
>           nsString* url = aAttributes->getValue(nsHtml5AttributeName::ATTR_XLINK_HREF);
>           if (url) {
>-            mSpeculativeLoadQueue.AppendElement()->InitImage(*url);
>+            mSpeculativeLoadQueue.AppendElement()->InitImage(*url, nsnull);

Is there any ongoing spec and code follow-up for making SVG images support CORS? (Do we support painting SVG to <canvas>? IIRC, Opera supports it.)

r=hsivonen with the s/const nsAString*/const nsAString&/ change in InitImage.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> and move the pointer dereference to the caller 

I can do that (and that's where I started), but it causes uglier code in the caller.  In any case, will do.

> but it seems to me it wouldn't hurt to have a field mCharsetOrCrossorigin

OK, will do.

> shouldn't there be a follow-up bug about making video, audio and poster frame
> loads CORS aware?

Probably yes.

> Is there any ongoing spec and code follow-up for making SVG images support
> CORS?

Not that I know of....

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 548391 [details] [diff] [review]
Add the pre-interned attribute to the parser

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments and with Henri's attribute addition merged in

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 548513 [details] [diff] [review]
Updated to comments and with Henri's attribute addition merged in

Looks good.

Comment 9

[Joe Drew (not getting mail)]

(In reply to comment #5)
> > Is there any ongoing spec and code follow-up for making SVG images support
> > CORS?
> 
> Not that I know of....

<img src="foo.svg"> will transparently support CORS with the code added in bug 664299. Or did you mean something else?

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 548513 [details] [diff] [review]
Updated to comments and with Henri's attribute addition merged in

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

::: content/base/src/nsDocument.cpp
@@ +7692,5 @@
> +    loadFlags |= imgILoader::LOAD_CORS_USE_CREDENTIALS;
> +  }
> +  // else should we err on the side of not doing the preload if
> +  // aCrossOriginAttr is nonempty?  Let's err on the side of doing the
> +  // preload as CORS_NONE.

That's how we'll load, so it makes sense to preload the same way.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Henri means <svg:image src="something.gif">

> That's how we'll load,

Not necessarily.  Consider <img crossorigin="   UsE-CREDentials  ">

In this case the preload will happen CORS_NONE, but the actual load will be CORS_USE_CREDENTIALS.  But I don't think duplicating the exact attribute parsing here is worthwhile.

Comment 12

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f5aecf9010ef

Comment 13

[Boris Zbarsky [:bzbarsky]]

And backed out because of build bustage; I somehow managed to push an old version of the patch....

Comment 14

[Boris Zbarsky [:bzbarsky]]

Now it's fine: http://hg.mozilla.org/integration/mozilla-inbound/rev/185e9d01a1ec

Comment 15

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/185e9d01a1ec