Bug 171291 – [JSC] Object.keys() must discard property names with no PropertyDescriptor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caitlin Potter (:caitp)]

[JSC] Object.keys() must discard property names with no PropertyDescriptor

[Caitlin Potter (:caitp)]

Created attachment 308146 [details]
Patch

[Build Bot]

Comment on attachment 308146 [details]
Patch

Attachment 308146 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3604465

New failing tests:
es6.yaml/es6/Proxy_ownKeys_duplicates.js.default

[Build Bot]

Comment on attachment 308146 [details]
Patch

Attachment 308146 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3604632

New failing tests:
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context.html

[Build Bot]

Created attachment 308157 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Caitlin Potter (:caitp)]

Created attachment 308172 [details]
Patch

[Caitlin Potter (:caitp)]

Created attachment 308178 [details]
Patch

[Caitlin Potter (:caitp)]

The last patchset is modified to pass a proposed test262 change (https://github.com/tc39/test262/pull/1003/)

[Yusuke Suzuki]

Nice catch.
Looking up each descriptor is costly.
If Proxy can prodduce key w/o desc, I think we should add this as a slow path for Proxy case and keep the current one as the fast path.

[Caitlin Potter (:caitp)]

(In reply to Caitlin Potter (:caitp) from comment #7)
> The last patchset is modified to pass a proposed test262 change
> (https://github.com/tc39/test262/pull/1003/)

I believe I've prevented lookup when it's not needed/observable, but it is a bit confusing. Can you take a look at that approach and confirm that the slow path only taken when needed?

[Yusuke Suzuki]

Comment on attachment 308178 [details]
Patch

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

Oops, right. r=me :)

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:733
> +    // Object.keys() cannot skip calling [[GetOwnProperty]] on Proxy objects, which can observe the call.

Let's add https://tc39.github.io/ecma262/#sec-enumerableownproperties URL here.

[Caitlin Potter (:caitp)]

Created attachment 308240 [details]
Update comments, refactor a bit for readability

[Caitlin Potter (:caitp)]

Comment on attachment 308178 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:733
>> +    // Object.keys() cannot skip calling [[GetOwnProperty]] on Proxy objects, which can observe the call.
> 
> Let's add https://tc39.github.io/ecma262/#sec-enumerableownproperties URL here.

I've updated the comment, and refactored a bit for readability. Let me know if that change works for you, and I'll go ahead and land.

[Yusuke Suzuki]

Comment on attachment 308240 [details]
Update comments, refactor a bit for readability

r=me, nice.

[WebKit Commit Bot]

Comment on attachment 308240 [details]
Update comments, refactor a bit for readability

Clearing flags on attachment: 308240

Committed r215799: <http://trac.webkit.org/changeset/215799>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 308240 [details]
Update comments, refactor a bit for readability

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

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:742
> +        return object->getOwnPropertyDescriptor(exec, name, descriptor) && descriptor.enumerable();

I think you're missing an exception check here or the caller of this lambda

[Caitlin Potter (:caitp)]

Comment on attachment 308240 [details]
Update comments, refactor a bit for readability

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:742
>> +        return object->getOwnPropertyDescriptor(exec, name, descriptor) && descriptor.enumerable();
> 
> I think you're missing an exception check here or the caller of this lambda

I realize it looks like that, but I wasn't able to make that cause any problems.

I can fix this in a followup if it's a big deal

[Saam Barati]

(In reply to Caitlin Potter (:caitp) from comment #17)
> Comment on attachment 308240 [details]
> Update comments, refactor a bit for readability
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308240&action=review
> 
> >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:742
> >> +        return object->getOwnPropertyDescriptor(exec, name, descriptor) && descriptor.enumerable();
> > 
> > I think you're missing an exception check here or the caller of this lambda
> 
> I realize it looks like that, but I wasn't able to make that cause any
> problems.
> 
> I can fix this in a followup if it's a big deal

Were you able to observe a function being called twice when it shouldn't have been? My guess is your patch would've allowed that.

This is definitely worth fixing. The whole point of ThrowScope is that when we call something that throws, we must check for an exception.

[Caitlin Potter (:caitp)]

(In reply to Saam Barati from comment #18)
> (In reply to Caitlin Potter (:caitp) from comment #17)
> > Comment on attachment 308240 [details]
> > Update comments, refactor a bit for readability
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=308240&action=review
> > 
> > >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:742
> > >> +        return object->getOwnPropertyDescriptor(exec, name, descriptor) && descriptor.enumerable();
> > > 
> > > I think you're missing an exception check here or the caller of this lambda
> > 
> > I realize it looks like that, but I wasn't able to make that cause any
> > problems.
> > 
> > I can fix this in a followup if it's a big deal
> 
> Were you able to observe a function being called twice when it shouldn't
> have been? My guess is your patch would've allowed that.
> 
> This is definitely worth fixing. The whole point of ThrowScope is that when
> we call something that throws, we must check for an exception.

I wasn't able to observe this, though I was actually surprised that I wasn't able to. I assume there's some caching behaviour that prevents this.

[Caitlin Potter (:caitp)]

(In reply to Caitlin Potter (:caitp) from comment #19)
> (In reply to Saam Barati from comment #18)
> > (In reply to Caitlin Potter (:caitp) from comment #17)
> > > Comment on attachment 308240 [details]
> > > Update comments, refactor a bit for readability
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=308240&action=review
> > > 
> > > >> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:742
> > > >> +        return object->getOwnPropertyDescriptor(exec, name, descriptor) && descriptor.enumerable();
> > > > 
> > > > I think you're missing an exception check here or the caller of this lambda
> > > 
> > > I realize it looks like that, but I wasn't able to make that cause any
> > > problems.
> > > 
> > > I can fix this in a followup if it's a big deal
> > 
> > Were you able to observe a function being called twice when it shouldn't
> > have been? My guess is your patch would've allowed that.
> > 
> > This is definitely worth fixing. The whole point of ThrowScope is that when
> > we call something that throws, we must check for an exception.
> 
> I wasn't able to observe this, though I was actually surprised that I wasn't
> able to. I assume there's some caching behaviour that prevents this.

I mean, if you're referring to multiple `getOwnPropertyDescriptor` trap calls for the same property. I wasn't able to make that happen (and I expected this to be able to happen).

If you're referring to additional properties being processed after a getOwnPropertyDescriptor trap throws, no, that certainly did not happen

[Caitlin Potter (:caitp)]

Fixup in https://bugs.webkit.org/show_bug.cgi?id=171330

[Saam Barati]

Comment on attachment 308240 [details]
Update comments, refactor a bit for readability

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

>>>>>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:742
>>>>>> +        return object->getOwnPropertyDescriptor(exec, name, descriptor) && descriptor.enumerable();
>>>>> 
>>>>> I think you're missing an exception check here or the caller of this lambda
>>>> 
>>>> I realize it looks like that, but I wasn't able to make that cause any problems.
>>>> 
>>>> I can fix this in a followup if it's a big deal
>>> 
>>> Were you able to observe a function being called twice when it shouldn't have been? My guess is your patch would've allowed that.
>>> 
>>> This is definitely worth fixing. The whole point of ThrowScope is that when we call something that throws, we must check for an exception.
>> 
>> I wasn't able to observe this, though I was actually surprised that I wasn't able to. I assume there's some caching behaviour that prevents this.
> 
> I mean, if you're referring to multiple `getOwnPropertyDescriptor` trap calls for the same property. I wasn't able to make that happen (and I expected this to be able to happen).
> 
> If you're referring to additional properties being processed after a getOwnPropertyDescriptor trap throws, no, that certainly did not happen

Yeah I'm a bit surprised that this couldn't happen as well. Perhaps the reason for this is that JSObject::getOwnPropertyDescriptor's first line of code is to call getOwnPropertySlot. So it probably depends on what that instance of getOwnPropertySlot does. Maybe it tries to do something it thinks it can throw, but it doesn't throw, but there is already a pending exception, so it returns early. Hard to tell.

Anyways, I think the right fix is what you did in a follow up to just add the needed exception check.