Bug 141952 – GenericArguments<Type>::getOwnPropertyNames() should respect modified descriptors

• Status: NEW
• Component: JavaScriptCore

[Filip Pizlo]

Wow, that's a mouthful.  The title of this bug is almost as weird as how browsers behave when you do defineProperty on arguments.

[Caio Lima]

This strange behavior was fixed on https://bugs.webkit.org/show_bug.cgi?id=159398

[Alexey Shvayka]

Created attachment 423105 [details]
Patch

[Alexey Shvayka]

(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 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]

(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?