Closed Bug 1254293 Opened 4 years ago Closed 4 years ago

Fix GetArrayIndexFromId to do the right thing for indices that don't fit in an int32_t

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Right now we handle them incorrect.y
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?)
Attachment #8727590 - Flags: review?(peterv) → review+
> 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.
https://hg.mozilla.org/mozilla-central/rev/3ec41d5331ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.