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

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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
(Assignee)

Comment 1

a year ago
Created attachment 8834831 [details] [diff] [review]
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.
Attachment #8834831 - Flags: review?(evilpies)
(Assignee)

Comment 2

a year ago
Created attachment 8834834 [details] [diff] [review]
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.
Attachment #8834834 - Flags: review?(evilpies)
Comment on attachment 8834831 [details] [diff] [review]
bug1323825-part1-update-steps.patch

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

Great, thanks for updating those.
Attachment #8834831 - Flags: review?(evilpies) → review+
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
Attachment #8834834 - Flags: review?(evilpies) → review+
(Assignee)

Comment 5

a year ago
Created attachment 8835478 [details] [diff] [review]
bug1323825-part2-remove-has.patch

Update part 2 per review comments, carrying r+ from evilpie.
Attachment #8834834 - Attachment is obsolete: true
Attachment #8835478 - Flags: review+
(Assignee)

Comment 6

a year ago
(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 8

a year ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/64fc958a94f4
https://hg.mozilla.org/mozilla-central/rev/d23e3f6246fb
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Updated

6 months ago
Duplicate of this bug: 922301
You need to log in before you can comment on or make changes to this bug.