Update document.all/HTMLAllCollection legacy caller

RESOLVED FIXED in Firefox 64



2 years ago
a month ago


(Reporter: d, Assigned: bzbarsky)


(Blocks 1 bug, {dev-doc-complete})


Firefox Tracking Flags

(firefox64 fixed)



(3 attachments)



2 years ago
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
Thanks (as always) for a great bug report. We'll try to get to this some time soon.
Priority: -- → P2
Adding ddn, just in case anything needs updating on our end.
Keywords: dev-doc-needed
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.
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.  :(
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?
Flags: needinfo?(jwalden+bmo)


6 months ago
Flags: needinfo?(jwalden+bmo)


6 months ago
Assignee: nobody → bzbarsky
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.
Attachment #9017820 - Flags: review?(jwalden+bmo) → review+
Attachment #9017821 - Flags: review?(continuation) → review+

Comment 10

6 months ago
Pushed by bzbarsky@mozilla.com:
part 1.  Expose StringIsArrayIndex taking a char pointer in jsfriendapi.  r=waldo
part 2.  Update document.all item() and legacycaller to new spec semantics.  r=mccr8
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
Flags: needinfo?(bzbarsky)

Comment 12

6 months ago
Pushed by bzbarsky@mozilla.com:
part 1.  Expose StringIsArrayIndex taking a char pointer in jsfriendapi.  r=waldo
part 2.  Update document.all item() and legacycaller to new spec semantics.  r=mccr8
That's what I get for adding a test after the try run.
Flags: needinfo?(bzbarsky)

Comment 14

6 months ago
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Note to docs team:

I've added a note to the Fx 64 rel notes:

I'm not convinced we really need to bother documenting this (we don't currently), but I could be convinced otherwise.
I'm leaving this; let me know if you think we need documentation on this.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.