Cache logic for CORS in imagelib seems to be broken

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bz, Assigned: Joe Drew (not getting mail))

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox8-)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Blocks: 444641
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 546219 [details] [diff] [review]
fix
Assignee: nobody → joe
Attachment #546219 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

6 years ago
Created attachment 546220 [details] [diff] [review]
test (still fails)
Attachment #546220 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
Attachment #546236 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

6 years ago
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.
Attachment #546220 - Attachment is obsolete: true
Attachment #546238 - Flags: review?(bzbarsky)
Attachment #546220 - Flags: review?(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 on attachment 546219 [details] [diff] [review]
fix

r=me
Attachment #546219 - Flags: review?(bzbarsky) → review+
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
Attachment #546236 - Flags: review?(bzbarsky) → review+
Comment on attachment 546238 [details] [diff] [review]
test v2

Why do you need the binarystream bit?
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.
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
Created attachment 546264 [details] [diff] [review]
test v3
Attachment #546238 - Attachment is obsolete: true
Attachment #546264 - Flags: review?(bzbarsky)
Attachment #546238 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

6 years ago
(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 on attachment 546264 [details] [diff] [review]
test v3

r=me
Attachment #546264 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

6 years ago
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
Whiteboard: [inbound]
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/mozilla-central/rev/b8025eb1ac19
http://hg.mozilla.org/mozilla-central/rev/090b23c76bd2
http://hg.mozilla.org/mozilla-central/rev/c3cc5a259c04
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
Attachment #546236 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Attachment #546264 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

6 years ago
Attachment #546219 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #546219 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #546236 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

6 years ago
Attachment #546264 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Comment 18

6 years ago
Denying this for mozilla-aurora as we denied bug 664299

Updated

6 years ago
tracking-firefox8: ? → -

Updated

6 years ago
Depends on: 702900
You need to log in before you can comment on or make changes to this bug.