1114580 - Implement ES6 Symbol.toStringTag

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Comment 1

[ziyunfei]

If someone can mentor me, I'd like to try to implement this. :)

Comment 2

[Jason Orendorff [:jorendorff]]

Still interested? I can mentor.

Comment 3

[Caitlin Potter [:caitp]]

I'd be happy to review or help review submitted patches (and might even suggest marking this as a good first bug due to being heavily tied to the JS standard lib, and being an easily self-hosted JS feature)

@@toStringTag has some (negative) perf implications (A quick glance at https://github.com/mozilla/gecko-dev/blob/fe91296868526cab0a7e5776c6fcd0aa3bfb8601/js/src/builtin/Object.cpp#L303-L362 looks like it could kill some no-alloc optimizations for common classes), and has had an impact on some Dromaeo benchmarks (which arguably rely too much on O.p.toString in a sort of unrealistic way)

How it affects WebIDL bindings is sort of a work-in-progress thing (and in blink/v8 it's been temporarily unshipped until the bindings story is figured out), I've filed https://github.com/heycam/webidl/issues/54 to try to get that hammered out in more detail.

Comment 4

[Till Schneidereit [:till]]

Ziyunfei, are you still interested in working on this? Otherwise, it'd be nice to be able to take caitp up on her generous offer after letting someone else have a go at this.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Note that for cross-origin windows we'd want @@toStringTag to return undefined, not throw.

Comment 6

[George]

I would like to take it up and hoping to complete with some mentoring if possible.

I checked out the file https://github.com/mozilla/gecko-dev/blob/fe91296868526cab0a7e5776c6fcd0aa3bfb8601/js/src/tests/ecma_6/Symbol/toString.js which looks to be passing. 

Could a test case with the @@tag be the first step ?

Comment 7

[George]

Attachment: toStringTag.basic.patch

This patch just adds the toStringTag to the wellKnownList of Symbols. Its the basic version which makes the symbol available as Symbol.toStringTag. 

A test case is also added to the patch which contains the test to check if the symbol exists.

Could this big be given to me. Looking forward to comments and ways to complete the toStringTag function and hope to get some mentoring.

Comment 8

[Tooru Fujisawa [:arai]]

You could use well-known.js [1] test for checking property type, descriptor, etc :)

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/tests/ecma_6/Symbol/well-known.js

Comment 9

[George]

Attachment: The patch enables the to toStringTag in the global Symbol

Could some please have a look at this patch ? The regression results are positive and it passes. What more is there to be done for this to be completed.

Comment 10

[Caitlin Potter [:caitp]]

The change to the test looks good (can't r+, but I'm sure :arai will).

If this isn't blocked on implementing the changes to JS_BasicObjectToString(), it can probably land. However, I'm of the opinion that it's better to implement the changes to JS_BasicObjectToString in the same patch.

Comment 11

[Jason Orendorff [:jorendorff]]

We should definitely do these three things all at the same time:

1.  introduce Symbol.toStringTag
2.  change Object.prototype.toString to use it as specified in ES6 19.1.3.6
3.  add @@toStringTag properties on prototype objects throughout the engine,
    as specified throughout the standard (search for "[ @@toStringTag ]")

We must implement 2 and 3 at the same time in order to avoid breaking web sites that depend on .toString(). We should implement 1 at the same time as the others so that the existence of Symbol.toStringTag can be used for feature testing.

Comment 12

[Jason Orendorff [:jorendorff]]

The patch is a correct patch for part 1, but we need the other parts --- including tests.

George, are you interested in doing some more work? :)

Comment 13

[George]

Yes Jason, excited to do more work. Great to know that Part 1 is done. 

For completing tasks 2 and 3.

Step 2
  Write test cases to satisfy each case of ES6 19.1.3.6. 
  Implement Object.prototype.toString to satisfy the case.  

Step 3
  Write test case to test "[ @@toStiringTag ]" throughout engine.
  Implement it in the code. 

There is a comment previously which states that there could be some performace issues that could be expected while implementing toString(). Is there something I need to be careful of while implementing this ? 

Caitlin mentioned about cross origin access issues happening in chrome. Is a situation like that going to happen in spidermonkey as well ? 

This is a link to the issue referred to. 
http://jsfiddle.net/kboybsbh/

Comment 14

[Caitlin Potter [:caitp]]

It looks like it should be much simpler to solve the access check issue here, because of the rich JSPrincipal API in xpconnect. I'm also not sure the performance hit will be as significant in SpiderMonkey. You need to load an additional property and test that it's a string, but it looks like it won't be as bad here.

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to George from comment #13)
> Step 2
>   Write test cases to satisfy each case of ES6 19.1.3.6.
>   Implement Object.prototype.toString to satisfy the case.

Right. You can put the new test file(s) in js/src/tests/ecma_6/Object/toString.js or toString-1.js, toString-2.js, etc. if it gets to be more than a single logical thread of stuff.

Test files have this at the top:

    /* Any copyright is dedicated to the Public Domain.
     * http://creativecommons.org/licenses/publicdomain/ */

and this at the bottom:

    reportCompare(0, 0);

and in between, use assertions like this:

    assertEq(Object.prototype.toString.call(37), "[object Number]");

To run a test:

    cd js/src/tests
    ./jstests.py -s -o $JS_SHELL TESTFILE


> Step 3
>   Write test case to test "[ @@toStringTag ]" throughout engine.
>   Implement it in the code.

The hardest part of this is finding where all the relevant objects are defined in the engine. It really is all over the place.

> There is a comment previously which states that there could be some
> performace issues that could be expected while implementing toString(). Is
> there something I need to be careful of while implementing this ?

No. We don't care about those benchmarks.

> Caitlin mentioned about cross origin access issues happening in chrome. Is a
> situation like that going to happen in spidermonkey as well ? 
>
> This is a link to the issue referred to.
> http://jsfiddle.net/kboybsbh/

Yes. This means there's more work to do here:

4.  Make our security layer detect cross-origin access to @@toStringTag and have it return undefined rather than throwing a security exception. I'm not sure yet how to do this.

I do know that we'll need a different kind of test for this one, like a mochitest, because that security layer isn't present in the JS shell.

5.  Make sure all DOM objects that need @@toStringTag have it, so that the result of stuff like `document.toString()` doesn't suddenly change from "[object HTMLDocument]" to "[object Undefined]" which would be an incompatible change.

Comment 16

[Jason Orendorff [:jorendorff]]

Boris, two questions:

1.  Do Xray wrappers support intercepting a cross-origin [[Get]] and forcing it to return undefined?

2.  Is there a nice bottleneck in the DOM bindings code where we can define a @@toStringTag property on each DOM prototype? Is that the right thing to do?

Comment 17

[Jason Orendorff [:jorendorff]]

Maybe DefineConstructorAndPrototype (in jsobj.cpp) should automatically define a @@toStringTag property on the new prototype object. This would eliminate the need for custom code in some cases. Not all.

Comment 18

[Boris Zbarsky [:bzbarsky]]

That first question is really for Bobby.  I might be able to figure it out if I poked in the code a bit, but he should know off the top of his head.  In any case, that's certainly the behavior we want for @@toStringTag gets on cross-origin wrappers, per that es-discuss thread.

As for bindings... the problem is that instances and prototypes current have different class names.  Think "Document" vs "DocumentPrototype".  So you'd need @@toStringTag on each instance or something, or have @@toStringTag be an accessor property on the prototype, or change behavior so that the prototype also say "Document".  See also discussion in <https://www.w3.org/Bugs/Public/show_bug.cgi?id=28244>.

My gut feeling is we just want an accessor @@toStringTag on DOM prototypes in the near term.  We could definitely set that up in the bindings.  We could probably even have that accessor return undefined when invoked via Xrays, and make the access check machinery allow the get through, thus kinda answering your first question.

Comment 19

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #16)
> 1.  Do Xray wrappers support intercepting a cross-origin [[Get]] and forcing
> it to return undefined?

XrayWrappers can do what we want, but cross-origin objects have very specific behavior [1]. Any changes to that behavior need to be discussed carefully.

> In any case, that's certainly the behavior we want for @@toStringTag gets on cross-origin wrappers,
> per that es-discuss thread.

Can someone describe the specific desired behavior in more detail? I don't follow es-discuss, and recognize that this generally waives my right to have an opinion on most things. However, the precise MOP behavior of cross-origin objects happens to be an area where my input is pretty much required to produce a valid outcome.

[1] https://etherpad.mozilla.org/html5-cross-origin-objects

Comment 20

[Bobby Holley (:bholley)]

And to be more specific, cross-origin objects have null prototypes, and accessing any own property that isn't specifically whitelisted throws an exception. Neither toString nor valueOf are whitelisted, so I don't think there's anything specific we need to support in that department.

Comment 21

[Bobby Holley (:bholley)]

(a link to the es-discuss thread mentioned above would be great, since I can't seem to find it in the archives)

Comment 22

[Caitlin Potter [:caitp]]

The es discuss thread was iirc span from public-script-coord https://lists.w3.org/Archives/Public/public-script-coord/2015JulSep/0022.html

Comment 23

[Bobby Holley (:bholley)]

Oh dear. Mea culpa.

What's been happening is that I'm actually subscribed to those lists, but my filters put them in a place where I never look at them, which unfortunately includes threads where I'm explicitly CCed, like that one. :-(

Let me try to repair the damage here.

Comment 24

[Bobby Holley (:bholley)]

Ok, I've read all the backstory and understand jorendorff's original question.

To implement this, we should do the following:

(1) Modify AccessCheck::isCrossOriginAccessPermitted to respect the whitelist.
(2) Modify CrossOriginXrayWrapper::getPropertyDescriptor to return undefined for any symbol-valued id.

Comment 25

[Jason Orendorff [:jorendorff]]

George, I think you have everything you need to tackle at least steps 2 and 3... and even a few hints toward step 4 (see comment 24) and step 5 (comment 18).

Let us know if you need anything else!

Comment 26

[Jason Orendorff [:jorendorff]]

Er... even steps 2 and 3 would be pretty daunting if you haven't hacked on this codebase before, so don't hesitate to ask anything. We're on IRC too. https://wiki.mozilla.org/IRC

Comment 27

[George]

Thanks for all the help. 

I will dig into it a bit and update periodically here and in IRC when I reach somewhere or if I get stuck.

Comment 28

[George]

Attachment: toStringTag.patch

This patch is the next version with Step 2 and Part of Step 3 added with objects that can easily be found. 

Could I get some suggestions as to whether the patch is in the right direction or if it needs a complete revamp. 

I have written a test case as mentioned in the comments. Some of the types were not returned as parameters and were functions. How could I be handling such situations.

Looking forward to comments

Comment 29

[Jason Orendorff [:jorendorff]]

Comment on attachment 8683370 [details] [diff] [review]
toStringTag.patch

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

Thank you for the patch! Sorry for the very slow review here.

This looks like a good start. I have a bunch of comments below, but each one should be easy to address. I hope this doesn't seem too intimidating!

But if you don't have time to work through this soon, please let me know and I can take over.

::: js/src/builtin/MapObject.cpp
@@ +1030,5 @@
>          RootedId iteratorId(cx, SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator));
>          if (!JS_DefinePropertyById(cx, proto, iteratorId, funval, 0))
>              return nullptr;
> +
> +        // ES6 23.2.3.12 Set.prototype [ @@toStringTag ] 

There's a lot of trailing whitespace in this patch. Please go through and delete it. Thanks!

@@ +1034,5 @@
> +        // ES6 23.2.3.12 Set.prototype [ @@toStringTag ] 
> +        RootedString toStringVal(cx, JS_NewStringCopyZ(cx, "Set"));
> +        RootedId toStringId(cx, SYMBOL_TO_JSID(cx->wellKnownSymbols().toStringTag));
> +        if (!JS_DefinePropertyById(cx, proto, toStringId, toStringVal, JSPROP_READONLY))
> +             return nullptr;

1. Instead of using JS_NewStringCopyZ() here, let's create this string once and share it across all web pages. You can do that in two steps: first, add an entry in js/src/vm/CommonPropertyNames.h; then use

    RootedValue tag(cx, StringValue(cx->names().Set));

2. Instead of the public JS_DefinePropertyById, use DefineProperty().

3. Consider factoring out these few lines of code into a function, so that each builtin constructor only needs to do

    if (!DefineToStringTag(cx, proto, cx->names().Set))
        return nullptr;

4. This patch does the trick for standard builtin classes. Please also add a tag in DefineConstructorAndPrototype() in jsobj.cpp. This affects non-DOM classes defined outside the JS engine -- mostly weird internal stuff.

(At some point we will *also* need to figure out how to add tags for DOM classes, like Window and HTMLDocument. That might involve CreateInterfacePrototypeObject in dom/bindings/BindingUtils.cpp. But the DOM standard around this is still not really finished yet, so let's worry about this part later.)

5. Try building the browser and running js/xpconnect/tests/chrome/test_xrayToJS.xul. It will fail, complaining that you've added new properties to a bunch of prototype objects. Update the test so that it expects the new @@toStringTag properties to be there. Then it should pass again.

::: js/src/builtin/Object.cpp
@@ +301,5 @@
>  }
>  #endif /* JS_HAS_TOSOURCE */
>  
> +JS::Symbol*
> +getToStringTagSymbol(JSContext* cx) 

SpiderMonkey style rules:

- If a function is local to a particular file, put the `static` keyword before the return type.

- Function names start with an uppercase letter.

- Only existing function names that are public APIs (visible outside the JS engine) should start with "JS_".

@@ +335,5 @@
> +    // Get symbol info later. 
> +    StringBuffer sb(cx);
> +    RootedId id(cx, SYMBOL_TO_JSID(getToStringTagSymbol(cx)));
> +    RootedValue toStrTagVal(cx);
> +    if(JS_GetPropertyById(cx, obj, id, &toStrTagVal)) {

Use js::GetProperty, and as a rule, try to early-return on error, like this:

    if (!GetProperty(cx, obj, obj, id, &tagVal))
        return nullptr;

@@ +337,5 @@
> +    RootedId id(cx, SYMBOL_TO_JSID(getToStringTagSymbol(cx)));
> +    RootedValue toStrTagVal(cx);
> +    if(JS_GetPropertyById(cx, obj, id, &toStrTagVal)) {
> +        sb.append("[object "); 
> +        sb.append(toStrTagVal.toString());

toStrTagVal.toString() works only if toStrTagVal.isString(). Otherwise, it crashes!  You should be able to write a test that triggers this crash. Try it out. :)

Of course the fix is that this code just needs to implement step 16 of the standard:

    16.  If Type(tag) is not String, let tag be builtinTag.

@@ +351,2 @@
>  JSString*
>  JS_BasicObjectToString(JSContext* cx, HandleObject obj)

It seems this public API is not being used anymore. Please delete the declaration from jsapi.h, and move all the code from this function into obj_toString().

@@ +351,4 @@
>  JSString*
>  JS_BasicObjectToString(JSContext* cx, HandleObject obj)
>  {
>      // Some classes are really common, don't allocate new strings for them.

We'll need some additional changes in order to get the behavior specified in <https://tc39.github.io/ecma262/#sec-dataview-constructor>.

It really does need to be effectively the same as following the standard step by step, for maximum cross-browser compatibility.

However, we are free to do things in a slightly different order, for speed. So perhaps:

    RootedString builtinTag(cx);
    bool isArray;
    if (obj->is<PlainObject>()) {
        builtinTag = cx->names().objectObject;
    } else if (obj->is<StringObject>()) {
        builtinTag = cx->names().objectString;
    } else if (!JS::IsArray(cx, obj, &isArray)) {
        return false;
    } else if (isArray) {
        builtinTag = cx->names().objectArray;
    } else ...
        ... 6 other cases ...
    } else {
        builtinTag = cx->names().objectObject;
    }
    
    ... code for steps 14-17 goes here ...

    JSString* result = sb.finishString();
    if (!result)
        return false;
    args.rval().setString(result);
    return true;

::: js/src/tests/ecma_6/Symbol/toString.js
@@ +6,4 @@
>      {sym: Symbol("ok"), str: "Symbol(ok)"},
>      {sym: Symbol("\0"), str: "Symbol(\0)"},
>      {sym: Symbol.iterator, str: "Symbol(Symbol.iterator)"},
> +    {sym: Symbol.toStringTag, str: "Symbol(Symbol.toStringTag)"},

This change isn't necessary - the test just tries converting one of each kind of symbol.

::: js/src/tests/ecma_6/Symbol/toStringTag.js
@@ +31,5 @@
> +	[true, "[object Boolean]"],
> +	[5, "[object Number]"],
> +	[new Date(), "[object Date]"],
> +	[/regexp/, "[object RegExp]"],
> +	[obj, "[object abc]"],  //Add this to the list once fixed.

This comment is bogus, right?

@@ +60,5 @@
> +// ES6 19.4.3.5 Symbol.prototype [ @@toStringTag ]
> +testDefault(Symbol.prototype, "Symbol");
> +
> +// ES6 20.2.1.9 Math [ @@toStringTag ]
> +// testDefault(Math, "Math"); // Ask in IRC. 

Ask what? :)

@@ +101,5 @@
> +// ES6 25.3.1.5 Generator.prototype [ @@toStringTag ]
> +// Test legacy and star.
> +// testDefault(genFn.prototype, "Generator");
> +// Looks like none of them are implemented as an object.
> +// The description is not returned from them.

Right. For some reason the standard says that these are not supposed to be exposed as global builtins.

I think you can get Generator and GeneratorFunction like this:

    function* gen() {}
    let Generator = Object.getPrototypeOf(gen);
    let GeneratorFunction = gen.constructor;

@@ +105,5 @@
> +// The description is not returned from them.
> +
> +// ES6 25.4.5.4 Promise.prototype [ @@toStringTag ]
> +// testDefault(Promise.prototype, "Promise");
> +// Promise is not yet implemented.

if (this.Promise) {
    ...test Promise...
}

That way this part of the test will be enabled when we run it in the browser. (Promise has been implemented in Firefox for some time.)

@@ +110,5 @@
> +
> +// ES6 26.3.1 @@toStringTag
> +// testDefault(Symbol.toStringTag, "Module"); 
> +// Where should this be handled. 
> +// An object needs to be created inside the wellKnownSymbols. 

I don't understand this.

Comment 30

[Tom S [:evilpie]]

Attachment: WIP

Comment 31

[Tom S [:evilpie]]

Attachment: v1 - Implement @@toStringTag

This is a complete implementation including all @@toStringTag properties defined in the spec, except for Promise. For that we would need a way to define string value properties with just the ClassSpec.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a43c82e1b0db&selectedJob=21931146

Comment 32

[George]

I tried finding time to complete it but unfortunately could not find time to get it done. Good to see the progress with it :)

Comment 33

[Tom S [:evilpie]]

Those CGC failures on try look suspicious and need to be investigated after I manage to reproduce them locally.

Comment 34

[Tom S [:evilpie]]

So, I investigated the plugin failure. We call NPObjWrapper_GetProperty with a symbol jsid. That code ends up calling PluginScriptableObjectParent::GetPropertyHelper, which uses FromNPIdentifier to turn the jsid (now casted as NPIdentifier) to something transportable. However FromNPIdentifier does not handle symbols at all. So we either have to change that, or always return undefined for symbol properties (There already is special handling for @@toPrimitive) in NPObjWrapper_GetProperty.

Needinfo to everyone who came up on blame.

Comment 35

[(No longer employed by Mozilla) Aaron Klotz]

Anything I touched in there was relating to async plugin init, so I don't really have anything to add on this topic.

Comment 36

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I can't imagine plugins care about symbol jsids. Please just do whatever is easiest.

Comment 37

[Tom S [:evilpie]]

Attachment: v2 - Implement @@toStringTag

This seems to pass try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8f32de68010&selectedJob=22247482.

Same caveat as before, no Promise support.

Comment 38

[Tom S [:evilpie]]

Attachment: Remove DOM test known failure

Comment 39

[Tom S [:evilpie]]

Attachment: Always return undefined to symbol lookup on plugin objects

Comment 40

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8761762 [details] [diff] [review]
Remove DOM test known failure

You'll also need to update /dom/imptests/failures/html/js/builtins/test_WeakMap.prototype-properties.html.json, I think. (Don't forget to remove the trailing comma from the previous line.)

r=me assuming this gets folded into the commit that makes it pass.

Comment 42

[Jason Orendorff [:jorendorff]]

Comment on attachment 8761761 [details] [diff] [review]
v2 - Implement @@toStringTag

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

I'm only ~20% through this review, but posting this much since I'm curious.

::: js/src/builtin/MapObject.cpp
@@ +337,5 @@
>          RootedId iteratorId(cx, SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator));
>          if (!JS_DefinePropertyById(cx, proto, iteratorId, funval, 0))
>              return nullptr;
> +
> +        // Define Symbol.toStringTag.

Thanks for the comment, but it's misleading. Instead:

        // Define Map.prototype[@@toStringTag].

@@ +1056,5 @@
>          RootedId iteratorId(cx, SYMBOL_TO_JSID(cx->wellKnownSymbols().iterator));
>          if (!JS_DefinePropertyById(cx, proto, iteratorId, funval, 0))
>              return nullptr;
> +
> +        // Define Symbol.toStringTag.

Same here.

::: js/src/builtin/Object.cpp
@@ +337,5 @@
> +    RootedString builtinTag(cx);
> +    if (isArray)
> +        builtinTag = cx->names().objectArray;
> +    else if (obj->is<StringObject>())
> +        builtinTag = cx->names().objectString;

The spec requires these steps to work on objects from other globals:

    var g = newGlobal();
    var d = new g.Date();
    d[Symbol.toStringTag] = "Lump";
    assertEq(Object.prototype.toString.call(d), "[object Date]");

I think this will fail in the current patch, because such objects are wrappers. (Please add the test if so.)

Not sure what the best fix is here. I'm guessing we should call CheckedUnwrap(cx, obj), and if that fails, clear the exception and continue to step 15? Maybe the implementations of ProxyHandler::className in js/xpconnect/wrappers have code or comments that will tell us more?

@@ +353,5 @@
> +        builtinTag = cx->names().objectDate;
> +    else if (obj->is<RegExpObject>())
> +        builtinTag = cx->names().objectRegExp;
> +    // Step 14.
> +    // Currently omitted for non-standard fallback.

Style nit: Blank line before the comment, please.

Comment 43

[Jason Orendorff [:jorendorff]]

Comment on attachment 8761761 [details] [diff] [review]
v2 - Implement @@toStringTag

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

Fantastic tests.

A few to add:
* a strict-mode arguments object
* toString.call on special-cased objects returns the expected special answer, even if the object's @@toStringTag property or __proto__ has been set.
* toString.call(Object.create(JSON)) === "[object JSON]"
* but toString.call(Object.create(new Number)) === "[object Object]"
* toString.call(proxy) calls the "get" proxy handler, and nothing else
* ...even if the target object is one of the types that Object.prototype.toString handles specially, like a Date
* ...unless the target object is an array, or a proxy to an array. Then the handler is not called.

r=me for everything so far, with the cross-compartment wrapper issue fixed. I imagine your changes here will be worth a new review (preferably a new patch containing only the changes).

::: js/src/builtin/TypedArray.js
@@ +1384,5 @@
> +
> +    // Steps 4-6.
> +    // Modified to retrieve the [[TypedArrayName]] from the constructor.
> +    var constructor = _ConstructorForTypedArray(O)
> +    return constructor.name;

I think the spec requires this to still work after `delete Int8Array.name`. Follow-up bug, still blocking es6; or fix it, maybe by rewriting this getter in C++ (alas).

Comment 44

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8761763 [details] [diff] [review]
Always return undefined to symbol lookup on plugin objects

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

Given the comment here:
http://searchfox.org/mozilla-central/source/dom/plugins/base/nsJSNPRuntime.cpp#1758-1768
I'm now a little worried that we'll break Java or something.

I thought about this more and I think I have a better solution. What if we do the same thing for toString as we do for toPrimitive? It looks like toPrimitive just calls toString anyway:
http://searchfox.org/mozilla-central/source/dom/plugins/base/nsJSNPRuntime.cpp#1756

Does this plan sound okay to you Tom? I think you just need to change a few lines.

Comment 45

[Tom S [:evilpie]]

Attachment: Change ESClass to a proper type

Changes ESClassValue to ESClass and makes it a proper enum class. Also adds Arguments/Error, which I need for Object.prototype.toString soon.

Comment 46

[Jason Orendorff [:jorendorff]]

Comment on attachment 8766007 [details] [diff] [review]
Change ESClass to a proper type

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

Nice, thanks.

::: js/ipc/WrapperAnswer.cpp
@@ +489,5 @@
>  bool
>  WrapperAnswer::RecvGetBuiltinClass(const ObjectId& objId, ReturnStatus* rs,
>                                     uint32_t* classValue)
>  {
> +    *classValue = static_cast<uint32_t>(js::ESClass::Other);

This doesn't need a static_cast; it can just be `uint32_t(js::ESClass::Other)`.

@@ +507,4 @@
>      if (!js::GetBuiltinClass(cx, obj, &cls))
>          return fail(jsapi, rs);
>  
> +    *classValue = static_cast<uint32_t>(cls);

and here

::: js/ipc/WrapperOwner.cpp
@@ +742,4 @@
>      ReturnStatus status;
>      if (!SendGetBuiltinClass(objId, &status, &cls))
>          return ipcfail(cx);
> +    *classValue = static_cast<ESClass>(cls);

and here

::: js/src/shell/js.cpp
@@ +7525,5 @@
>      DestructSharedArrayBufferMailbox();
>  
> +    ESClass test;
> +    uint32_t x = static_cast<uint32_t>(ESClass::Error);
> +    test = static_cast<ESClass>(x);

Looks like you didn't mean to commit this.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3e76c6750c
Change ESClassValue to an enum class. r=jorendorff

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc3e76c6750c

Comment 49

[Tom S [:evilpie]]

Attachment: v3 - Implement @@toStringTag

I am going to fix the TypedArray getter in a new bug.

Comment 50

[Jason Orendorff [:jorendorff]]

Comment on attachment 8766900 [details] [diff] [review]
v3 - Implement @@toStringTag

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

I left a bunch of wrong comments last time, because I thought that @@toStringTag was not queried for certain types of objects. This isn't the case; the spec says (rather stupidly) to figure out a fallback value first, then Get @@toStringTag, and then use the fallback value if the Get didn't produce a string.

So the behavior in this patch looks correct, but please add one last test, checking that you really *can* set an @@toStringTag property on a RegExp or something, and it does affect the Object.prototype.toString output.

::: js/src/builtin/Object.cpp
@@ +370,5 @@
> +                builtinTag = cx->names().objectFunction;
> +            break;
> +        }
> +    }
> +    // Step 14.

Style nit: Blank line before this comment, please.

::: js/src/jit-test/tests/debug/Object-class.js
@@ +11,5 @@
>      assertEq(arr[2].class, "Function");
>      assertEq(arr[3].class, "Date");
>      assertEq(arr[4].class, "Object");
>      assertEq(arr[5].class, "Function");
> +    assertEq(arr[6].class, "Object");

Yeah, OK. This seems like an improvement, actually...

Comment 51

[Tom S [:evilpie]]

We've got a bit of a new issue. Because <embed> elements for some weird reason are callable, they now get [object Function]. We would need to define a @@toStringTag element with "HTMLEmbedElement" to preserve the current result.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5397ba2f8274&selectedJob=23554862

There is also something about test_xrayToJS that I don't understand yet: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5397ba2f8274&selectedJob=23554306. Are xrayed prototypes of Boolean etc plain objects?

Comment 52

[Boris Zbarsky [:bzbarsky]]

> Because <embed> elements for some weird reason are callable, they now get [object Function].

Why?  Did you change the priority of the class name thing wrt the other bits?  Seems to me like class name should be checked right after @@toStringTag...

Note that this also affects the ctypes xpcshell tests: 

 TEST-UNEXPECTED-FAIL | toolkit/components/ctypes/tests/unit/test_jsctypes.js | do_check_class - [do_check_class : 43] "[object Function]" == "[object CType]" 

and some other tests that have the same issue for <object>, not surprisingly.

> Are xrayed prototypes of Boolean etc plain objects?

xrayed prototypes are not.  However if you look at the test, it does:

      let expectedProto = /Opaque/.test(new iwin[c]) ? iwin['Object'].prototype
                                                     : iwin[c].prototype;

and then:

      is(Object.getPrototypeOf(new iwin[c]), expectedProto,
         "prototype is correct: " + c);

So at a guess, the toString result on "new iwin[c]" changed to no longer include the string "Opaque" so we're setting the wrong expectedProto (iwin[c].prototype instead of iwin.Object.prototype).  But the actual prototype we see is in fact iwin.Object.prototype which is where that "[object Object]" string comes from.

The "Opaque" thing is supposed to happen when we try to create an Xray to something that is not Xrayable.  In this case, looks like Boolean, Number, and String objects.  Worth checking what "String(new iwin[c])" is at that point for those values of "c".

Comment 53

[Tom S [:evilpie]]

So fixing the first issue was super easy, thanks for the JSCLASS_IS_DOMJSCLASS ide.

Anyway I am not sure what is going on in xray test. We end up calling Wrapper::getBuiltinClass, from obj_toString, so we indeed don't seem to have an opaque object.

Comment 54

[Boris Zbarsky [:bzbarsky]]

> Anyway I am not sure what is going on in xray test. 

Have you tried just stepping through that stringification attempt?

Comment 55

[Tom S [:evilpie]]

Attachment: Define getBuiltinClass on Xray

So we figured out the that problem with Xray wrapper is that they don't handle getBuiltinClass at all. So in this case even Opaque wrappers get the default getBuiltinClass implementation, which is transparent. So an opaque wrapper for a boolean object, would claim to be a boolean. Changing this to ESClass::Other preserve the opaqueness of the wrapper in this case.

Comment 56

[Tom S [:evilpie]]

Anyone has an opinion on how to fix ctypes? We can either define @@toStringTag on most the objects, or just remove the explicit toString test: https://dxr.mozilla.org/mozilla-central/search?q=do_check_class.

This yet another issue caused by the [[Call]] check, but before removing the classname fallback we would have to fix it anyway.

Comment 57

[Tom S [:evilpie]]

(In reply to Tom Schuster [:evilpie] from comment #55)
> Created attachment 8770961 [details] [diff] [review]
> Define getBuiltinClass on Xray
> 
> So we figured out the that problem with Xray wrapper is that they don't
> handle getBuiltinClass at all. So in this case even Opaque wrappers get the
> default getBuiltinClass implementation, which is transparent. So an opaque
> wrapper for a boolean object, would claim to be a boolean. Changing this to
> ESClass::Other preserve the opaqueness of the wrapper in this case.

I can't land this at the moment, because of a new test failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=513caef0b6f6&selectedJob=24374729

dom/push/test/test_register_key.html | Wrong exception name for invalid key - got "InternalError", expected "InvalidAccessError"
InternalError: Promise rejection value is a non-unwrappable cross-compartment wrapper.

It's not super obvious to me why we would even have wrappers in this test case.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Note that the new test failure is a result of turning on spidermonkey promises.  They are a bit more restrictive in what they allow a promise to be rejected with.

> It's not super obvious to me why we would even have wrappers in this test case.

I'd guess because this test registers some chrome script to provide a fake API implementation and then tests itself against that, at least in part.  And because the normal API implementation is partially in chrome JS too.  Which means you now have chrome JS running (but maybe not the normal chrome JS that would implement this API).  My guess, though I have not tracked it down is that it's mixing together promises from untrusted code in dom/push/test/test_utils.js and promises from trusted code in dom/push/test/mockpushserviceparent.js into a single chain, then ending up with an exception from chrome code that propagates down that chain...

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d13c44c9b01
Define getBuiltinClass on Xray. r=peterv

Comment 60

[Tom S [:evilpie]]

Attachment: Return a fixed name for @@toString on plugins

This still returns undefined in all other cases, because the plugin code obviously isn't ready to actually handle symbol jsids. (The same probably goes for the Set/Delete variants)

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d13c44c9b01

Comment 62

[Tom S [:evilpie]]

Attachment: v4 - Implement @@toStringTag

Changes in this version:
- Added unwrapping to check for DOM classes in wrapped objects (This is required for various tests)
- Check for the existence of isProxy when testing toString of functions in test code, because the browser runner doesn't have it
- Fixed that blank line

Comment 63

[Tom S [:evilpie]]

Attachment: Remove ctypes class tests

I don't think it's necessary to setup @@toStringTag for ctypes.

Comment 64

[Tom S [:evilpie]]

Last try push looks okayish to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d533564eb8dd&selectedJob=26030928. Our orange factor is so high :/

Comment 65

[Tom S [:evilpie]]

Jason what's up? Sadly I think their might be some kind of GC/gray marking issue. https://treeherder.mozilla.org/#/jobs?repo=try&revision=efd6325d68fc&selectedJob=26519186

In mda: 
Assertion failure: IsInsideNursery(obj) || !obj->asTenured().isMarked(gc::GRAY), at /builds/slave/try-lx-d-000000000000000000000/build/src/js/src/jscntxtinlines.h:71

Comment 66

[Jason Orendorff [:jorendorff]]

Comment on attachment 8782985 [details] [diff] [review]
v4 - Implement @@toStringTag

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

Thank you! And I'm sorry for the slow reviews here. Good luck with GC issues. :-|

Would you mind making follow-up bugs are filed for module namespace objects and Promise.prototype?

Also, did you add this (from comment 50):
> So the behavior in this patch looks correct, but please add one last test, checking that
> you really *can* set an @@toStringTag property on a RegExp or something, and it does
> affect the Object.prototype.toString output.

? I didn't see it anywhere, but might have overlooked it.

::: js/src/builtin/Object.cpp
@@ +366,5 @@
> +            builtinTag = cx->names().objectRegExp;
> +            break;
> +          default:
> +            if (obj->isCallable()) {
> +                // Non-standard: Prevent <object> from showing up as Function.

*sympathy*

I don't even want to know why don't we need the same ugly hack in js::TypeOfObject (which is used by the `typeof` keyword).

::: js/src/builtin/TypedArray.js
@@ +1386,5 @@
> +
> +    // Steps 4-6.
> +    // Modified to retrieve the [[TypedArrayName]] from the constructor.
> +    var constructor = _ConstructorForTypedArray(O)
> +    return constructor.name;

Hmm. This is observably different from the standard, if you do `delete Uint8Array.name`. Why the difference? I suppose because we already have _ConstructorForTypedArray and it's a pain to add another thing?  :-P

::: js/src/jit-test/tests/debug/Object-class.js
@@ +12,4 @@
>      assertEq(arr[3].class, "Date");
>      assertEq(arr[4].class, "Object");
>      assertEq(arr[5].class, "Function");
> +    assertEq(arr[6].class, "Object");

ES6 is so weird.

::: js/src/tests/ecma_6/shell.js
@@ +99,5 @@
>              assertSameValue(ac, bc, msg);
>              switch (ac) {
>              case "[object Function]":
> +                if (typeof isProxy !== "undefined" && !isProxy(a) && !isProxy(b))
> +                    assertSameValue(Function_toString(a), Function_toString(b), msg);

Stricter:

    if (typeof isProxy !== "undefined") {
        assertEq(isProxy(a), isProxy(b), msg);
        if (!isProxy(a))
            assertSameValue(Function_toString(a), Function_toString(b), msg);
    }

Comment 67

[Jason Orendorff [:jorendorff]]

Comment on attachment 8782995 [details] [diff] [review]
Remove ctypes class tests

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

OK.

Comment 68

[Boris Zbarsky [:bzbarsky]]

> I don't even want to know why don't we need the same ugly hack in js::TypeOfObject

Because typeof returns "function" for these things.  It's just that they are not "[object Function]" in toString terms.

Comment 69

[Tom S [:evilpie]]

(In reply to Jason Orendorff [:jorendorff] from comment #66)
> Comment on attachment 8782985 [details] [diff] [review]
> v4 - Implement @@toStringTag
> 
> Review of attachment 8782985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you! And I'm sorry for the slow reviews here. Good luck with GC
> issues. :-|
> 
Seems like that is probably bug 1296275.
> Would you mind making follow-up bugs are filed for module namespace objects
> and Promise.prototype?
> 
> Also, did you add this (from comment 50):
> > So the behavior in this patch looks correct, but please add one last test, checking that
> > you really *can* set an @@toStringTag property on a RegExp or something, and it does
> > affect the Object.prototype.toString output.
> 
> ? I didn't see it anywhere, but might have overlooked it.
> 
Oh I thought I was testing this, but turns out I wasn't.
> ::: js/src/builtin/Object.cpp
> @@ +366,5 @@
> > +            builtinTag = cx->names().objectRegExp;
> > +            break;
> > +          default:
> > +            if (obj->isCallable()) {
> > +                // Non-standard: Prevent <object> from showing up as Function.
> 
> *sympathy*
> 
> I don't even want to know why don't we need the same ugly hack in
> js::TypeOfObject (which is used by the `typeof` keyword).
> 
> ::: js/src/builtin/TypedArray.js
> @@ +1386,5 @@
> > +
> > +    // Steps 4-6.
> > +    // Modified to retrieve the [[TypedArrayName]] from the constructor.
> > +    var constructor = _ConstructorForTypedArray(O)
> > +    return constructor.name;
> 
> Hmm. This is observably different from the standard, if you do `delete
> Uint8Array.name`. Why the difference? I suppose because we already have
> _ConstructorForTypedArray and it's a pain to add another thing?  :-P
> 
Right, let's fix that properly.
> ::: js/src/tests/ecma_6/shell.js
> @@ +99,5 @@
> >              assertSameValue(ac, bc, msg);
> >              switch (ac) {
> >              case "[object Function]":
> > +                if (typeof isProxy !== "undefined" && !isProxy(a) && !isProxy(b))
> > +                    assertSameValue(Function_toString(a), Function_toString(b), msg);
> 
> Stricter:
> 
>     if (typeof isProxy !== "undefined") {
>         assertEq(isProxy(a), isProxy(b), msg);
>         if (!isProxy(a))
>             assertSameValue(Function_toString(a), Function_toString(b), msg);
>     }
Ah nice idea, sadly this fails some tests :/

Comment 70

[Tom S [:evilpie]]

Attachment: v5 - Implement @@toStringTag

Comment 71

[Jason Orendorff [:jorendorff]]

Comment on attachment 8786134 [details] [diff] [review]
v5 - Implement @@toStringTag

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

::: js/src/vm/SelfHosting.cpp
@@ +1979,5 @@
> +
> +    RootedObject object(cx, &args[0].toObject());
> +    MOZ_ASSERT(object->is<TypedArrayObject>());
> +
> +    RootedString name(cx, NewStringCopyZ<CanGC>(cx, object->getClass()->name));

Atomize instead here, perhaps? This should save an allocation: since the typed array constructors themselves already exist and have names, the atoms should almost certainly exist.

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f382d56789de
Implement ES6 Symbol.toStringTag. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9eb93beee9
Return a fixed name for @@toString on plugins. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/b53afafbfb43
Remove DOM test known failure. r=Ms2ger
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c3d7a54cc16
Remove ctypes class tests. r=jorendorff

Comment 73

[Tom S [:evilpie]]

This might make bug 1296275 more likely to trigger, so please have a look before backing this out for that.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f382d56789de
https://hg.mozilla.org/mozilla-central/rev/2f9eb93beee9
https://hg.mozilla.org/mozilla-central/rev/b53afafbfb43
https://hg.mozilla.org/mozilla-central/rev/8c3d7a54cc16

Comment 75

[Alexandre Folle de Menezes]

Nice job, but today's nightly is still failing one of the tests ("Symbol.toStringTag, new built-ins") here:

http://kangax.github.io/compat-table/es6/

Comment 76

[Till Schneidereit [:till]]

(In reply to Alexandre Folle de Menezes from comment #75)
> Nice job, but today's nightly is still failing one of the tests
> ("Symbol.toStringTag, new built-ins") here:
> 
> http://kangax.github.io/compat-table/es6/

Thanks for the report! That's bug 1299321, so should be fixed in a few days.

Comment 77

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/51#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/toStringTag