Closed Bug 676413 Opened 9 years ago Closed 9 years ago

crossOrigin attribute invalid-value-default should be Anonymous

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bjacob, Unassigned)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files)

The crossOrigin attribute is described here:

http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#cors-settings-attribute

It says: "The empty string is also a valid keyword, and maps to the Anonymous state. The attribute's invalid value default is the Anonymous state. The missing value default, used when the attribute is omitted, is the No CORS state."

The behavior currently implemented in bug 664299 is that the invalid-value-default is No CORS. So we need to change that to Anonymous, but without changing the missing-value-default.

Can you help? It's not straightforward, because our facilities for implementing an enum attribute, such as NS_IMPL_ENUM_ATTR_DEFAULT_VALUE, don't seem to distinguish between an invalid value and a missing value.

Also, this really matters in practice, see bug 643651.
NS_IMPL_ENUM_ATTR_DEFAULT_VALUE will return the value stored in the nsAttrValue if that's an enumerated value.  If there is no nsAttrValue or the nsAttrValue isn't enumerated, it will return the last argument to the macro.

So in this case, it seems like we should just make the last argument to the macro "no cors" and make sure that as long as the attribute is set the nsAttrValue is enumerated and has the right enum value.  That means that in ParseAttribute we need to ParseEnumValue and if that returns false go ahead and set the nsAttrValue to the "anonymous CORS" value.

Unfortunately, it looks like we have no public API on nsAttrValue for setting it to an enumerated value.  Jonas, can we just change ParseEnumValue to take an optional default value if the value doesn't match anything in the enumeration?
Sure, I'd be fine with that. Though we have to make sure that the string value that we store is whatever value was actually passed in.
Yes, we do that anyway.
So is either of you taking this bug?

We really would like to have this in Firefox 8. A big major WebGL site coming out mid-August is relying on anonymous CORS and apparently is doing crossOrigin="" i.e. relying on the invalid-value-default.
If you need this in Fx8, I'm the wrong person, since I'm going on vacation from tomorrow until after Fx8 branches.
Or more precisely I _could_ try to write up a patch this afternoon, maybe, but someone else would obviously need to drive it in....
OK no problem.

Jonas: can you do the part about the nsAttrValue API and give me directions for the rest? Or can you do everything?
Sorry, I won't have time to do this.
I'm also not likely to have time to write tests and whatnot, even if I can get the code done.

I should have read the spec way better in bug 664299, by the way.  For example, crossorigin="aNONymous" should give img.crossOrigin == "aNONymous" whereas we canonicalize it.  That should be in tests too.
I can write the tests :-)
Comment on attachment 551524 [details] [diff] [review]
This should do the trick

r=me assuming tests pass
Attachment #551524 - Flags: review+
This also renames "leave-default-crossOrigin-attribute" to "missing-value-default" because that's really what it meant.
(With Boris's patch and that test patch, it passes. With only Boris's patch and without the test patch, it fails)
Comment on attachment 551537 [details] [diff] [review]
test that crossOrigin="" or invalid value has the behavior of "anonymous"

Anyone can review this?
Attachment #551537 - Flags: review?(jonas)
Attachment #551537 - Flags: review?(Olli.Pettay)
Comment on attachment 551537 [details] [diff] [review]
test that crossOrigin="" or invalid value has the behavior of "anonymous"

Hmm.. these tests are complicated enough that I think someone that know them better than me should review them.
Attachment #551537 - Flags: review?(jonas)
I'll review the tests tomorrow, but I'll have to review bug 664299 before that.
...unless Boris has time to look at the test changes.
Comment on attachment 551537 [details] [diff] [review]
test that crossOrigin="" or invalid value has the behavior of "anonymous"

Jeff said he'd review this because he's a sucker.
Attachment #551537 - Flags: review?(Olli.Pettay) → review?(jmuizelaar)
and this one too, which he asked for. testing that <img crossOrigin> really behaves like <img crossOrigin="">.
Attachment #551588 - Flags: review?(jmuizelaar)
Comment on attachment 551537 [details] [diff] [review]
test that crossOrigin="" or invalid value has the behavior of "anonymous"

This looks fine as far as it goes, so r=me on these changes, but we should also add a test for the reflection stuff in comment 10.  I seem to recall we have a DOM attribute reflection test framework in content/html/content/test/reflect.js; might be worth looking at other tests that use it.
Attachment #551537 - Flags: review+
Attachment #551537 - Flags: review?(jmuizelaar)
Attachment #551588 - Flags: review?(jmuizelaar)
(In reply to Boris Zbarsky (:bz) from comment #21)
> Comment on attachment 551537 [details] [diff] [review] [diff] [details] [review]
> test that crossOrigin="" or invalid value has the behavior of "anonymous"
> 
> This looks fine as far as it goes, so r=me on these changes, but we should
> also add a test for the reflection stuff in comment 10.

I don't even know what is the specified behavior that we would want to test here. Smaug, Jonas?
See for example comment #c10
> I don't even know what is the specified behavior

Getting .crossOrigin returns exactly the string that set using setAttribute.

Setting .crossOrigin causes getAttribute to return exactly the string that was set.

In particular, you want to test leading/trailing whitespace and case variations.
Ah OK thanks for the detailed explanation. Filed bug 678077 about adding that test.

Landed on central:
http://hg.mozilla.org/mozilla-central/rev/89a9f4a88d5b
http://hg.mozilla.org/mozilla-central/rev/289becc07558
http://hg.mozilla.org/mozilla-central/rev/aac29f0bdd10
Status: NEW → RESOLVED
Closed: 9 years ago
Depends on: 678077
Resolution: --- → FIXED
Backed out:

http://hg.mozilla.org/mozilla-central/rev/a1b3ba6eabf8
http://hg.mozilla.org/mozilla-central/rev/686e5ad8fa96
http://hg.mozilla.org/mozilla-central/rev/1221d45e7aca

for:


65289 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | Enumerated attributes should be case-insensitive. - got "ANONYMOUS", expected "anonymous"
65293 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | Enumerated attributes should be case-insensitive. - got "ANONYMOUS", expected "anonymous"
65297 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | Enumerated attributes should be case-insensitive. - got "USE-CREDENTIALS", expected "use-credentials"
65301 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | Enumerated attributes should be case-insensitive. - got "USE-CREDENTIALS", expected "use-credentials"
65303 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | When the content attribute is set to an invalid value, the default value should be returned. - got "foobar", expected ""
65305 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug664299.html | When the value is set to an invalid value, the default value should be returned. - got "foobar", expected ""
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yeah, that test is just testing things that don't match the spec...
Good news/bad news time.

Good news: the test that Boris asked for in comment 10 already exists, Joe wrote it in bug 664299. It uses the reflect.js framework as mentioned in comment 21. 

Bad news: That's the test that's failing in comment 26. As far as I understand, the failures fall into 2 categories:

> 65289 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug664299.html | Enumerated attributes
> should be case-insensitive. - got "ANONYMOUS", expected "anonymous"
> 65293 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug664299.html | Enumerated attributes
> should be case-insensitive. - got "ANONYMOUS", expected "anonymous"
> 65297 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug664299.html | Enumerated attributes
> should be case-insensitive. - got "USE-CREDENTIALS", expected
> "use-credentials"
> 65301 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug664299.html | Enumerated attributes
> should be case-insensitive. - got "USE-CREDENTIALS", expected
> "use-credentials"

This seems to be a real regression, right? But looking at Boris' patch I don't understand how it can cause this regression. Anyone?

> 65303 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug664299.html | When the content
> attribute is set to an invalid value, the default value should be returned.
> - got "foobar", expected ""
> 65305 ERROR TEST-UNEXPECTED-FAIL |
> /tests/content/html/content/test/test_bug664299.html | When the value is set
> to an invalid value, the default value should be returned. - got "foobar",
> expected ""

Here I'm afraid that it's the reflect.js framework that is not prepared to distinguish between invalid-value-default and missing-value-default, right?
(In reply to Boris Zbarsky (:bz) from comment #27)
> Yeah, that test is just testing things that don't match the spec...

The 2 last out of these 6 failures indeed don't seem to match the spec. But the first 4? Aren't they testing exactly what you asked for in comment 10/21/24?
> This seems to be a real regression, right?

It's a real behavior change, and expected.  The test needs to be updated.

> Here I'm afraid that it's the reflect.js framework that is not prepared

The reflect.js framework is being called via reflectLimitedEnumerated here.  So it's making various assertions about case treatment and whatnot that are only true for limited enumerated attributes.

But this attribute is just enumerated, not limited enumerated.

So this test should be using reflectString and a larger set of values to test that includes case variations and so forth.
OK thanks! I didn't realize the difference between enumerated and limited enumerated.
Attachment #552507 - Flags: review+
Depends on: 679689
This is documented there: https://developer.mozilla.org/en/HTML/Element/img#attr-crossorigin
"If invalid, it is handled as if the enumerated keyword anonymous was used."
and there: https://developer.mozilla.org/en/HTML/CORS_settings_attributes
"An invalid keyword will be handled as the anonymous keyword."

I don't think we need to add something in "Fx8 for developers".

->dev-doc-complete (reset to dev-doc-needed if you disagree)
Depends on: 702578
Depends on: 715124
You need to log in before you can comment on or make changes to this bug.