Update document.all/HTMLAllCollection legacy caller

RESOLVED FIXED in Firefox 64



a year ago
2 days ago


(Reporter: d, Assigned: bzbarsky)


(Blocks: 1 bug, {dev-doc-needed})


Firefox Tracking Flags

(firefox57 affected, firefox64 fixed)



(3 attachments)



a year 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.
Blocks: 1498357

Comment 4

29 days ago
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

29 days ago
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)


29 days ago
Flags: needinfo?(jwalden+bmo)

Comment 6

29 days ago
Created attachment 9017820 [details] [diff] [review]
part 1.  Expose StringIsArrayIndex taking a char pointer in jsfriendapi
Attachment #9017820 - Flags: review?(jwalden+bmo)


29 days ago
Assignee: nobody → bzbarsky

Comment 7

29 days ago
Created attachment 9017821 [details] [diff] [review]
part 2.  Update document.all item() and legacycaller to new spec semantics
Attachment #9017821 - Flags: review?(continuation)

Comment 8

28 days ago
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

28 days 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

27 days 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

26 days ago
Last Resolved: 26 days ago
status-firefox64: --- → fixed
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.
You need to log in before you can comment on or make changes to this bug.