Cross-frame for-in is broken on Firefox 37, 38 beta

[GoogleCodeExporter]

Firefox 37 and 38.0 beta.

  var o = frame.untame(frame.iframe.contentWindow.eval('({})'));
  for (var x in o) {}

The for loop throws "TypeError: undefined is not a function". This shouldn't 
even be possible (without proxies, and there aren't any proxies here), so it's 
probably a browser bug.

Any untame()d record object exhibits the problem. The unsafe eval is just to 
have less machinery involved.

Other language meta-operations (e.g. getOwnPropertyNames) on the object work 
normally.

Original issue reported on code.google.com by kpreid@google.com on 6 Apr 2015 at 11:26

[GoogleCodeExporter]

Further debugging says:

• The taming membrane is not actually involved at all; simply looping on an 
object constructed in the SES frame, from the outer frame, is sufficient.

• If I load SES in a frame (not the rest of Caja), then the problem occurs. 
If I disable the root call to clean() in startSES, then it does not.

Original comment by kpreid@google.com on 8 Apr 2015 at 7:06

[GoogleCodeExporter]

I instrumented cleanProperty to check for the problem before and after each 
deletion. It indicated the problem being at 
cajaVM.anonIntrinsics.IteratorPrototype.next.

If I whitelist this property as '*', then the problem goes away.

I turn this over to MarkM to determine whether this is a bug in Firefox that it 
cares about the property, or something we need to support for ES6-world.

Original comment by kpreid@google.com on 8 Apr 2015 at 9:16

[GoogleCodeExporter]

This is a bug in Firefox. for/of must depend on .next and break if you delete 
it. for/in must not.

Original comment by erights@google.com on 8 Apr 2015 at 9:40

[GoogleCodeExporter]

OK. Can you take care of reporting it?

(Oh, also note that it only occurs cross-frame; for-in does not throw when 
evaluated in the frame which has had the property deleted.)

Original comment by kpreid@google.com on 8 Apr 2015 at 9:42

[GoogleCodeExporter]

Standalone test case (partly constructed from the cajaVM.anonIntrinsics code).

Bizarrely, the thrown TypeError may appear in the console _before_ the rest of 
the messages, depending on version and whether the console is open when the 
page is loaded.

<iframe id="tmf" src="about:blank"></iframe>
<script>
var frame = document.getElementById('tmf');
var frwin = frame.contentWindow;

var iteratorSym = frwin.Symbol.iterator;
var arrayIter = (new frwin.Array())[iteratorSym]();
var ArrayIteratorPrototype = Object.getPrototypeOf(arrayIter);
var arrayIterProtoBase = Object.getPrototypeOf(ArrayIteratorPrototype);
var IteratorPrototype = arrayIterProtoBase;
console.log(IteratorPrototype.next);
delete IteratorPrototype.next;
console.log(IteratorPrototype.next);

var tam = frwin.eval('({})');
console.log('(1) About to fail');
for (var x in tam) { console.log('(1)', x); }
console.log('(1) Didn\'t fail');
</script>

Original comment by kpreid@google.com on 8 Apr 2015 at 9:55

• Changed title: Cross-frame for-in is broken on Firefox 37, 38 beta

[GoogleCodeExporter]

Reported at https://bugzilla.mozilla.org/show_bug.cgi?id=1152550

Original comment by erights@google.com on 8 Apr 2015 at 10:01

[GoogleCodeExporter]

From the discussion on the bugzilla bug thread, it looks likely that Firefox's 
behavior is correct by ES6 and is simply the first browser to implement the ES6 
behavior here rather than the ES5 behavior. We need to fix this on the Caja 
side. The easiest fix, if we decide it is safe, is to whitelist 
cajaVM.anonIntrinsics.IteratorPrototype.next .

Does anyone see any problem whitelisting this? If you were going to look for a 
possible exploit this enables, where would you look?

Original comment by erights@google.com on 9 Apr 2015 at 12:55

• Changed state: Accepted

[GoogleCodeExporter]

FWIW, I haven't consulted the spec, but this _should_ be a purely local 
operator (i.e. it mutates its arguments/this only) so it should be safe.

Original comment by kpreid@google.com on 13 Apr 2015 at 5:30

[GoogleCodeExporter]

See the bugzilla thread. The spec is actually ambiguous and we've agreed to 
tighten it up so it is safe for ES7 -- by making the .next used by the spec be 
an own property of the unobservable iterator driving the iteration, and so 
itself unobservable. It also sounds like FF will adopt this in its 
implementation as well. 

So SES only needs to change something (likely: whitelist .next) if cross-frame 
for/in needs to work for current customer's before the FF fix. Opinions?

Original comment by erights@google.com on 13 Apr 2015 at 5:37

[GoogleCodeExporter]

As a user of Caja, I'm seeing more and more of our users running into this 
issue. Would be wonderful to have a fix soon.

Original comment by ma...@google.com on 13 Apr 2015 at 5:53

[GoogleCodeExporter]

Started: https://codereview.appspot.com/222570043/

Original comment by erights on 13 Apr 2015 at 6:22

• Changed state: Started