928508 - String.prototype.@@iterator (and thus for-of) incorrectly iterates over code units rather than Unicode characters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The spec <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype-@@iterator> is not totally finished yet, but it’s agreed that [..."

Comment 1

[Jason Orendorff [:jorendorff]]

I guess bugzilla does not like emoji.

Well, to say it the boring way:

    js> [..."\uD83D\uDE80"]
    ["\uD83D", "\uDE80"]

This is wrong. Since it's a valid surrogate pair, the result should be ["\uD83D\uDE80"].

(Tests need to cover busted surrogates as well.)

Comment 2

[André Bargull [:anba]]

Attachment: string_iterator.patch

Initial patch for String.prototype[@@iterator]. For the most part this was copying and slightly modifying the existing ArrayIterator code. 

I don't know whether it makes sense to apply the changes right now or instead wait until Allen updates the draft to include String.prototype[@@iterator].

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 819355 [details] [diff] [review]
string_iterator.patch

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

Changing f? to r+ with the comments below addressed.

::: js/src/builtin/String.js
@@ +57,5 @@
> +    return this;
> +}
> +
> +function StringIteratorNext() {
> +    // FIXME: StringIterator prototype should not pass this test.  Bug 924059.

The cross-compartment wrapper problems in bug 924059 are relevant here, so keep the FIXME, but we should be able to handle the StringIterator prototype object correctly. See comments below and please fix this comment.

::: js/src/jit-test/tests/for-of/string-iterator-surfaces.js
@@ +12,5 @@
> +}
> +
> +function isCallable(o) {
> +  try{
> +    (new Proxy(o, {apply: () => {}}))();

I think a simpler way to test ES6 IsCallable() is

    return typeof o === "function";

In any case, make the indentation 4 spaces here and in isConstructor, please; and add a space after the keyword 'try'.

@@ +28,5 @@
> +    return false;
> +  }
> +}
> +
> +function isBuiltinFunction(o, name, arity) {

Please rename to assertBuiltinFunction.

@@ +40,5 @@
> +
> +    assertEq(typeof fn, "function");
> +    assertEq(Object.getPrototypeOf(fn), Function.prototype);
> +    assertEq(isCallable(fn), true);
> +    // FIXME: Proxy should only have [[Construct]] if target has [[Construct]] (bug XXXXXX)

Please do file this and update the comment.

@@ +47,5 @@
> +    arraysEqual(Object.getOwnPropertyNames(fn).sort(), ["length", "name", "arguments", "caller"].sort());
> +
> +    // Also test "name", "arguments" and "caller" in addition to "length"?
> +    assertDataDescriptor(Object.getOwnPropertyDescriptor(fn, "length"), {
> +        value: 0,

Do you mean `value: arity`? No observable bug, but might as well use the argument, since we have it...

@@ +65,5 @@
> +
> +// StringIterator.prototype inherits from Iterator.prototype
> +assertEq(Object.getPrototypeOf(iterProto), Iterator.prototype);
> +
> +// Own properties for StringIterator.prototype "next" and @@iterator

If you write only the bad, string-based version of this, then someone will see it when we implement symbols and fix the test to only accept the symbol.

If you include both versions, then the bad, string-based version of the test will probably stick around forever.

::: js/src/jsiter.cpp
@@ +1880,5 @@
>      }
>  
> +    if (global->getSlot(STRING_ITERATOR_PROTO).isUndefined()) {
> +        const Class *cls = &StringIteratorObject::class_;
> +        proto = global->createBlankPrototypeInheriting(cx, cls, *iteratorProto);

I think TC39 has agreed that iterator objects should not inherit from any common prototype, so please don't use iteratorProto. We'll kill it eventually.

The simplest way to provide the desired observable behavior, for now, is to have

    static const Class StringIteratorPrototypeClass = {
        "String Iterator",
        ... no reserved slots, default everything else ...
    };

Comment 4

[André Bargull [:anba]]

Attachment: bug928508.patch

Updated patch based on review comments. Carrying +r from :jorendorff.

Comment 5

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #3)
> @@ +40,5 @@
> > +
> > +    assertEq(typeof fn, "function");
> > +    assertEq(Object.getPrototypeOf(fn), Function.prototype);
> > +    assertEq(isCallable(fn), true);
> > +    // FIXME: Proxy should only have [[Construct]] if target has [[Construct]] (bug XXXXXX)
> 
> Please do file this and update the comment.

Bug 929467.


> 
> @@ +47,5 @@
> > +    arraysEqual(Object.getOwnPropertyNames(fn).sort(), ["length", "name", "arguments", "caller"].sort());
> > +
> > +    // Also test "name", "arguments" and "caller" in addition to "length"?
> > +    assertDataDescriptor(Object.getOwnPropertyDescriptor(fn, "length"), {
> > +        value: 0,
> 
> Do you mean `value: arity`? No observable bug, but might as well use the
> argument, since we have it...

Not using `value: arity` was a typo, this is now fixed. But in addition to "length", it's also possible to test "name", "arguments" etc. For example the current ES6 draft specifies that built-in functions have poison pilled "arguments" and "caller" properties, so in an ES6 environment it makes sense to test those properties as well, just like I've written here: https://github.com/anba/es6draft/blob/master/src/test/scripts/suite/lib/assert.js#L192-L201. I'm not sure whether it makes sense to perform the same tests right now in SpiderMonkey.


> 
> @@ +65,5 @@
> > +
> > +// StringIterator.prototype inherits from Iterator.prototype
> > +assertEq(Object.getPrototypeOf(iterProto), Iterator.prototype);
> > +
> > +// Own properties for StringIterator.prototype "next" and @@iterator
> 
> If you write only the bad, string-based version of this, then someone will
> see it when we implement symbols and fix the test to only accept the symbol.
> 
> If you include both versions, then the bad, string-based version of the test
> will probably stick around forever.

Oki, only testing the bad version. ;-)


> 
> ::: js/src/jsiter.cpp
> @@ +1880,5 @@
> >      }
> >  
> > +    if (global->getSlot(STRING_ITERATOR_PROTO).isUndefined()) {
> > +        const Class *cls = &StringIteratorObject::class_;
> > +        proto = global->createBlankPrototypeInheriting(cx, cls, *iteratorProto);
> 
> I think TC39 has agreed that iterator objects should not inherit from any
> common prototype, so please don't use iteratorProto. We'll kill it
> eventually.
> 
> The simplest way to provide the desired observable behavior, for now, is to
> have
> 
>     static const Class StringIteratorPrototypeClass = {
>         "String Iterator",
>         ... no reserved slots, default everything else ...
>     };

I've added the proposed StringIteratorPrototypeClass Class, but I don't know whether or not it is necessary to set the JSCLASS_IMPLEMENTS_BARRIERS flag. (In the updated patch, I've set that flag.)

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to André Bargull from comment #5)
> I've added the proposed StringIteratorPrototypeClass Class, but I don't know
> whether or not it is necessary to set the JSCLASS_IMPLEMENTS_BARRIERS flag.
> (In the updated patch, I've set that flag.)

That's right. It should be set.

(This flag says that objects of this class do not have any mysterious mutable internal state that violates certain incremental GC rules. Having this flag on most classes allows incremental GC to be faster overall.)

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to André Bargull from comment #5)
> the current ES6 draft specifies that built-in functions have poison pilled
> "arguments" and "caller" properties, so in an ES6 environment it makes sense
> to test those properties as well, just like I've written here:
> https://github.com/anba/es6draft/blob/master/src/test/scripts/suite/lib/
> assert.js#L192-L201. I'm not sure whether it makes sense to perform the same
> tests right now in SpiderMonkey.

Filed bug 929642 to get this fixed. Omit testing it for now, since it doesn't work yet.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fbd79b930b2