Last Comment Bug 600702 - Harmony proxies: for-in loop on proxy does not suppress properties deleted during enumeration
: Harmony proxies: for-in loop on proxy does not suppress properties deleted du...
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: P2 normal (vote)
: ---
Assigned To: Andreas Gal :gal
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2010-09-29 14:45 PDT by Tom Van Cutsem
Modified: 2011-04-26 15:10 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (687 bytes, patch)
2010-09-29 15:39 PDT, Andreas Gal :gal
brendan: review+
Details | Diff | Splinter Review

Description Tom Van Cutsem 2010-09-29 14:45:45 PDT
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

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.
Comment 1 Andreas Gal :gal 2010-09-29 15:39:13 PDT
Created attachment 479596 [details] [diff] [review]
Comment 2 Brendan Eich [:brendan] 2010-09-30 08:35:02 PDT
Comment on attachment 479596 [details] [diff] [review]

>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))

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.

Comment 3 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-04-16 05:43:24 PDT
Was this patch forgotten? Does it still need pushing?
Comment 4 Andreas Gal :gal 2011-04-16 11:18:07 PDT
Looks like it. Want to grab it jdm or should I land it?
Comment 5 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-04-16 11:21:24 PDT
Please feel free. I'm supposed to be packing to go travelling for six weeks.
Comment 6 Andreas Gal :gal 2011-04-16 14:53:35 PDT
Comment 7 Chris Leary [:cdleary] (not checking bugmail) 2011-04-26 15:10:40 PDT

Note You need to log in before you can comment on or make changes to this bug.