Bug 154768 - ProxyObject.[[GetOwnProperty]] is partially broken because it doesn't propagate information back to the slot
Summary: ProxyObject.[[GetOwnProperty]] is partially broken because it doesn't propaga...
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
Depends on:
Reported: 2016-02-26 18:13 PST by Saam Barati
Modified: 2016-02-28 10:40 PST (History)
4 users (show)

See Also:

patch (12.23 KB, patch)
2016-02-26 20:34 PST, Saam Barati
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-02-26 18:13:49 PST
Comment 1 Saam Barati 2016-02-26 20:34:46 PST
Created attachment 272397 [details]
Comment 2 Ryosuke Niwa 2016-02-26 21:53:11 PST
Comment on attachment 272397 [details]

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

> Source/JavaScriptCore/ChangeLog:10
> +        This fixes a big bug with ProxyObject.[[GetOwnProperty]]. We weren't
> +        correctly propagating the result of this operation to the out PropertySlot&
> +        parameter. This patch fixes that and adds tests.

Can we refer to http://www.ecma-international.org/ecma-262/6.0/#sec-proxy-object-internal-methods-and-internal-slots-getownproperty-p
either in the change log or in the code?

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:236
>      JSObject* description = constructEmptyObject(exec);
> +    if (exec->hadException())
> +        return jsUndefined();

Please mention this change in the change log.

> Source/JavaScriptCore/runtime/ProxyObject.cpp:216
> +        GetterSetter* getterSetter = trapResultAsDescriptor.slowGetterSetter(exec);
> +        if (exec->hadException())
> +            return false;

I guess the only way we can fail here would be memory exhaustion?
Otherwise, we should be careful about the order in which these conditions are checked as they're observable.
Comment 3 Saam Barati 2016-02-28 10:40:53 PST
landed in: