Bug 141952 - GenericArguments<Type>::getOwnPropertyNames() should respect modified descriptors
Summary: GenericArguments<Type>::getOwnPropertyNames() should respect modified descrip...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords:
Depends on: 141951
Blocks:
  Show dependency treegraph
 
Reported: 2015-02-23 21:14 PST by Filip Pizlo
Modified: 2021-04-09 18:36 PDT (History)
9 users (show)

See Also:


Attachments
Patch (11.23 KB, patch)
2021-03-13 11:05 PST, Alexey Shvayka
ysuzuki: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 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.
Comment 1 Caio Lima 2016-12-29 09:15:26 PST
This strange behavior was fixed on https://bugs.webkit.org/show_bug.cgi?id=159398
Comment 2 Alexey Shvayka 2021-03-13 11:05:44 PST
Created attachment 423105 [details]
Patch
Comment 3 Alexey Shvayka 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.
Comment 4 Yusuke Suzuki 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.
Comment 5 Alexey Shvayka 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?