Last Comment Bug 676413 - crossOrigin attribute invalid-value-default should be Anonymous
: crossOrigin attribute invalid-value-default should be Anonymous
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on: 678077 679689 702578 715124
Blocks: 643651 664299
  Show dependency treegraph
 
Reported: 2011-08-03 15:58 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-01-06 08:55 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This should do the trick (9.13 KB, patch)
2011-08-08 12:01 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
Details | Diff | Splinter Review
test that crossOrigin="" or invalid value has the behavior of "anonymous" (4.28 KB, patch)
2011-08-08 12:40 PDT, Benoit Jacob [:bjacob] (mostly away)
bzbarsky: review+
Details | Diff | Splinter Review
test crossOrigin without value (4.24 KB, patch)
2011-08-08 15:21 PDT, Benoit Jacob [:bjacob] (mostly away)
bzbarsky: review+
Details | Diff | Splinter Review
fix the crossOrigin attribute test (1.07 KB, patch)
2011-08-11 14:35 PDT, Benoit Jacob [:bjacob] (mostly away)
bugs: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2011-08-03 15:58:58 PDT
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.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-08-03 19:13:11 PDT
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?
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-04 13:13:39 PDT
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.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-08-04 18:53:08 PDT
Yes, we do that anyway.
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-08-08 10:56:55 PDT
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.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-08-08 11:03:57 PDT
If you need this in Fx8, I'm the wrong person, since I'm going on vacation from tomorrow until after Fx8 branches.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-08-08 11:05:22 PDT
Or more precisely I _could_ try to write up a patch this afternoon, maybe, but someone else would obviously need to drive it in....
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2011-08-08 11:08:20 PDT
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?
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-08 11:23:27 PDT
Sorry, I won't have time to do this.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-08-08 11:34:35 PDT
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.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2011-08-08 11:53:42 PDT
I can write the tests :-)
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-08-08 12:01:05 PDT
Created attachment 551524 [details] [diff] [review]
This should do the trick
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-08 12:17:58 PDT
Comment on attachment 551524 [details] [diff] [review]
This should do the trick

r=me assuming tests pass
Comment 14 Benoit Jacob [:bjacob] (mostly away) 2011-08-08 12:40:08 PDT
Created attachment 551537 [details] [diff] [review]
test that crossOrigin="" or invalid value has the behavior of "anonymous"

This also renames "leave-default-crossOrigin-attribute" to "missing-value-default" because that's really what it meant.
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-08-08 12:41:21 PDT
(With Boris's patch and that test patch, it passes. With only Boris's patch and without the test patch, it fails)
Comment 16 Benoit Jacob [:bjacob] (mostly away) 2011-08-08 12:49:52 PDT
Comment on attachment 551537 [details] [diff] [review]
test that crossOrigin="" or invalid value has the behavior of "anonymous"

Anyone can review this?
Comment 17 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-08 14:15:40 PDT
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.
Comment 18 Olli Pettay [:smaug] 2011-08-08 14:19:31 PDT
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 19 Benoit Jacob [:bjacob] (mostly away) 2011-08-08 15:20:45 PDT
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.
Comment 20 Benoit Jacob [:bjacob] (mostly away) 2011-08-08 15:21:52 PDT
Created attachment 551588 [details] [diff] [review]
test crossOrigin without value

and this one too, which he asked for. testing that <img crossOrigin> really behaves like <img crossOrigin="">.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2011-08-08 22:20:32 PDT
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.
Comment 22 Benoit Jacob [:bjacob] (mostly away) 2011-08-09 12:31:16 PDT
(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?
Comment 23 Olli Pettay [:smaug] 2011-08-09 12:42:09 PDT
See for example comment #c10
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-08-10 10:19:56 PDT
> 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.
Comment 25 Benoit Jacob [:bjacob] (mostly away) 2011-08-10 15:34:13 PDT
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
Comment 26 David Baron :dbaron: ⌚️UTC-7 2011-08-10 18:02:04 PDT
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 ""
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2011-08-10 20:22:51 PDT
Yeah, that test is just testing things that don't match the spec...
Comment 28 Benoit Jacob [:bjacob] (mostly away) 2011-08-10 21:16:34 PDT
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?
Comment 29 Benoit Jacob [:bjacob] (mostly away) 2011-08-10 21:17:53 PDT
(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?
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2011-08-10 22:00:58 PDT
> 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.
Comment 31 Benoit Jacob [:bjacob] (mostly away) 2011-08-11 10:02:55 PDT
OK thanks! I didn't realize the difference between enumerated and limited enumerated.
Comment 32 Benoit Jacob [:bjacob] (mostly away) 2011-08-11 14:35:50 PDT
Created attachment 552507 [details] [diff] [review]
fix the crossOrigin attribute test
Comment 34 Jean-Yves Perrier [:teoli] 2011-10-18 05:02:48 PDT
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)

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