Bug 171291 - [JSC] Object.keys() must discard property names with no PropertyDescriptor
Summary: [JSC] Object.keys() must discard property names with no PropertyDescriptor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caitlin Potter (:caitp)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-25 14:40 PDT by Caitlin Potter (:caitp)
Modified: 2017-04-26 11:44 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.91 KB, patch)
2017-04-25 14:42 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (943.60 KB, application/zip)
2017-04-25 15:56 PDT, Build Bot
no flags Details
Patch (7.61 KB, patch)
2017-04-25 17:28 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2017-04-25 17:54 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff
Update comments, refactor a bit for readability (7.99 KB, patch)
2017-04-26 06:31 PDT, Caitlin Potter (:caitp)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.