Bug 154650 – [ES6] Implement Proxy.[[OwnPropertyKeys]]

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

[Saam Barati]

...

[Saam Barati]

Created attachment 272272 [details]
WIP

[Saam Barati]

Created attachment 272352 [details]
more WIP

[Saam Barati]

Created attachment 272356 [details]
more WIP

[Joseph Pecoraro]

Comment on attachment 272356 [details]
more WIP

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

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:456
> +        if (UNLIKELY(exec->hadException()))
> +            return JSValue();
> +    } else
>          return jsUndefined();

Is there any observable difference between returning JSValue() and jsUndefined()? In this case, the page is doing something like `var value = InspectorFrontendHost.method()`; Do JSValue() and jsUndefined() here produce the same result in `value`?

> Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:466
>      JSArray* array = constructEmptyArray(exec, nullptr);
> +    if (UNLIKELY(exec->hadException()))
> +        return JSValue();

There are a dozen other places we construct an empty object / array and putDirect in this file. Seems weird to check for just these couple.

[Saam Barati]

(In reply to comment #4)
> Comment on attachment 272356 [details]
> more WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272356&action=review
> 
> > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:456
> > +        if (UNLIKELY(exec->hadException()))
> > +            return JSValue();
> > +    } else
> >          return jsUndefined();
> 
> Is there any observable difference between returning JSValue() and
> jsUndefined()? In this case, the page is doing something like `var value =
> InspectorFrontendHost.method()`; Do JSValue() and jsUndefined() here produce
> the same result in `value`?
An exception was thrown so value will never actually get assigned to.
Therefore, there should be no observable difference.

> 
> > Source/JavaScriptCore/inspector/JSInjectedScriptHost.cpp:466
> >      JSArray* array = constructEmptyArray(exec, nullptr);
> > +    if (UNLIKELY(exec->hadException()))
> > +        return JSValue();
> 
> There are a dozen other places we construct an empty object / array and
> putDirect in this file. Seems weird to check for just these couple.
I'll probably just take this out and open a bug for this, that way
this patch is doing two things at once.

[Saam Barati]

Created attachment 272378 [details]
patch

[WebKit Commit Bot]

Attachment 272378 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ProxyObject.cpp:643:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 272378 [details]
patch

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

> Source/JavaScriptCore/API/JSObjectRef.cpp:627
> +    jsObject->methodTable()->getPropertyNames(jsObject, exec, array, EnumerationMode()); // FIXME: This throws but no caller expects it to.

I'm not sure what to do about this function.
It's callers don't expect it to throw. Maybe we can make it
safe and detect if it might throw and just not perform that operation?

[Geoffrey Garen]

Comment on attachment 272378 [details]
patch

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

r=me

>> Source/JavaScriptCore/API/JSObjectRef.cpp:627
>> +    jsObject->methodTable()->getPropertyNames(jsObject, exec, array, EnumerationMode()); // FIXME: This throws but no caller expects it to.
> 
> I'm not sure what to do about this function.
> It's callers don't expect it to throw. Maybe we can make it
> safe and detect if it might throw and just not perform that operation?

We just have to accept this limitation in the API, since there's no way to fix it and retain binary compatibility.

The ObjC API does not have this problem.

> Source/JavaScriptCore/runtime/JSObject.h:1529
> +        if (LIKELY(index <= std::numeric_limits<unsigned>::max()))
> +            next = arrayLikeValue.get(exec, static_cast<unsigned>(index));
> +        else
> +            next = arrayLikeValue.get(exec, Identifier::from(exec, static_cast<double>(index)));

This should be a helper function on JSValue.

> Source/JavaScriptCore/runtime/ProxyObject.h:40
> +    // We lie an say we override getPropertyNames() because it prevents
> +    // property name enumeration caching.

Why is this a lie? We do in fact implement getOwnPropertyNames.

> Source/JavaScriptCore/runtime/Structure.h:580
> -    DEFINE_BITFIELD(bool, hasNonEnumerableProperties, HasNonEnumerableProperties, 1, 5);
> +    DEFINE_BITFIELD(bool, isQuickPropertyAccessPreventedForEnumeration, IsQuickPropertyAccessPreventedForEnumeration, 1, 5);

It's bad for booleans to express negatives, because then you end up writing double negatives ("!isQuickPropertyAccessPreventedForEnumeration" means "is not quick property access not allowed?").

Let's call this isQuickPropertyAccessForEnumeration, and invert its meaning.

[Saam Barati]

Comment on attachment 272378 [details]
patch

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

>> Source/JavaScriptCore/runtime/JSObject.h:1529
>> +            next = arrayLikeValue.get(exec, Identifier::from(exec, static_cast<double>(index)));
> 
> This should be a helper function on JSValue.

will change.

>> Source/JavaScriptCore/runtime/ProxyObject.h:40
>> +    // property name enumeration caching.
> 
> Why is this a lie? We do in fact implement getOwnPropertyNames.

We do implement *getOwnPropertyNames*. But we do not implement *getPropertyNames*.

>> Source/JavaScriptCore/runtime/Structure.h:580
>> +    DEFINE_BITFIELD(bool, isQuickPropertyAccessPreventedForEnumeration, IsQuickPropertyAccessPreventedForEnumeration, 1, 5);
> 
> It's bad for booleans to express negatives, because then you end up writing double negatives ("!isQuickPropertyAccessPreventedForEnumeration" means "is not quick property access not allowed?").
> 
> Let's call this isQuickPropertyAccessForEnumeration, and invert its meaning.

will change

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/197539