1323825 - Replace Has+Get with Get where required by spec in jsarray.cpp

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Test case:
---
Array.prototype.join.call(new Proxy([1, 2], new Proxy({}, {
  get(t, pk){
    print(pk);
  }
})));
---

Expected: Doesn't call "has" trap
Actual: Calls "has" trap twice

Comment 1

[André Bargull [:anba]]

Attachment: bug1323825-part1-update-steps.patch

Updates the step numbers to match the current ES2017 draft, so it's easier to verify the implementation follows the spec.

Comment 2

[André Bargull [:anba]]

Attachment: bug1323825-part2-remove-has.patch

Adds a new GetElement method without hole-checking and uses it in Array.prototype.{join, pop, shift}. 

Also inlines DoGetElement into its sole caller.

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8834831 [details] [diff] [review]
bug1323825-part1-update-steps.patch

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

Great, thanks for updating those.

Comment 4

[Tom S [:evilpie]]

Comment on attachment 8834834 [details] [diff] [review]
bug1323825-part2-remove-has.patch

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

Great tests as usual. I thought about this a bit and I couldn't come up with any other way to make this observable without using proxies. And maybe a bit faster for Arrays with holes \o/

::: js/src/jsarray.cpp
@@ +213,3 @@
>   */
> +static bool
> +GetElement(JSContext* cx, HandleObject obj, HandleObject receiver, uint32_t index, bool* hole,

Great, we already have to many GetElement variants. I feel like we should have a followup for renaming this to HasAndGetElement.

@@ +253,5 @@
>      return GetElement(cx, obj, obj, index, hole, vp);
>  }
>  
> +/*
> + * Gets the property at the given index into the location pointed by vp.

This is a weird comment, I think we can just remove it.

::: js/src/tests/ecma_6/Array/pop-no-has-trap.js
@@ +1,1 @@
> +// Test that Array.prototype.join doesn't call the [[HasProperty]] internal method of objects

.pop

::: js/src/tests/ecma_6/Array/shift-no-has-trap.js
@@ +1,1 @@
> +// Test that Array.prototype.join doesn't call the [[HasProperty]] internal method of objects

.shift

Comment 5

[André Bargull [:anba]]

Attachment: bug1323825-part2-remove-has.patch

Update part 2 per review comments, carrying r+ from evilpie.

Comment 6

[André Bargull [:anba]]

(In reply to Tom Schuster [:evilpie] from comment #4)
> I thought about this a bit and I couldn't come up with any other way to make this observable without using proxies.
> 

Yes, I don't think it's possible to observe this without proxies.


> ::: js/src/jsarray.cpp
> @@ +213,3 @@
> >   */
> > +static bool
> > +GetElement(JSContext* cx, HandleObject obj, HandleObject receiver, uint32_t index, bool* hole,
> 
> Great, we already have to many GetElement variants. I feel like we should
> have a followup for renaming this to HasAndGetElement.

Filed bug 1338126.

Comment 7

[André Bargull [:anba]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=700d0dfbb55be1293c08d3f2d9a0c342d7f0525f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d20a1d6504e85a7fab5d64f92c180a252bba1ff

The failures on OS X 10.7 opt, SM(p) seem to be intermittent.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64fc958a94f4
Part 1: Synchronize step number comments with latest ES2017 draft. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/d23e3f6246fb
Part 2: Remove spec incompliant [[HasProperty]] calls in Array.prototype.{join,pop,shift}. r=evilpie

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/64fc958a94f4
https://hg.mozilla.org/mozilla-central/rev/d23e3f6246fb