Bug 172687 – Can't use Object.defineProperty() to add an item to a DOMStringMap or Storage

• Component: Bindings
• Status: RESOLVED FIXED

[Sam Weinig]

Created attachment 311447 [details]
Test case

Using Object.defineProperty() to add a item to a DOMStringMap does not seem to work. Given an dataset create like so:

    let element = document.createElement("div");
    element.dataset["a"] = "1";
    element.dataset["b"] = "2";

If you try to add an additional item like so:

    Object.defineProperty(element.dataset, "key", {
        value: 'value'
    });

The element remains unchanged (This is shown in the test case by printing out element.outerHTML).

[Sam Weinig]

I believe what we need to do is override the 'defineOwnProperty' static function, in addition to 'put' and 'putByIndex'.

I'd be interested in going over all the overrides we do at some point, as I am not 100% what the ramifications of not overriding some (specifically getOwnNonIndexPropertyNames, getPropertyNames, getEnumerableLength, getStructurePropertyNames, getGenericPropertyNames, preventExtensions, and isExtensible) are with respect to the WebIDL specification of 'legacy platform objects' (https://heycam.github.io/webidl/#es-legacy-platform-objects)

[Saam Barati]

(In reply to Sam Weinig from comment #1)
> I believe what we need to do is override the 'defineOwnProperty' static
> function, in addition to 'put' and 'putByIndex'.
> 
> I'd be interested in going over all the overrides we do at some point, as I
> am not 100% what the ramifications of not overriding some (specifically
> getOwnNonIndexPropertyNames, getPropertyNames, getEnumerableLength,
> getStructurePropertyNames, getGenericPropertyNames, preventExtensions, and
> isExtensible) are with respect to the WebIDL specification of 'legacy
> platform objects'
> (https://heycam.github.io/webidl/#es-legacy-platform-objects)

Sam, what does the element derive from here? (I think JSDOMStringMap is a derived source and I don't have a build handy.) Anyways, it seems like it's probably easy to implement the "legacy platform object" semantics you linked to. It is a bit surprising that the element is totally unchanged here.

[Saam Barati]

Ok, my build finished. So I think this is probably as simple as implanting the specced behavior. It is weird though that we're not seeing the defineOwnProperty. Perhaps it has to do with how getOwnPropertySlot is written:

```
bool JSDOMStringMap::getOwnPropertySlot(JSObject* object, ExecState* state, PropertyName propertyName, PropertySlot& slot)
{
    auto* thisObject = jsCast<JSDOMStringMap*>(object);
    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
    if (thisObject->classInfo() == info() && !propertyName.isSymbol()) {
        auto item = thisObject->wrapped().namedItem(propertyNameToAtomicString(propertyName));
        if (!IDLDOMString::isNullValue(item)) {
            slot.setValue(thisObject, 0, toJS<IDLDOMString>(*state, item));
            return true;
        }
    }
    if (Base::getOwnPropertySlot(thisObject, state, propertyName, slot))
        return true;
    return false;
}
```

[Sam Weinig]

Yeah, I'm just going to implement the defineOwnProperty hook. One issue though is that as specified, I need to modify the PropertyDescriptor (see step 3 of https://heycam.github.io/webidl/#legacy-platform-object-defineownproperty) but the hook passes it as a const reference. Seems relatively straightforward to change this in JSC, but I am not clear if that was done for a reason.

[Saam Barati]

(In reply to Sam Weinig from comment #4)
> Yeah, I'm just going to implement the defineOwnProperty hook. One issue
> though is that as specified, I need to modify the PropertyDescriptor (see
> step 3 of
> https://heycam.github.io/webidl/#legacy-platform-object-defineownproperty)
> but the hook passes it as a const reference. Seems relatively
> straightforward to change this in JSC, but I am not clear if that was done
> for a reason.

I'm not sure why JSC passes it as const. However, why not just create a new one? I think that should be pretty cheap.

[Sam Weinig]

(In reply to Saam Barati from comment #5)
> (In reply to Sam Weinig from comment #4)
> > Yeah, I'm just going to implement the defineOwnProperty hook. One issue
> > though is that as specified, I need to modify the PropertyDescriptor (see
> > step 3 of
> > https://heycam.github.io/webidl/#legacy-platform-object-defineownproperty)
> > but the hook passes it as a const reference. Seems relatively
> > straightforward to change this in JSC, but I am not clear if that was done
> > for a reason.
> 
> I'm not sure why JSC passes it as const. However, why not just create a new
> one? I think that should be pretty cheap.

Oh duh.

[Sam Weinig]

Created attachment 311953 [details]
Patch

[Sam Weinig]

Created attachment 311961 [details]
Patch

[Sam Weinig]

Committed r217773: <http://trac.webkit.org/changeset/217773>

[Saam Barati]

Comment on attachment 311961 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:483
> +        push(@$outputArray, "    JSValue proto = thisObject->getPrototypeDirect();\n");
> +        push(@$outputArray, "    if (proto.isObject() && jsCast<JSObject*>(proto)->hasProperty(state, propertyName))\n");
> +        push(@$outputArray, "        return false;\n\n");

Is it safe to use getPrototypeDirect here? Do we not expect that this is generated for something that has a custom getPrototype hook? Would that even matter?

Also, don't we need an exception check after hasProperty?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:763
> +            push(@$outputArray, "    PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry };\n");

VMInquiry looks wrong here. I think you want something else, but it depends on which operation you're trying to perform.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:764
> +            push(@$outputArray, "    JSValue prototype = thisObject->getPrototypeDirect();\n");

ditto about safety of using getPrototypeDirect here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:887
> +            # FIXME: Is Base::getOwnPropertySlot the right function to call? Is there a function that will
> +            #        only look at the actual properties, and not call into our implementation of the
> +            #        [[GetOwnProperty]] hook?

This seems correct to me, but it probably depends on what Base is.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:904
> +            # 2.2.1. If the result of calling IsDataDescriptor(Desc) is false, then return false.
> +            push(@$outputArray, $additionalIndent . "        if (!propertyDescriptor.isDataDescriptor())\n");
> +            push(@$outputArray, $additionalIndent . "            return false;\n");
> +            
> +            # 2.2.2. Invoke the named property setter with P and Desc.[[Value]].
> +            GenerateInvokeNamedPropertySetter($outputArray, $additionalIndent . "        ", $interface, $namedSetterOperation, "propertyDescriptor.value()");

These specced semantics are bizarre. I'm sure there is some reason they are the way they are, but it's counter-intuitive that defineOwnProperty can mean "call this setter instead".

[Saam Barati]

Comment on attachment 311961 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:634
> +    push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, true);\n");

nit: Should not matter in practice, but I'd return false in case of an exception.

[Alexey Shvayka]

(In reply to Saam Barati from comment #11)
> Comment on attachment 311961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311961&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:634
> > +    push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, true);\n");
> 
> nit: Should not matter in practice, but I'd return false in case of an
> exception.

Saam, I've replied to the post-review feedback in https://bugs.webkit.org/show_bug.cgi?id=218849#c8.

Would also appreciate your insight on slot caching in that patch: while [[Delete]] tests are a bit flaky without DeletePropertySlot::disableCaching(), get() and put() seem to work fine w/o it.