Harmony proxies: for-in loop on proxy does not suppress properties deleted during enumeration

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Tom Van Cutsem, Assigned: gal)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
In tracemonkey, for-in loops normally suppress properties deleted during enumeration:

var obj = {a:0,b:1,c:2};
for (var p in obj) { delete obj['c']; print(p) };

prints: a,b

For proxies, this is not the case:

var target = {a:0,b:1,c:2};
var proxy = Proxy.create({
  has: function(name) {
    return name in target;
  },
  delete: function(name) {
    return delete target[name];
  },
  enumerate: function() {
    return ['a','b','c'];
  }
});

for (var prop in proxy) {
  delete proxy['c']; // delete a yet-to-be-enumerated property
  print(prop);
}

This prints a,b,c instead of the expected a,b.
It turns out enumeration on proxies does not invoke the proxy's has() trap to determine whether the property is still present on the proxy when it is enumerated.

While the semantics of proxy enumeration is still largely left unspecified in the spec, I think there was agreement that existing properties deleted during an enumeration should no longer be enumerated.
(Reporter)

Updated

7 years ago
Priority: -- → P2
(Assignee)

Comment 1

7 years ago
Created attachment 479596 [details] [diff] [review]
patch
Assignee: general → gal
(Assignee)

Updated

7 years ago
Attachment #479596 - Flags: review?(jorendorff)
Comment on attachment 479596 [details] [diff] [review]
patch

>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp
>--- a/js/src/jsproxy.cpp
>+++ b/js/src/jsproxy.cpp
>@@ -890,17 +890,18 @@ proxy_SetAttributes(JSContext *cx, JSObj
>     return JSProxy::defineProperty(cx, obj, id, &desc);
> }
> 
> static JSBool
> proxy_DeleteProperty(JSContext *cx, JSObject *obj, jsid id, Value *rval, JSBool strict)
> {
>     // TODO: throwing away strict
>     bool deleted;
>-    if (!JSProxy::delete_(cx, obj, id, &deleted))
>+    if (!JSProxy::delete_(cx, obj, id, &deleted) ||
>+        !js_SuppressDeletedProperty(cx, obj, id))
>         return false;

Braces for multiline any-part of if and if/else, but:

>+    if (!JSProxy::delete_(cx, obj, id, &deleted) || !js_SuppressDeletedProperty(cx, obj, id))
..1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890

this fits well within the 100-column style guide limit, so you can pull the condition up to fit in one line and avoid bracing.

r=me with that, another steal to relieve jorendorff.

/be
Attachment #479596 - Flags: review?(jorendorff) → review+

Comment 3

6 years ago
Was this patch forgotten? Does it still need pushing?
(Assignee)

Comment 4

6 years ago
Looks like it. Want to grab it jdm or should I land it?

Comment 5

6 years ago
Please feel free. I'm supposed to be packing to go travelling for six weeks.
(Assignee)

Comment 6

6 years ago
http://hg.mozilla.org/tracemonkey/rev/556ec2a89192
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/556ec2a89192
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.