Bug 154376 – [ES6] Come up with a test for Proxy.[[GetOwnProperty]] that tests the isExtensible error when the result of the trap is undefined

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

We want code that tests this part of the function:
if (!target->isExtensible()) {
            // Object.seal(o) will make all fields [[Configurable]] false so I'm not sure how to test this
            throwVMTypeError(exec, ASCIILiteral("When 'getOwnPropertyDescriptor' returns undefined, the 'target' of a Proxy should be extensible."));
            return
}

[Alexey Shvayka]

Created attachment 371890 [details]
Patch

[Alexey Shvayka]

This very test on Test262: https://test262.report/browse/built-ins/Proxy/getOwnPropertyDescriptor/result-is-undefined-target-is-not-extensible.js

[Oliver Hunt]

Comment on attachment 371890 [details]
Patch

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

> Source/JavaScriptCore/runtime/ProxyObject.cpp:-257
> -        // FIXME: this doesn't work if 'target' is another Proxy. We don't have isExtensible implemented in a way that fits w/ Proxys.
> -        // https://bugs.webkit.org/show_bug.cgi?id=154375

Why is this being removed? The FIXME is referencing an open bug that is not being addressed in this patch.

[Alexey Shvayka]

(In reply to Oliver Hunt from comment #3)
> Comment on attachment 371890 [details]
> Patch
>
> Why is this being removed? The FIXME is referencing an open bug that is not
> being addressed in this patch.

The referenced bug is indeed not being addressed in this patch, but it is RESOLVED.

[Saam Barati]

Comment on attachment 371890 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:-257
>> -        // https://bugs.webkit.org/show_bug.cgi?id=154375
> 
> Why is this being removed? The FIXME is referencing an open bug that is not being addressed in this patch.

Can we add a test for this too?

[Saam Barati]

(In reply to Saam Barati from comment #5)
> Comment on attachment 371890 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371890&action=review
> 
> >> Source/JavaScriptCore/runtime/ProxyObject.cpp:-257
> >> -        // https://bugs.webkit.org/show_bug.cgi?id=154375
> > 
> > Why is this being removed? The FIXME is referencing an open bug that is not being addressed in this patch.
> 
> Can we add a test for this too?

Or do you know when this was fixed? It was marked as config changed, but I'm not sure when it was actually fixed.

[Alexey Shvayka]

Created attachment 378900 [details]
Patch

Add Proxy.[[IsExtensible]] test.

[Alexey Shvayka]

(In reply to Saam Barati from comment #6)
> > Can we add a test for this too?
> 
> Or do you know when this was fixed? It was marked as config changed, but I'm
> not sure when it was actually fixed.

This was fixed in https://trac.webkit.org/changeset/197420. I've added a test too.

[Alexey Shvayka]

Created attachment 379162 [details]
Patch

Remove duplicated test case.

[Ross Kirsling]

Comment on attachment 379162 [details]
Patch

r=me, but let's update the Changelog to describe the other test as well.

[Alexey Shvayka]

Created attachment 379509 [details]
Patch

Set reviewer and update ChangeLog.

[WebKit Commit Bot]

Comment on attachment 379509 [details]
Patch

Clearing flags on attachment: 379509

Committed r250323: <https://trac.webkit.org/changeset/250323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55684240>