1408740 - Objects emulating undefined not handled properly in IteratorClose

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[André Bargull [:anba]]

Test case:
---
var iter = {
  [Symbol.iterator]() { return this; },
  next() { return {}; },
  return: window.document.all,
};
for (var x of iter) break;
---


Expected: Throws TypeError
Actual: No TypeError thrown

Comment 1

[Jeff Walden [:Waldo]]

I have a patch (incidentally, yield* has the same problem), just need to figure out how to write upstreamable test262 tests these days.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch sans tests

Getting a test262 test for this is messy, because it requires a host-defined function that exposes an object emulating |undefined| *and* with defined behavior when an attempt is made to call it.  Gonna take time to upstream such test.  In the meantime, this seems to be the code changes that actually fix this (with the usual caveat that lightly-tested code is often not fully-tested, landable code).

Comment 3

[Jeff Walden [:Waldo]]

https://github.com/tc39/test262/pull/1287 for an upstream PR, but there's an issue to settle still of whether tests belong in test262 (because they're testing iterator protocol details) or in w-p-t (because they use an object akin to |document.all|).  Woo fun spec hot potato.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8919006 [details] [diff] [review]
Patch sans tests

One or the other upstream PR is going to add upstream testing for this, and the separate tests upstream properly target both changes here if I flip the changes on/off accordingly, so this looks ready to go.

Unless someone says otherwise, I'll assume this is landable once a test has landed upstream.

Comment 5

[André Bargull [:anba]]

Comment on attachment 8919006 [details] [diff] [review]
Patch sans tests

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

IteratorClose (https://tc39.github.io/ecma262/#sec-iteratorclose) calls GetMethod (https://tc39.github.io/ecma262/#sec-getmethod), and GetMethod returns `undefined` if the value is either `undefined` or `null`, so we actually need to check for both values.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Updated

Right, I knew that.  :-)

I went for !(null or undef) for the function semantics because both cases here want it.  There's at least one more case that wants the opposite; I'll add a JSOP_NOT to handle it.  (That case could also use JSOP_IFNE, but IfThenElseEmitter doesn't really support JSOP_IFNE, and I'm a little leery of adding such without careful consideration.)

The test landed upstream, so just a matter of reimporting -- and adding uncallableAndIsHTMLDDA support to the runner -- to pick it up.

Comment 7

[André Bargull [:anba]]

Comment on attachment 8919553 [details] [diff] [review]
Updated

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

Thanks!

Comment 8

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e980c5cb174fd2f612ebd616e52a5464bad44d2

...did we lose the bot that drops commit URLs in bugs when a patch is pushed for them, in my absence?  :-\

Upstream testing is...more complicated than expected, because it turns out |document.all| is supposed to throw or not-throw in various unusual ways depending what's passed to it.  :-(  Can't just say the $262.uncallableOrIsHTMLDDA function has to return a noncallable object.

For the IteratorClose case, <iterator>.return is called with no arguments -- and per latest spec that *should* actually return null.  See https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#HTMLAllCollection-call first step.  But it happens that if our |document.all| behaved correctly (either bug 1389922 or bug 1398354), returning null would then cause the spec algorithm to just briefly later throw a TypeError anyway.  So you could say, here, the requirement is that either 1) Call(obj, ..., []) throws a TypeError or 2) the call has no side effects and returns a non-Object value.  So the existing test is correct even with the Gecko bug fixed.

For the yield* case, <iterator>.return is called with one argument.  This case doesn't always throw -- it could return an Element.  But if you pass "" for the argument (which you can, because the argument is user-controllable if you try hard),  calling |document.all| passing "" would return null and thus fall into the same throw-TypeError path as for IteratorClose.

I'm unsure what the right approach for this is.  There could be super-bespoke $262.* functions for each of these very quirky interfaces, but I suspect the very esoteric requirements might get some pushback as such functions multiply.  Hmm.

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8e980c5cb174