Last Comment Bug 671906 - Cache logic for CORS in imagelib seems to be broken
: Cache logic for CORS in imagelib seems to be broken
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Joe Drew (not getting mail)
:
Mentors:
Depends on: 702900
Blocks: 444641 664299
  Show dependency treegraph
 
Reported: 2011-07-15 10:28 PDT by Boris Zbarsky [:bz]
Modified: 2011-11-16 00:33 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
fix (712 bytes, patch)
2011-07-15 13:45 PDT, Joe Drew (not getting mail)
bzbarsky: review+
christian: approval‑mozilla‑aurora-
Details | Diff | Review
test (still fails) (4.89 KB, patch)
2011-07-15 13:46 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Review
part 0: correct img crossorigin case (771 bytes, patch)
2011-07-15 15:09 PDT, Joe Drew (not getting mail)
bzbarsky: review+
christian: approval‑mozilla‑aurora-
Details | Diff | Review
test v2 (5.74 KB, patch)
2011-07-15 15:13 PDT, Joe Drew (not getting mail)
no flags Details | Diff | Review
test v3 (5.51 KB, patch)
2011-07-15 17:54 PDT, Joe Drew (not getting mail)
bzbarsky: review+
christian: approval‑mozilla‑aurora-
Details | Diff | Review

Description Boris Zbarsky [:bz] 2011-07-15 10:28:11 PDT
I tried building on the patch for bug 664299 and my code didn't work at first, until I noticed that ValidateCORS returns true if the principals are NOT equal.  That's backwards.  And we apparently don't have any tests for that codepath...

We need to fix this before shipping Fx8.  I have a fix, obviously; it's just removing the '!'.  But I'm not sure how to best write a test here.
Comment 1 Joe Drew (not getting mail) 2011-07-15 13:44:20 PDT
I have a patch and a test I'm going to upload, but unfortunately it still fails because the HTML5 parser preloads images without taking into account their CORS status. The only reason this works in the CORS tests we added in bug 664299 is because we manually create images in JS.

I guess nsDocument::MaybePreLoadImage needs to take into account whether crossorigin='' was specified on the img tag, but I have no idea how to query that information.
Comment 2 Joe Drew (not getting mail) 2011-07-15 13:45:21 PDT
Created attachment 546219 [details] [diff] [review]
fix
Comment 3 Joe Drew (not getting mail) 2011-07-15 13:46:06 PDT
Created attachment 546220 [details] [diff] [review]
test (still fails)
Comment 4 Joe Drew (not getting mail) 2011-07-15 14:26:21 PDT
Please ignore comment 1. The HTML5 parser's preloading seems blameless here. However, we aren't getting nsGkAtoms::crossOrigin given to us by the parser in ParseAttribute.
Comment 5 Joe Drew (not getting mail) 2011-07-15 15:09:59 PDT
Created attachment 546236 [details] [diff] [review]
part 0: correct img crossorigin case

We can't have capitalization in the nsGkAtoms list, because then we don't match anything ever when we do NS_NewAtom() from a lowercased HTML attribute.
Comment 6 Joe Drew (not getting mail) 2011-07-15 15:13:46 PDT
Created attachment 546238 [details] [diff] [review]
test v2

We have to have Access-Control-Allow-Origin: * in our image's HTTP headers, or else nsCORSListenerProxy cancels the load. Also, I added a third iframe load, just to be extra-sure.

If you reload this test, though, we fail it; I think that's due to iframes caching their src.
Comment 7 Boris Zbarsky [:bz] 2011-07-15 16:22:11 PDT
> However, we aren't getting nsGkAtoms::crossOrigin given to us by the parser in
> ParseAttribute.

Is that because you're starting the load before all the attributes have been set?

It seems like when the crossorigin attribute changes we should discard any existing image load and start a new one anyway, no?
Comment 8 Boris Zbarsky [:bz] 2011-07-15 16:26:59 PDT
Comment on attachment 546219 [details] [diff] [review]
fix

r=me
Comment 9 Boris Zbarsky [:bz] 2011-07-15 16:28:05 PDT
Comment on attachment 546236 [details] [diff] [review]
part 0: correct img crossorigin case

Please change the name to be all lowercase too, and r=me
Comment 10 Boris Zbarsky [:bz] 2011-07-15 16:30:27 PDT
Comment on attachment 546238 [details] [diff] [review]
test v2

Why do you need the binarystream bit?
Comment 11 Boris Zbarsky [:bz] 2011-07-15 16:31:56 PDT
And we should file a followup bug to either skip preloading crossorigin images or preload them with the right CORS mode, because what we're doing now is clearly wrong.
Comment 12 Joe Drew (not getting mail) 2011-07-15 17:12:18 PDT
(In reply to comment #10)
> Comment on attachment 546238 [details] [diff] [review] [review]
> test v2
> 
> Why do you need the binarystream bit?

Just copy and paste silliness. I'm removing it.
Comment 13 Joe Drew (not getting mail) 2011-07-15 17:54:40 PDT
Created attachment 546264 [details] [diff] [review]
test v3
Comment 14 Joe Drew (not getting mail) 2011-07-15 19:38:24 PDT
(In reply to comment #11)
> And we should file a followup bug to either skip preloading crossorigin
> images or preload them with the right CORS mode, because what we're doing
> now is clearly wrong.

This is bug 672014.
Comment 15 Boris Zbarsky [:bz] 2011-07-15 21:06:34 PDT
Comment on attachment 546264 [details] [diff] [review]
test v3

r=me
Comment 18 christian 2011-07-26 15:08:26 PDT
Denying this for mozilla-aurora as we denied bug 664299

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