Last Comment Bug 672014 - Image preloading needs to take crossorigin attribute into account
: Image preloading needs to take crossorigin attribute into account
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 664299
  Show dependency treegraph
 
Reported: 2011-07-15 19:38 PDT by Joe Drew (not getting mail)
Modified: 2011-08-01 07:51 PDT (History)
4 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Partial fix (9.39 KB, patch)
2011-07-25 21:35 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
hsivonen: review+
Details | Diff | Review
Add the pre-interned attribute to the parser (44.11 KB, patch)
2011-07-26 01:10 PDT, Henri Sivonen (:hsivonen)
bzbarsky: review+
Details | Diff | Review
Updated to comments and with Henri's attribute addition merged in (56.33 KB, patch)
2011-07-26 10:15 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
joe: review+
hsivonen: review+
Details | Diff | Review

Description Joe Drew (not getting mail) 2011-07-15 19:38:07 PDT
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 [:bz] (Out June 25-July 6) 2011-07-15 20:03:14 PDT
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 [:bz] (Out June 25-July 6) 2011-07-25 21:35:15 PDT
Created attachment 548363 [details] [diff] [review]
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) 2011-07-26 01:10:20 PDT
Created attachment 548391 [details] [diff] [review]
Add the pre-interned attribute to the parser
Comment 4 Henri Sivonen (:hsivonen) 2011-07-26 01:24:28 PDT
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 [:bz] (Out June 25-July 6) 2011-07-26 05:50:43 PDT
> 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 [:bz] (Out June 25-July 6) 2011-07-26 09:53:16 PDT
Comment on attachment 548391 [details] [diff] [review]
Add the pre-interned attribute to the parser

r=me
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-26 10:15:00 PDT
Created attachment 548513 [details] [diff] [review]
Updated to comments and with Henri's attribute addition merged in
Comment 8 Henri Sivonen (:hsivonen) 2011-07-27 05:48:29 PDT
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) 2011-07-28 13:51:30 PDT
(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) 2011-07-28 13:55:14 PDT
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 [:bz] (Out June 25-July 6) 2011-07-28 13:59:52 PDT
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 [:bz] (Out June 25-July 6) 2011-07-29 11:32:23 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f5aecf9010ef
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 12:38:23 PDT
And backed out because of build bustage; I somehow managed to push an old version of the patch....
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 18:37:23 PDT
Now it's fine: http://hg.mozilla.org/integration/mozilla-inbound/rev/185e9d01a1ec
Comment 15 Marco Bonardo [::mak] 2011-08-01 07:51:49 PDT
http://hg.mozilla.org/mozilla-central/rev/185e9d01a1ec

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