[JSC] Object.keys() must discard property names with no PropertyDescriptor
Created attachment 308146 [details] Patch
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 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
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
Created attachment 308172 [details] Patch
Created attachment 308178 [details] Patch
The last patchset is modified to pass a proposed test262 change (https://github.com/tc39/test262/pull/1003/)
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.
(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 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.
Created attachment 308240 [details] Update comments, refactor a bit for readability
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 on attachment 308240 [details] Update comments, refactor a bit for readability r=me, nice.
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>
All reviewed patches have been landed. Closing bug.
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 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
(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.
(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.
(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
Fixup in https://bugs.webkit.org/show_bug.cgi?id=171330
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.