Closed Bug 1114580 Opened 9 years ago Closed 8 years ago

Implement ES6 Symbol.toStringTag

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51

People

(Reporter: evilpie, Assigned: evilpie)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(6 files, 9 obsolete files)

535 bytes, patch
Ms2ger
: review+
Details | Diff | Splinter Review
53.81 KB, patch
jorendorff
: review+
evilpie
: checkin+
Details | Diff | Splinter Review
4.05 KB, patch
peterv
: review+
evilpie
: checkin+
Details | Diff | Splinter Review
1.14 KB, patch
billm
: review+
Details | Diff | Splinter Review
5.03 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
33.64 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
      No description provided.
Blocks: 1055473
If someone can mentor me, I'd like to try to implement this. :)
Blocks: 1119779
No longer blocks: es6
Still interested? I can mentor.
Flags: needinfo?(446240525)
Component: JavaScript Engine → JavaScript: Standard Library
Assignee: nobody → 446240525
Flags: needinfo?(446240525)
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.
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.
Note that for cross-origin windows we'd want @@toStringTag to return undefined, not throw.
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 ?
Attached patch toStringTag.basic.patch (obsolete) — Splinter Review
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.
Attachment #8658996 - Flags: review+
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
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.
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.
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.
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? :)
Flags: needinfo?(georgethomas.mec)
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/
Flags: needinfo?(jorendorff)
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.
(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.
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky)
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.
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.
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
(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
Flags: needinfo?(bobbyholley) → needinfo?(jorendorff)
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.
(a link to the es-discuss thread mentioned above would be great, since I can't seem to find it in the archives)
The es discuss thread was iirc span from public-script-coord https://lists.w3.org/Archives/Public/public-script-coord/2015JulSep/0022.html
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.
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.
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!
Flags: needinfo?(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
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.
Flags: needinfo?(georgethomas.mec)
Attached patch toStringTag.patch (obsolete) — Splinter Review
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
Attachment #8658996 - Attachment is obsolete: true
Attachment #8660866 - Attachment is obsolete: true
Flags: needinfo?(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.
Attachment #8683370 - Flags: feedback+
Flags: needinfo?(jorendorff)
Assignee: 446240525 → evilpies
Attached file WIP (obsolete) —
Blocks: 1277799
Blocks: 1277801
Attachment #8683370 - Attachment is obsolete: true
Attached patch v1 - Implement @@toStringTag (obsolete) — Splinter Review
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
Attachment #8758689 - Attachment is obsolete: true
I tried finding time to complete it but unfortunately could not find time to get it done. Good to see the progress with it :)
Those CGC failures on try look suspicious and need to be investigated after I manage to reproduce them locally.
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.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(aklotz)
Anything I touched in there was relating to async plugin init, so I don't really have anything to add on this topic.
Flags: needinfo?(aklotz)
I can't imagine plugins care about symbol jsids. Please just do whatever is easiest.
Flags: needinfo?(wmccloskey)
Attached patch v2 - Implement @@toStringTag (obsolete) — Splinter Review
This seems to pass try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8f32de68010&selectedJob=22247482.

Same caveat as before, no Promise support.
Attachment #8761761 - Flags: review?(jorendorff)
Attachment #8759626 - Attachment is obsolete: true
Attachment #8761762 - Flags: review?(Ms2ger)
Attachment #8761763 - Flags: review?(wmccloskey)
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.
Attachment #8761762 - Flags: review?(Ms2ger) → review+
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 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).
Attachment #8761761 - Flags: review?(jorendorff) → review+
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.
Attachment #8761763 - Flags: review?(wmccloskey)
Changes ESClassValue to ESClass and makes it a proper enum class. Also adds Arguments/Error, which I need for Object.prototype.toString soon.
Attachment #8766007 - Flags: review?(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.
Attachment #8766007 - Flags: review?(jorendorff) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3e76c6750c
Change ESClassValue to an enum class. r=jorendorff
Keywords: leave-open
Attached patch v3 - Implement @@toStringTag (obsolete) — Splinter Review
I am going to fix the TypedArray getter in a new bug.
Attachment #8761761 - Attachment is obsolete: true
Attachment #8766900 - Flags: review?(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...
Attachment #8766900 - Flags: review?(jorendorff) → review+
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?
> 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".
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.
> Anyway I am not sure what is going on in xray test. 

Have you tried just stepping through that stringification attempt?
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.
Attachment #8770961 - Flags: review?(peterv)
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.
Attachment #8770961 - Flags: review?(peterv) → review+
(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.
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...
Depends on: 1288903
Attachment #8766007 - Flags: checkin+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d13c44c9b01
Define getBuiltinClass on Xray. r=peterv
Attachment #8770961 - Flags: checkin+
Attachment #8761763 - Attachment is obsolete: true
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)
Attachment #8782631 - Flags: review?(wmccloskey)
Attached patch v4 - Implement @@toStringTag (obsolete) — Splinter Review
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
Attachment #8766900 - Attachment is obsolete: true
Attachment #8782985 - Flags: review?(jorendorff)
I don't think it's necessary to setup @@toStringTag for ctypes.
Attachment #8782995 - Flags: review?(jorendorff)
Last try push looks okayish to me: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d533564eb8dd&selectedJob=26030928. Our orange factor is so high :/
Attachment #8782631 - Flags: review?(wmccloskey) → review+
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 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);
    }
Attachment #8782985 - Flags: review?(jorendorff) → review+
Comment on attachment 8782995 [details] [diff] [review]
Remove ctypes class tests

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

OK.
Attachment #8782995 - Flags: review?(jorendorff) → review+
> 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.
(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 :/
Attachment #8786134 - Flags: review?(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.
Attachment #8786134 - Flags: review?(jorendorff) → review+
Pushed by evilpies@gmail.com:
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
This might make bug 1296275 more likely to trigger, so please have a look before backing this out for that.
Attachment #8782985 - Attachment is obsolete: true
Blocks: 1299321
Blocks: 1299323
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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/
(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.
Target Milestone: --- → mozilla51
Depends on: 1303714
Depends on: 1321299
You need to log in before you can comment on or make changes to this bug.