1398354 - Update document.all/HTMLAllCollection legacy caller

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Domenic Denicola]

The spec changed a while back to some reasonable intersection semantics, I believe in https://github.com/whatwg/html/commit/93cc395912de3a9824fdf35e8d9395f7ddb7c003 .

While making an editorial change recently ( https://github.com/whatwg/html/commit/0679448c59cb4cd69019be1e1f35fee1bcbc9c66 ), we added tests for that, which Gecko fails some of:

- https://github.com/w3c/web-platform-tests/pull/7254
- http://w3c-test.org/html/infrastructure/common-dom-interfaces/collections/htmlallcollection.html

The spec should be implementable by just updating the IDL and continuing to use legacycaller, even though the spec no longer does. https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#htmlallcollection

Comment 1

[Andrew Overholt [:overholt]]

Thanks (as always) for a great bug report. We'll try to get to this some time soon.

Comment 2

[Chris Mills [:cmills]]

Adding ddn, just in case anything needs updating on our end.

Comment 3

[Andrew McCreight [:mccr8]]

There are a few legacy caller tests that Firefox fails, but no other browser fails:
  legacy caller with "array index property name"
  legacy caller with "array index property name" as number
  legacy caller with no argument

I'm not sure what the first two are, but at least the final one seems to involve behavior that was mentioned in the pull request in comment 0.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I believe to match the spec we need to do a few things:

1) Implement the new spec semantics for HTMLAllCollection.item.  
   Those are pretty annoying, of course; they involve converting numbers
   to strings and then back.  :(
2) Make the legacycaller just be item().

We could fix that third test mentioned in comment 3 by making the legacycaller arg optional now, I guess.  But   we might as well just do the whole silly thing.  :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

Do we have JSAPI for testing https://tc39.github.io/ecma262/#array-index with (char16_t,length) pair or some equivalent?  Or even with a JS::Value?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Expose StringIsArrayIndex taking a char pointer in jsfriendapi

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Update document.all item() and legacycaller to new spec semantics

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 9017820 [details] [diff] [review]
part 1.  Expose StringIsArrayIndex taking a char pointer in jsfriendapi

Review of attachment 9017820 [details] [diff] [review]:
-----------------------------------------------------------------

Add a jsapi-test with obvious simple passing cases and failing cases in the obvious extreme and edge-case ways.

::: js/src/builtin/Array.cpp
@@ +265,5 @@
>  {
>      AutoCheckCannotGC nogc;
>      return str->hasLatin1Chars()
> +           ? ::StringIsArrayIndexHelper(str->latin1Chars(nogc), str->length(), indexp)
> +           : ::StringIsArrayIndexHelper(str->twoByteChars(nogc), str->length(), indexp);

Get rid of the :: qualifications now that they're unnecessary.

@@ +278,5 @@
> +
> +JS_FRIEND_API(bool)
> +js::StringIsArrayIndex(const char* str, uint32_t length, uint32_t* indexp)
> +{
> +    AutoCheckCannotGC nogc;

Don't need the nogc in these.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: jsapi tests

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e41c385edb9
part 1.  Expose StringIsArrayIndex taking a char pointer in jsfriendapi.  r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/344b7e258ea5
part 2.  Update document.all item() and legacycaller to new spec semantics.  r=mccr8

Comment 11

[Cristina Coroiu [:ccoroiu]]

Backed out 2 changesets (Bug 1398354) for spidermonkey and hazard bustages at js/src/jsapi-tests/testStringIsArrayIndex.cpp

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/a42a9fc0c5f4bc801873a3803ffefbe0761c9b59

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=344b7e258ea5d73fdd919a6ed6459189874c3533

Failure log: 

Spidermonkey: https://treeherder.mozilla.org/logviewer.html#?job_id=206277319&repo=mozilla-inbound&lineNumber=2683

Hazard: https://treeherder.mozilla.org/logviewer.html#?job_id=206277276&repo=mozilla-inbound&lineNumber=3121

Build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=206277248&repo=mozilla-inbound&lineNumber=33947

task 2018-10-18T03:17:16.650Z] --- check_spidermonkey_style.py expected output
[task 2018-10-18T03:17:16.650Z] +++ check_spidermonkey_style.py actual output
[task 2018-10-18T03:17:16.650Z] @@ -1,3 +1,6 @@
[task 2018-10-18T03:17:16.650Z] +js/src/jsapi-tests/testStringIsArrayIndex.cpp:10:11: error:
[task 2018-10-18T03:17:16.650Z] +    "jsapi-tests/tests.h" should be included after <stdio.h>
[task 2018-10-18T03:17:16.650Z] +
[task 2018-10-18T03:17:16.650Z]  js/src/tests/style/BadIncludes.h:3: error:
[task 2018-10-18T03:17:16.650Z]      the file includes itself
[task 2018-10-18T03:17:16.650Z]  
[task 2018-10-18T03:17:16.650Z] TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | actual output does not match expected output;  diff is above.
[task 2018-10-18T03:17:16.650Z] TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | Hint: If the problem is that you renamed a header, and many #includes are no longer in alphabetical order, commit your work and then try `check_spidermonkey_style.py --fixup`. You need to commit first because --fixup modifies your files in place.
[task 2018-10-18T03:17:16.654Z] Traceback (most recent call last):
[task 2018-10-18T03:17:16.654Z]   File "/usr/lib/python2.7/runpy.py", line 162, in _run_module_as_main
[task 2018-10-18T03:17:16.654Z]     "__main__", fname, loader, pkg_name)
[task 2018-10-18T03:17:16.654Z]   File "/usr/lib/python2.7/runpy.py", line 72, in _run_code
[task 2018-10-18T03:17:16.654Z]     exec code in run_globals
[task 2018-10-18T03:17:16.654Z]   File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/action/file_generate.py", line 110, in <module>
[task 2018-10-18T03:17:16.654Z]     sys.exit(main(sys.argv[1:]))
[task 2018-10-18T03:17:16.654Z]   File "/builds/worker/workspace/build/src/python/mozbuild/mozbuild/action/file_generate.py", line 70, in main
[task 2018-10-18T03:17:16.654Z]     ret = module.__dict__[method](output, *args.additional_arguments, **kwargs)
[task 2018-10-18T03:17:16.654Z]   File "/builds/worker/workspace/build/src/config/run_spidermonkey_checks.py", line 15, in main
[task 2018-10-18T03:17:16.654Z]     raise Exception(script + " failed")
[task 2018-10-18T03:17:16.654Z] Exception: /builds/worker/workspace/build/src/config/check_spidermonkey_style.py failed
[task 2018-10-18T03:17:16.660Z] backend.mk:20: recipe for target '.deps/spidermonkey_checks.stub' failed
[task 2018-10-18T03:17:16.660Z] make[3]: *** [.deps/spidermonkey_checks.stub] Error 1
[task 2018-10-18T03:17:16.660Z] make[3]: Leaving directory '/builds/worker/workspace/build/src/obj-spider/js/src/build'
[task 2018-10-18T03:17:16.660Z] /builds/worker/workspace/build/src/config/recurse.mk:101: recipe for target 'js/src/build/misc' failed
[task 2018-10-18T03:17:16.660Z] make[2]: *** [js/src/build/misc] Error 2
[task 2018-10-18T03:17:16.660Z] make[2]: Leaving directory '/builds/worker/workspace/build/src/obj-spider'
[task 2018-10-18T03:17:16.661Z] /builds/worker/workspace/build/src/config/recurse.mk:32: recipe for target 'misc' failed
[task 2018-10-18T03:17:16.661Z] make[1]: *** [misc] Error 2
[task 2018-10-18T03:17:16.661Z] make[1]: Leaving directory '/builds/worker/workspace/build/src/obj-spider'
[task 2018-10-18T03:17:16.661Z] /builds/worker/workspace/build/src/config/rules.mk:431: recipe for target 'default' failed
[task 2018-10-18T03:17:16.661Z] make: *** [default] Error 2

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7142afba712
part 1.  Expose StringIsArrayIndex taking a char pointer in jsfriendapi.  r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/59e0564b7435
part 2.  Update document.all item() and legacycaller to new spec semantics.  r=mccr8

Comment 13

[Boris Zbarsky [:bzbarsky]]

That's what I get for adding a test after the try run.

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b7142afba712
https://hg.mozilla.org/mozilla-central/rev/59e0564b7435

Comment 15

[Chris Mills [:cmills]]

Note to docs team:

I've added a note to the Fx 64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#APIs

I'm not convinced we really need to bother documenting this (we don't currently), but I could be convinced otherwise.

Comment 16

[Chris Mills [:cmills]]

I'm leaving this; let me know if you think we need documentation on this.