Bug 168295 – Expose Symbol.toPrimitive / valueOf on Location instances

• Component: DOM
• Status: RESOLVED FIXED

[Anne van Kesteren]

Test: http://web-platform.test:8000/html/browsers/history/the-location-interface/location-symbol-toprimitive.html.

[Anne van Kesteren]

The same goes for valueOf: http://web-platform.test:8000/html/browsers/history/the-location-interface/location-valueof.html. (Let me know if you need a separate bug.)

[Chris Dumez]

To create a Location object, run these steps:

Let location be a new Location platform object.

Perform ! location.[[DefineOwnProperty]]("valueOf", { [[Value]]: %ObjProto_valueOf%, [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false }).

Perform ! location.[[DefineOwnProperty]](@@toPrimitive, { [[Value]]: undefined, [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false }).

Set the value of the [[DefaultProperties]] internal slot of location to location.[[OwnPropertyKeys]]().

Return location.

[Chris Dumez]

Will take a look soon.

[Chris Dumez]

Created attachment 301516 [details]
Patch

[Darin Adler]

Comment on attachment 301516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301516&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3430
> +        push(@implContent, "    auto objProtoValueOf = globalObject()->objectPrototype()->getDirect(vm, vm.propertyNames->valueOf);\n");

Please just spell out "object" instead of using "obj".

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3432
> +        push(@implContent, "    putDirect(vm, vm.propertyNames->valueOf, objProtoValueOf, DontDelete | ReadOnly | DontEnum);\n");

Is this correct if the object prototype valueOf property’s value changes after creating the Location interface? Do we have test cases covered?

[Geoffrey Garen]

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3431
> +        push(@implContent, "    auto objProtoValueOf = globalObject()->objectPrototype()->getDirect(vm, vm.propertyNames->valueOf);\n");
> +        push(@implContent, "    ASSERT(objProtoValueOf);\n");

It's possible to change or delete this property, so I don't think this is correct.

In cases where we might later need the original value of a certain property, we provide an accessor on JSGlobalObject, such as:

    // The following accessors return pristine values, even if a script 
    // replaces the global object's associated property.

    GetterSetter* speciesGetterSetter() const { return m_speciesGetterSetter.get(); }

    RegExpConstructor* regExpConstructor() const { return m_regExpConstructor.get(); }

    ErrorConstructor* errorConstructor() const { return m_errorConstructor.get(); }
    ArrayConstructor* arrayConstructor() const { return m_arrayConstructor.get(); }
    ObjectConstructor* objectConstructor() const { return m_objectConstructor.get(); }
    JSPromiseConstructor* promiseConstructor() const { return m_promiseConstructor.get(); }
    JSInternalPromiseConstructor* internalPromiseConstructor() const { return m_internalPromiseConstructor.get(); }
    NativeErrorConstructor* evalErrorConstructor() const { return m_evalErrorConstructor.get(this); }
    NativeErrorConstructor* rangeErrorConstructor() const { return m_rangeErrorConstructor.get(); }
    NativeErrorConstructor* referenceErrorConstructor() const { return m_referenceErrorConstructor.get(this); }
    NativeErrorConstructor* syntaxErrorConstructor() const { return m_syntaxErrorConstructor.get(this); }
    NativeErrorConstructor* typeErrorConstructor() const { return m_typeErrorConstructor.get(); }
    NativeErrorConstructor* URIErrorConstructor() const { return m_URIErrorConstructor.get(this); }

    NullGetterFunction* nullGetterFunction() const { return m_nullGetterFunction.get(); }
    NullSetterFunction* nullSetterFunction() const { return m_nullSetterFunction.get(); }

    JSFunction* parseIntFunction() const { return m_parseIntFunction.get(); }

    JSFunction* evalFunction() const { return m_evalFunction.get(); }
    JSFunction* callFunction() const { return m_callFunction.get(); }
    JSFunction* applyFunction() const { return m_applyFunction.get(); }
    JSFunction* throwTypeErrorFunction() const { return m_throwTypeErrorFunction.get(); }
    JSFunction* arrayProtoToStringFunction() const { return m_arrayProtoToStringFunction.get(this); }
    JSFunction* arrayProtoValuesFunction() const { return m_arrayProtoValuesFunction.get(this); }
    JSFunction* initializePromiseFunction() const { return m_initializePromiseFunction.get(this); }
    JSFunction* iteratorProtocolFunction() const { return m_iteratorProtocolFunction.get(this); }
    JSFunction* newPromiseCapabilityFunction() const { return m_newPromiseCapabilityFunction.get(); }
    JSFunction* functionProtoHasInstanceSymbolFunction() const { return m_functionProtoHasInstanceSymbolFunction.get(); }
    JSObject* regExpProtoExecFunction() const { return m_regExpProtoExec.get(); }
    JSObject* regExpProtoSymbolReplaceFunction() const { return m_regExpProtoSymbolReplace.get(); }
    JSObject* regExpProtoGlobalGetter() const { return m_regExpProtoGlobalGetter.get(); }
    JSObject* regExpProtoUnicodeGetter() const { return m_regExpProtoUnicodeGetter.get(); }

[Chris Dumez]

(In reply to comment #6)
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3431
> > +        push(@implContent, "    auto objProtoValueOf = globalObject()->objectPrototype()->getDirect(vm, vm.propertyNames->valueOf);\n");
> > +        push(@implContent, "    ASSERT(objProtoValueOf);\n");
> 
> It's possible to change or delete this property, so I don't think this is
> correct.
> 
> In cases where we might later need the original value of a certain property,
> we provide an accessor on JSGlobalObject, such as:
> 
>     // The following accessors return pristine values, even if a script 
>     // replaces the global object's associated property.
> 
>     GetterSetter* speciesGetterSetter() const { return
> m_speciesGetterSetter.get(); }
> 
>     RegExpConstructor* regExpConstructor() const { return
> m_regExpConstructor.get(); }
> 
>     ErrorConstructor* errorConstructor() const { return
> m_errorConstructor.get(); }
>     ArrayConstructor* arrayConstructor() const { return
> m_arrayConstructor.get(); }
>     ObjectConstructor* objectConstructor() const { return
> m_objectConstructor.get(); }
>     JSPromiseConstructor* promiseConstructor() const { return
> m_promiseConstructor.get(); }
>     JSInternalPromiseConstructor* internalPromiseConstructor() const {
> return m_internalPromiseConstructor.get(); }
>     NativeErrorConstructor* evalErrorConstructor() const { return
> m_evalErrorConstructor.get(this); }
>     NativeErrorConstructor* rangeErrorConstructor() const { return
> m_rangeErrorConstructor.get(); }
>     NativeErrorConstructor* referenceErrorConstructor() const { return
> m_referenceErrorConstructor.get(this); }
>     NativeErrorConstructor* syntaxErrorConstructor() const { return
> m_syntaxErrorConstructor.get(this); }
>     NativeErrorConstructor* typeErrorConstructor() const { return
> m_typeErrorConstructor.get(); }
>     NativeErrorConstructor* URIErrorConstructor() const { return
> m_URIErrorConstructor.get(this); }
> 
>     NullGetterFunction* nullGetterFunction() const { return
> m_nullGetterFunction.get(); }
>     NullSetterFunction* nullSetterFunction() const { return
> m_nullSetterFunction.get(); }
> 
>     JSFunction* parseIntFunction() const { return m_parseIntFunction.get(); }
> 
>     JSFunction* evalFunction() const { return m_evalFunction.get(); }
>     JSFunction* callFunction() const { return m_callFunction.get(); }
>     JSFunction* applyFunction() const { return m_applyFunction.get(); }
>     JSFunction* throwTypeErrorFunction() const { return
> m_throwTypeErrorFunction.get(); }
>     JSFunction* arrayProtoToStringFunction() const { return
> m_arrayProtoToStringFunction.get(this); }
>     JSFunction* arrayProtoValuesFunction() const { return
> m_arrayProtoValuesFunction.get(this); }
>     JSFunction* initializePromiseFunction() const { return
> m_initializePromiseFunction.get(this); }
>     JSFunction* iteratorProtocolFunction() const { return
> m_iteratorProtocolFunction.get(this); }
>     JSFunction* newPromiseCapabilityFunction() const { return
> m_newPromiseCapabilityFunction.get(); }
>     JSFunction* functionProtoHasInstanceSymbolFunction() const { return
> m_functionProtoHasInstanceSymbolFunction.get(); }
>     JSObject* regExpProtoExecFunction() const { return
> m_regExpProtoExec.get(); }
>     JSObject* regExpProtoSymbolReplaceFunction() const { return
> m_regExpProtoSymbolReplace.get(); }
>     JSObject* regExpProtoGlobalGetter() const { return
> m_regExpProtoGlobalGetter.get(); }
>     JSObject* regExpProtoUnicodeGetter() const { return
> m_regExpProtoUnicodeGetter.get(); }

Thanks, I'll update.

[Darin Adler]

(In reply to comment #6)
> It's possible to change or delete this property, so I don't think this is
> correct.

We should verify this with test cases: one that changes and another that deletes the property.

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #6)
> > It's possible to change or delete this property, so I don't think this is
> > correct.
> 
> We should verify this with test cases: one that changes and another that
> deletes the property.

Will do.

[Chris Dumez]

Created attachment 301539 [details]
Patch

[Chris Dumez]

Take #2.

[Geoffrey Garen]

Comment on attachment 301539 [details]
Patch

r=me

[Keith Miller]

Comment on attachment 301539 [details]
Patch

r=me too.

[Mark Lam]

Comment on attachment 301539 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301539&action=review

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:461
> +    m_objectProtoValueOfFunction.set(vm, this, jsCast<JSFunction*>(objectPrototype()->getDirect(vm, vm.propertyNames->valueOf)));

You also need to visit m_objectProtoValueOfFunction in JSGlobalObject::visitChildren().

[Chris Dumez]

Created attachment 301543 [details]
Patch

[Chris Dumez]

(In reply to comment #14)
> Comment on attachment 301539 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301539&action=review
> 
> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:461
> > +    m_objectProtoValueOfFunction.set(vm, this, jsCast<JSFunction*>(objectPrototype()->getDirect(vm, vm.propertyNames->valueOf)));
> 
> You also need to visit m_objectProtoValueOfFunction in
> JSGlobalObject::visitChildren().

Sorry, I completely forgot to do this. This is fixed in the latest iteration.

[Chris Dumez]

Created attachment 301544 [details]
Patch

[Darin Adler]

Comment on attachment 301544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301544&action=review

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:461
> +    m_objectProtoValueOfFunction.set(vm, this, jsCast<JSFunction*>(objectPrototype()->getDirect(vm, vm.propertyNames->valueOf)));

It would be nicer to find a way to do this without the cast; there must be code that originally allocated this that knows it’s a JSFunction.

[Chris Dumez]

Comment on attachment 301544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301544&action=review

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:461
>> +    m_objectProtoValueOfFunction.set(vm, this, jsCast<JSFunction*>(objectPrototype()->getDirect(vm, vm.propertyNames->valueOf)));
> 
> It would be nicer to find a way to do this without the cast; there must be code that originally allocated this that knows it’s a JSFunction.

I am not sure how to do that. We use putDirectNativeFunctionWithoutTransition() to create the JSFunction from the NativeFunction and put in on the object for us.
But then, on getting via get/getDirect, we get a JSValue (Also note that I do not want to create a new JSFunction because I need to preserve identity between Object.prototype.valueOf and location.valueOf).

[Darin Adler]

Comment on attachment 301544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301544&action=review

>>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:461
>>> +    m_objectProtoValueOfFunction.set(vm, this, jsCast<JSFunction*>(objectPrototype()->getDirect(vm, vm.propertyNames->valueOf)));
>> 
>> It would be nicer to find a way to do this without the cast; there must be code that originally allocated this that knows it’s a JSFunction.
> 
> I am not sure how to do that. We use putDirectNativeFunctionWithoutTransition() to create the JSFunction from the NativeFunction and put in on the object for us.
> But then, on getting via get/getDirect, we get a JSValue (Also note that I do not want to create a new JSFunction because I need to preserve identity between Object.prototype.valueOf and location.valueOf).

One idea: Could add a JSFunction* return value to putDirectNativeFunctionWithoutTransition, ignored by other callers, but used in this case.

[Chris Dumez]

(In reply to comment #20)
> Comment on attachment 301544 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301544&action=review
> 
> >>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:461
> >>> +    m_objectProtoValueOfFunction.set(vm, this, jsCast<JSFunction*>(objectPrototype()->getDirect(vm, vm.propertyNames->valueOf)));
> >> 
> >> It would be nicer to find a way to do this without the cast; there must be code that originally allocated this that knows it’s a JSFunction.
> > 
> > I am not sure how to do that. We use putDirectNativeFunctionWithoutTransition() to create the JSFunction from the NativeFunction and put in on the object for us.
> > But then, on getting via get/getDirect, we get a JSValue (Also note that I do not want to create a new JSFunction because I need to preserve identity between Object.prototype.valueOf and location.valueOf).
> 
> One idea: Could add a JSFunction* return value to
> putDirectNativeFunctionWithoutTransition, ignored by other callers, but used
> in this case.

putDirectNativeFunctionWithoutTransition() is called inside ObjectPrototype::finishCreation(). I need access to it in the JSGlobalObject after the ObjectPrototype has been constructed.

[Chris Dumez]

(In reply to comment #21)
> (In reply to comment #20)
> > Comment on attachment 301544 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=301544&action=review
> > 
> > >>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:461
> > >>> +    m_objectProtoValueOfFunction.set(vm, this, jsCast<JSFunction*>(objectPrototype()->getDirect(vm, vm.propertyNames->valueOf)));
> > >> 
> > >> It would be nicer to find a way to do this without the cast; there must be code that originally allocated this that knows it’s a JSFunction.
> > > 
> > > I am not sure how to do that. We use putDirectNativeFunctionWithoutTransition() to create the JSFunction from the NativeFunction and put in on the object for us.
> > > But then, on getting via get/getDirect, we get a JSValue (Also note that I do not want to create a new JSFunction because I need to preserve identity between Object.prototype.valueOf and location.valueOf).
> > 
> > One idea: Could add a JSFunction* return value to
> > putDirectNativeFunctionWithoutTransition, ignored by other callers, but used
> > in this case.
> 
> putDirectNativeFunctionWithoutTransition() is called inside
> ObjectPrototype::finishCreation(). I need access to it in the JSGlobalObject
> after the ObjectPrototype has been constructed.

c.f.
void ObjectPrototype::finishCreation(VM& vm, JSGlobalObject* globalObject)
{
    Base::finishCreation(vm);
    ASSERT(inherits(vm, info()));
    // ...
    JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->valueOf, objectProtoFuncValueOf, DontEnum, 0);
    // ...
}

[Mark Lam]

Comment on attachment 301544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301544&action=review

>>>>>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:461
>>>>>> +    m_objectProtoValueOfFunction.set(vm, this, jsCast<JSFunction*>(objectPrototype()->getDirect(vm, vm.propertyNames->valueOf)));
>>>>> 
>>>>> It would be nicer to find a way to do this without the cast; there must be code that originally allocated this that knows it’s a JSFunction.
>>>> 
>>>> I am not sure how to do that. We use putDirectNativeFunctionWithoutTransition() to create the JSFunction from the NativeFunction and put in on the object for us.
>>>> But then, on getting via get/getDirect, we get a JSValue (Also note that I do not want to create a new JSFunction because I need to preserve identity between Object.prototype.valueOf and location.valueOf).
>>> 
>>> One idea: Could add a JSFunction* return value to putDirectNativeFunctionWithoutTransition, ignored by other callers, but used in this case.
>> 
>> putDirectNativeFunctionWithoutTransition() is called inside ObjectPrototype::finishCreation(). I need access to it in the JSGlobalObject after the ObjectPrototype has been constructed.
> 
> c.f.
> void ObjectPrototype::finishCreation(VM& vm, JSGlobalObject* globalObject)
> {
>     Base::finishCreation(vm);
>     ASSERT(inherits(vm, info()));
>     // ...
>     JSC_NATIVE_FUNCTION_WITHOUT_TRANSITION(vm.propertyNames->valueOf, objectProtoFuncValueOf, DontEnum, 0);
>     // ...
> }

Some details to consider:
1. JSGlobalObject::init() (this function) doesn't itself call putDirectNativeFunctionWithoutTransition() to create the Function that it is trying to cache.
2. JSGlobalObject::init() instantiates m_objectPrototype above.  It's the ObjectPrototype factory that creates the function.
3. It would be strange for ObjectPrototype::finishCreation() to cache the function in the globalObject.  This creates a tighter coupling between ObjectPrototype and JSGlobalObject.

For these reasons, I think the way Chris implemented it is just fine.  I suggest we move forward with this patch.

[WebKit Commit Bot]

Comment on attachment 301544 [details]
Patch

Rejecting attachment 301544 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 301544, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ckout/scm/scm.py", line 77, in run
    decode_output=decode_output)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/system/executive.py", line 443, in run_command
    output = output.decode(self._child_process_encoding())
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/encodings/utf_8.py", line 16, in decode
    return codecs.utf_8_decode(input, errors, True)
UnicodeDecodeError: 'utf8' codec can't decode byte 0xad in position 142: invalid start byte

Full output: http://webkit-queues.webkit.org/results/3122689

[Chris Dumez]

Comment on attachment 301544 [details]
Patch

Clearing flags on attachment: 301544

Committed r212378: <http://trac.webkit.org/changeset/212378>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.