1486656 - For-in can hit deleted keys

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[bakkot]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.106 Safari/537.36

Steps to reproduce:

// Run the following
if (typeof console === 'undefined') console = { log: print };

let a = {
  x: 0,
  y: 1,
};

let pa = new Proxy(a, {});

for (let key in pa) {
  console.log(key);
  delete a.y;
  if (key === 'y') {
    console.log('reached');
  }
}



Actual results:

Prints `x, y, reached`.


Expected results:

Prints only `x`. It should not reach the step with `y`, since `y` has been deleted before it gets to that iteration of the loop. The spec is pretty loose here, but does not allow this behavior: in #sec-enumerate-object-properties, it says "A property that is deleted before it is processed by the iterator's next method is ignored." No other engine except moddable's embedded XS engine has this behavior.

This doesn't happen if using `a` instead of the proxy `pa`, even though `pa` is a transparent wrapper around `a`.

If you add a trap for `getOwnPropertyDescriptor`, you can observe that `getOwnPropertyDescriptor` is being called for each property before enumerating executing the body of the loop at all, presumably to determine presence and enumerability. But because properties may be deleted before their iteration of the loop is reached, you need to check for presence immediately before the loop step covering them, not ahead of time. (It's not clear whether it's legal to check enumerability ahead of time.)

See also (and please comment on) this open spec bug about more precisely specifying the behavior of for-in, which prompted the investigation which lead me to discovering this: https://github.com/tc39/ecma262/issues/1281

Comment 1

[bakkot]

Actually, this can happen even with no proxies:


```
if (typeof console === 'undefined') console = { log: print };


let a = { x: 0 };

let b = { y: 1 };

Object.setPrototypeOf(a, b);

for (let key in a) {
  console.log(key);
  delete b.y;
  if (key === 'y') {
    console.log('reached');
  }
}
```

This prints `x, y, reached`. This is forbidden per the spec text cited above, and no other engine has this behavior.

Comment 2

[bakkot]

Ah, this is maybe a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=862771.

Comment 3

[Nicolas B. Pierron [:nbp]]

André, is this a correct interpretation of the spec, or is there anything specific with Proxy? MDN [1] seems to agree on for-in behaviour described in this issue.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in

Comment 5

[André Bargull [:anba]]

For the test case in comment #0:

When the [[Enumerate]] MOP method was removed post-ES6, the text for ordinary object's [[Enumerate]] was more or less copied to EnumerateObjectProperties, including the restriction to omit properties after they have been deleted. But I'm not actually sure this was an intended change at that point, cf. Brian's comment at <https://github.com/tc39/ecma262/pull/367/files#r52693126> which seems to imply that Proxies are allowed to invoke [[GetOwnProperty]] in any order, for example before evaluating ForIn/OfBodyEvaluation, so that Proxies can compute all enumerable properties at once.

Relevant spec issues and PR:
- https://github.com/tc39/ecma262/issues/160
- https://github.com/tc39/ecma262/issues/161
- https://github.com/tc39/ecma262/pull/367


Another test case to see the different behaviour across engines when enumerability is determined for Proxy objects.
---
function test(o) {
  for (var k in o) {
    print(k);
    Object.defineProperty(o, "b", {enumerable: false});
  }
}

// Prints "a" and "b" in all major engines, but only "a" when following
// the example EnumerateObjectProperties implementation from 13.7.5.15.
test({a: 0, b: 0});

// Prints "a" and "b" in SpiderMonkey and JSC, but only "a" in V8 and Chakra.
test(new Proxy({a: 0, b: 0}, {}));
---



For the test case in comment #1:

I don't see an easy way to (mis)interpret the spec somehow to allow the current SpiderMonkey behaviour, so I guess that case needs to be rated as a spec violation. (The implementation of js::SuppressDeletedProperty makes it quite obvious why deleted properties show up in SpiderMonkey, because we only check if the object whose property was deleted is currently in an enumerator, which totally ignores any prototype objects.)

Comment 6

[bakkot]

I read Brian's comment as being wrt the order of `[[GetOwnProperty]]` invocations among different properties, rather than among other steps in the enumeration. But I agree it's hard to interpret definitively either way (cf https://github.com/tc39/ecma262/issues/1281).

For the proxy case, I'd very much like to specify a particular algorithm (probably the example one) which engines are required to use when for-in hits a proxy. See https://github.com/bakkot/for-in-exploration.

When I presented this at the September 2018 TC39 the committee indicated they'd be amenable, but since we're discussing it, can I ask people here what their thoughts are? To be clear, that change would not affect the requirements for the non-proxy case (though I'd also like to specify precise behavior in some cases there, in a way which would not require any changes from any major implementation).