Bug 153920 – Attributes on the Window instance should be configurable unless [Unforgeable]

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Attributes on the Window instance should be configurable unless [Unforgeable]:
1. 'constructor' property: http://www.w3.org/TR/WebIDL/#interface-prototype-object
2. Constructor properties (e.g. window.Node): http://www.w3.org/TR/WebIDL/#es-interfaces
3. IDL attributes: http://heycam.github.io/webidl/#es-attributes (configurable unless [Unforgeable], e.g. window.location)

Firefox complies with the WebIDL specification but WebKit does not for 1. and 3.

[Chris Dumez]

Created attachment 270750 [details]
WIP Patch

[Chris Dumez]

Created attachment 270828 [details]
WIP Patch

[Chris Dumez]

Created attachment 270832 [details]
Patch

[Darin Adler]

Comment on attachment 270832 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:1676
>          for (auto iter = hashTable->begin(); iter != hashTable->end(); ++iter) {

Modern for loop would be nicer here.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:205
> +        const HashTableValue* entry = JSDOMWindow::info()->staticPropHashTable->entry(propertyName);

auto* would be better, and I’d put the definition inside the if

[Gavin Barraclough]

Comment on attachment 270832 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:210
>      }

Per conversation, looks like you've changed behavior for (thisObject->staticFunctionsReified() && !allowsAccess) – seems unlikely that this right. Some properties that were previously accessible (with altered attributes) will now be inaccessible.

> LayoutTests/js/getOwnPropertyDescriptor-window-attributes.html:61
> +shouldBeTrue("descriptor.configurable");

Should we also be checking descriptor.value?

> LayoutTests/js/getOwnPropertyDescriptor-window-attributes.html:69
> +shouldBeTrue("descriptor.configurable");

Should we also be checking descriptor.value?

[Chris Dumez]

Comment on attachment 270832 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSObject.cpp:1676
>>          for (auto iter = hashTable->begin(); iter != hashTable->end(); ++iter) {
> 
> Modern for loop would be nicer here.

I will need to tweak HashTable::ConstIterator but OK.

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:205
>> +        const HashTableValue* entry = JSDOMWindow::info()->staticPropHashTable->entry(propertyName);
> 
> auto* would be better, and I’d put the definition inside the if

OK.

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:210
>>      }
> 
> Per conversation, looks like you've changed behavior for (thisObject->staticFunctionsReified() && !allowsAccess) – seems unlikely that this right. Some properties that were previously accessible (with altered attributes) will now be inaccessible.

Good catch. I have checked the behavior of Firefox and Chrome and it seems they keep using the original property getter for cross-origin access, even if that property was deleted / redefined.
I propose we do the same.

I will also add test coverage for this.

>> LayoutTests/js/getOwnPropertyDescriptor-window-attributes.html:61
>> +shouldBeTrue("descriptor.configurable");
> 
> Should we also be checking descriptor.value?

Will do.

>> LayoutTests/js/getOwnPropertyDescriptor-window-attributes.html:69
>> +shouldBeTrue("descriptor.configurable");
> 
> Should we also be checking descriptor.value?

Will do.

[Chris Dumez]

Created attachment 270910 [details]
Patch

[Chris Dumez]

Created attachment 270912 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/24563211>

[Gavin Barraclough]

> Good catch. I have checked the behavior of Firefox and Chrome and it seems
> they keep using the original property getter for cross-origin access, even
> if that property was deleted / redefined.
> I propose we do the same.

Oh, that makes a lot of sense. As a future refactoring, it's probably cleanest (& therefore less error prone) to just make DOMWindow's getOwnPropertySlot do the cross origin check at the head of the function, separate from the set of real properties for when access is allowed.

    if (!allowAccess) {
        // return one of a few hard-coded permitted properties,
        // else fail.
    }
    // real properties for access allowed.

In fact, splitting it that way would probably make it clean to return different accessors in the cross-origin cases. If we did it right we can probably do away with dynamic security checks in the accessors for cacheable cases – I'm guessing that we can prove that anywhere we would return the real (access allowed) accessor, it would be safe to cache & never perform a security check, since permission is static.

[Gavin Barraclough]

Comment on attachment 270912 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:208
> +            return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);

Oh! – I just realized another possible case to check. What if there exists a item on the page that would be accessible via a named property, but that is shadowed by a configurable property in the static table, and that static property is deleted?

What I think should happen is, if you try to access that property before deletion you'll get the property from the static table, and after deletion you should get the named property. What I think will happen with your code as-is, is that before deletion you'll correctly get the property from the static table, but after deletion you'll return early here – returning property not found before the named lookup has a chance to run.

If I'm right about there being a problem, then I think a fix would be to invert the if:

    if (!thisObject->staticFunctionsReified() || !allowsAccess) {
        slot.setCacheableCustom(thisObject, allowsAccess ? entry->attributes() : ReadOnly | DontDelete | DontEnum, entry->propertyGetter());
        return true;
    }

(This would all also be easier when we switch to be more spec-compliant & move the named properties onto their own NPO object in the photo chain!)

[Chris Dumez]

Created attachment 270930 [details]
Patch

[Chris Dumez]

Created attachment 270932 [details]
Patch

[Chris Dumez]

(In reply to comment #11)
> Comment on attachment 270912 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270912&action=review
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:208
> > +            return Base::getOwnPropertySlot(thisObject, exec, propertyName, slot);
> 
> Oh! – I just realized another possible case to check. What if there exists a
> item on the page that would be accessible via a named property, but that is
> shadowed by a configurable property in the static table, and that static
> property is deleted?
> 
> What I think should happen is, if you try to access that property before
> deletion you'll get the property from the static table, and after deletion
> you should get the named property. What I think will happen with your code
> as-is, is that before deletion you'll correctly get the property from the
> static table, but after deletion you'll return early here – returning
> property not found before the named lookup has a chance to run.
> 
> If I'm right about there being a problem, then I think a fix would be to
> invert the if:
> 
>     if (!thisObject->staticFunctionsReified() || !allowsAccess) {
>         slot.setCacheableCustom(thisObject, allowsAccess ?
> entry->attributes() : ReadOnly | DontDelete | DontEnum,
> entry->propertyGetter());
>         return true;
>     }
> 
> (This would all also be easier when we switch to be more spec-compliant &
> move the named properties onto their own NPO object in the photo chain!)

Yes, you are right that it did not behave as expected in this case. I have added test coverage for this and made the fix.

[Chris Dumez]

Created attachment 270935 [details]
Patch

[Darin Adler]

Comment on attachment 270935 [details]
Patch

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

> Source/JavaScriptCore/runtime/Lookup.h:138
> +        const HashTableValue& operator*() { return *value(); }

I think this should be a const member, as should value(), key(), and operator->().

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:205
> +    // When accessing cross-origin known Window properties, we always use the original property getter,
> +    // even if the property was removed / redefined. This matches Firefox and Chrome's behavior.

These comments about matching other browsers often are slightly confusing and a liability long term. If you feel the need to state this in terms of the other browsers’  behavior because no standard makes this clear, then I think you should say "as of early 2016" instead of making it sound timeless.

[Chris Dumez]

Committed r196374: <http://trac.webkit.org/changeset/196374>