Bug 141952

Summary: GenericArguments<Type>::getOwnPropertyNames() should respect modified descriptors
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: NEW    
Severity: Normal CC: ashvayka, ews-watchlist, keith_miller, mark.lam, msaboff, saam, ticaiolima, tzagallo, ysuzuki
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=159398
Bug Depends on: 141951    
Bug Blocks:    
Attachments:
Description Flags
Patch ysuzuki: review+, ews-feeder: commit-queue-

Filip Pizlo
Reported 2015-02-23 21:14:52 PST
Wow, that's a mouthful. The title of this bug is almost as weird as how browsers behave when you do defineProperty on arguments.
Attachments
Patch (11.23 KB, patch)
2021-03-13 11:05 PST, Alexey Shvayka 		ysuzuki: review+
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Caio Lima
Comment 1 2016-12-29 09:15:26 PST
This strange behavior was fixed on https://bugs.webkit.org/show_bug.cgi?id=159398
Alexey Shvayka
Comment 2 2021-03-13 11:05:44 PST
Created attachment 423105 [details] Patch
Alexey Shvayka
Comment 3 2021-03-13 11:08:34 PST
(In reply to Caio Lima from comment #1) > This strange behavior was fixed on > https://bugs.webkit.org/show_bug.cgi?id=159398 r210146 is a great patch that fixed enumerability observed via getOwnPropertySlot[ByIndex](), but not via getOwnPropertyNames(), which is taken care by this patch.
Yusuke Suzuki
Comment 4 2021-03-13 18:23:07 PST
Comment on attachment 423105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423105&action=review r=me > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:101 > + auto shouldEnumerateIndex = [&] (unsigned index) -> bool { Let's take JSGlobalObject since it can throw an error now. > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:107 > + bool hasProperty = Base::getOwnPropertySlotByIndex(thisObject, globalObject, index, slot); > + slot.disallowVMEntry.reset(); Let's do an error check here still because we still have a chance invoking OOM errors etc. > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:116 > + if (shouldEnumerateIndex(i)) Let's insert RETURN_IF_EXCEPTION after this call.
Alexey Shvayka
Comment 5 2021-04-09 18:36:45 PDT
(In reply to Yusuke Suzuki from comment #4) Thank you for review! > > Source/JavaScriptCore/runtime/GenericArgumentsInlines.h:107 > > + bool hasProperty = Base::getOwnPropertySlotByIndex(thisObject, globalObject, index, slot); > > + slot.disallowVMEntry.reset(); > > Let's do an error check here still because we still have a chance invoking OOM errors etc. Hmm, I've followed Base::getOwnPropertySlotByIndex() and couldn't find how an error can be invoked. Base is JSObject here. JSObject::getOwnPropertySlotByIndex() is safe (no throw scope); it may forward globalObject to getOwnPropertySlot() method table call though. That would be JSObject::getOwnPropertySlotImpl(), which is itself safe (no throw scope) and may pass globalObject only to getOwnPropertySlotByIndex(). getOwnNonIndexPropertySlot() can't throw, GetterSetter allocation should be safe too. Seems like getting slot by index is safe, unlike indexed put. What am I missing?
Note You need to log in before you can comment on or make changes to this bug.