Boris Zbarsky [:bzbarsky] Assignee
	Description • 8 years ago

Right now we handle them incorrect.y

	Peter Van der Beken [:peterv]
	Comment 2 • 8 years ago

Comment on attachment 8727590 [details] [diff] [review]
Fix dom::GetArrayIndexFromId to actually follow the spec for large indices (i.e. ones that don't fit in in int32_t)

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

::: dom/bindings/DOMJSProxyHandler.h
@@ +160,5 @@
>  GetArrayIndexFromId(JSContext* cx, JS::Handle<jsid> id)
>  {
> +  // Much like js::IdIsIndex, except with a fast path for "length" and another
> +  // fast path for starting with a lowercase ascii char.  Is that second one
> +  // really needed?

IIRC the overhead of calling StringIsArrayIndex was showing up in profiles, but we should probably remeasure at some point.

@@ +171,5 @@
> +  if (MOZ_UNLIKELY(!JSID_IS_ATOM(id))) {
> +    return UINT32_MAX;
> +  }
> +
> +  JSAtom* atom = JSID_TO_ATOM(id);

You could move |JSLinearString* str = js::AtomToLinearString(JSID_TO_ATOM(id));| here and drop atom (GetLatin1AtomChars and GetTwoByteAtomChars call AtomToLinearString under the hood)...

@@ +178,5 @@
> +    JS::AutoCheckCannotGC nogc;
> +    if (js::AtomHasLatin1Chars(atom)) {
> +      s = *js::GetLatin1AtomChars(nogc, atom);
> +    } else {
> +      s = *js::GetTwoByteAtomChars(nogc, atom);

... and switch this to LinearStringHasLatin1Chars/GetLatin1LinearStringChars/GetTwoByteLinearStringChars.

::: testing/web-platform/tests/dom/collections/HTMLCollection-supported-property-indices.html
@@ +92,5 @@
> +  assert_equals(collection.namedItem(4294967297),
> +                document.getElementById("4294967297"));
> +  assert_equals(collection[4294967293], undefined);
> +  assert_equals(collection[4294967294], undefined);
> +  assert_equals(collection[4294967295], document.getElementById("4294967295"));

The difference between item and [] is unfortunate. (I guess it's because of how ints are stored in jsids?)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 8 years ago

> IIRC the overhead of calling StringIsArrayIndex was showing up in profiles

Updated the comment to point out that StringIsArrayIndex is out of line, so might be worth not calling.

> You could move |JSLinearString* str = js::AtomToLinearString(JSID_TO_ATOM(id));| here

Yep, done.

> I guess it's because of how ints are stored in jsids?

It's because valid array indices in JS and Web IDL are in the range [0, 2^32-2], because 2^32-1 is the max possible value of length.

On the other hand, item() can take any uint32_t, including 2^32-1.

	Pulsebot
	Comment 4 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ec41d5331ac

	Carsten Book [:Tomcat]
	Comment 5 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3ec41d5331ac