671906 - Cache logic for CORS in imagelib seems to be broken

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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)]

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)]

Attachment: fix

Comment 3

[Joe Drew (not getting mail)]

Attachment: test (still fails)

Comment 4

[Joe Drew (not getting mail)]

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)]

Attachment: 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)]

Attachment: 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 [:bzbarsky]]

> 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 [:bzbarsky]]

Comment on attachment 546219 [details] [diff] [review]
fix

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Comment on attachment 546238 [details] [diff] [review]
test v2

Why do you need the binarystream bit?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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)]

(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)]

Attachment: test v3

Comment 14

[Joe Drew (not getting mail)]

(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 [:bzbarsky]]

Comment on attachment 546264 [details] [diff] [review]
test v3

r=me

Comment 16

[Joe Drew (not getting mail)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b8025eb1ac19
http://hg.mozilla.org/integration/mozilla-inbound/rev/090b23c76bd2
http://hg.mozilla.org/integration/mozilla-inbound/rev/c3cc5a259c04

Comment 17

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/b8025eb1ac19
http://hg.mozilla.org/mozilla-central/rev/090b23c76bd2
http://hg.mozilla.org/mozilla-central/rev/c3cc5a259c04

Comment 18

[LegNeato]

Denying this for mozilla-aurora as we denied bug 664299