Bug 171291

Summary: [JSC] Object.keys() must discard property names with no PropertyDescriptor
Product: WebKit Reporter: Caitlin Potter (:caitp) <caitp>
Component: New BugsAssignee: Caitlin Potter (:caitp) <caitp>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, joepeck, keith_miller, mark.lam, msaboff, rniwa, sbarati, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Patch
none
Patch
none
Update comments, refactor a bit for readability none

Description Caitlin Potter (:caitp) 2017-04-25 14:40:32 PDT
[JSC] Object.keys() must discard property names with no PropertyDescriptor
Comment 1 Caitlin Potter (:caitp) 2017-04-25 14:42:30 PDT
Created attachment 308146 [details]
Patch
Comment 2 Build Bot 2017-04-25 15:21:21 PDT
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
Comment 3 Build Bot 2017-04-25 15:56:51 PDT
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
Comment 4 Build Bot 2017-04-25 15:56:52 PDT
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
Comment 5 Caitlin Potter (:caitp) 2017-04-25 17:28:51 PDT
Created attachment 308172 [details]
Patch
Comment 6 Caitlin Potter (:caitp) 2017-04-25 17:54:06 PDT
Created attachment 308178 [details]
Patch
Comment 7 Caitlin Potter (:caitp) 2017-04-25 17:55:04 PDT
The last patchset is modified to pass a proposed test262 change (https://github.com/tc39/test262/pull/1003/)
Comment 8 Yusuke Suzuki 2017-04-25 20:01:22 PDT
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.
Comment 9 Caitlin Potter (:caitp) 2017-04-26 05:37:08 PDT
(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?
Comment 10 Yusuke Suzuki 2017-04-26 05:50:43 PDT
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.
Comment 11 Caitlin Potter (:caitp) 2017-04-26 06:31:22 PDT
Created attachment 308240 [details]
Update comments, refactor a bit for readability
Comment 12 Caitlin Potter (:caitp) 2017-04-26 06:33:28 PDT
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.
Comment 13 Yusuke Suzuki 2017-04-26 06:35:56 PDT
Comment on attachment 308240 [details]
Update comments, refactor a bit for readability

r=me, nice.
Comment 14 WebKit Commit Bot 2017-04-26 07:56:57 PDT
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>
Comment 15 WebKit Commit Bot 2017-04-26 07:56:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Saam Barati 2017-04-26 09:04:01 PDT
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
Comment 17 Caitlin Potter (:caitp) 2017-04-26 09:05:42 PDT
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
Comment 18 Saam Barati 2017-04-26 09:13:47 PDT
(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.
Comment 19 Caitlin Potter (:caitp) 2017-04-26 09:25:41 PDT
(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.
Comment 20 Caitlin Potter (:caitp) 2017-04-26 09:29:50 PDT
(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
Comment 21 Caitlin Potter (:caitp) 2017-04-26 09:42:21 PDT
Fixup in https://bugs.webkit.org/show_bug.cgi?id=171330
Comment 22 Saam Barati 2017-04-26 11:44:51 PDT
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.