Bug 33159 – Make JSObject::getPropertyNames() non-virtual

• Component: JavaScriptCore
• Status: RESOLVED INVALID

[Kent Hansen]

With the introduction of JSObject::getOwnPropertyNames(), getPropertyNames() doesn't need to be virtual anymore.
Having two virtual methods for getting property names is confusing and error-prone. getPropertyNames() should
always follow the prototype chain and call getOwnPropertyNames() on each object.

[Kent Hansen]

Created attachment 45807 [details]
Proposed patch

From the C API perspective, the change has no impact, since JSObjectGetPropertyNamesCallback already has the semantics of getOwnPropertyNames().

OverridesGetPropertyNames flag renamed to OverridesGetOwnPropertyNames. (I don't know why the flag is set for several classes which _don't_ reimplement it? Are they just out of sync?)

DebuggerActivation: I think it's a bug that getOwnPropertyNames() was calling m_activation->getPropertyNames() instead of m_activation->getOwnPropertyNames()?

JSC::Bindings::Instance and friends: When enumerating such objects, the properties in the prototype chain will now be included as well. I'm not sure whether this is desirable or not. E.g. if you use an NPObject in a for-in statement, should the properties in the prototype chain be included? I would think "Yes", since the NP API has no knowledge of the prototype at all, so it's not up to it to block its properties. I'm seeing NPN_Enumerate as another "own" properties variation.

The RuntimeObjectImp::getOwnPropertyNames() implementation seemed broken; it simply called itself recursively with the same arguments, causing a stack overflow if you called e.g. Object.keys() on such an object.

JSDOMWindow: There will no longer be an "early cut-off" when getPropertyNames() is called on a JSDOMWindow that's not accessible from the current context; instead the prototype chain will be followed. This is bad and reason to reject this patch, but I wish there was a different way to handle the security checks (e.g. SpiderMonkey has a checkAccess callback in the base JS "class", so the check can be initiated from the engine side). Well, as it stands, I guess this disproves the claim in the task description. :]
From looking at the JSC C API, it seems that JSObjectGetPrototype() doesn't do a security check, so it's going to behave differently from JSObjectGetProperty("__proto__")? Is that fine? Having the security check in JSObject::prototype() would fix the issue with this part of the patch as well... (I realize there would probably be performance implications to investigate.)

JSQuarantinedObjectWrapper: Same issue as JSDOMWindow.

All the existing tests seem to pass, anyway.

Maybe it makes sense to split this bug into smaller tasks to deal with the various issues.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45807 [details] without any errors.

[Darin Adler]

(In reply to comment #1)
> Maybe it makes sense to split this bug into smaller tasks to deal with the
> various issues.

Yes. If the patch is just refactoring, then it should fix no bugs. If it fixes bugs, then they should ideally be fixed one at a time with either a test or an explanation of why it can't be tested.

[Eric Seidel (no email)]

Oliver or Geoff are probably your best reviewers here.

[Geoffrey Garen]

Comment on attachment 45807 [details]
Proposed patch

This patch looks promising, but I agree with Darin: refactoring and potential behavior changes should be separate patches. I'll respond to your specific questions in a separate post.

[Geoffrey Garen]

> From the C API perspective, the change has no impact, since
> JSObjectGetPropertyNamesCallback already has the semantics of
> getOwnPropertyNames().

Right.

> OverridesGetPropertyNames flag renamed to OverridesGetOwnPropertyNames. (I
> don't know why the flag is set for several classes which _don't_ reimplement
> it? Are they just out of sync?)

Yes, that sounds like a mistake.

> DebuggerActivation: I think it's a bug that getOwnPropertyNames() was calling
> m_activation->getPropertyNames() instead of
> m_activation->getOwnPropertyNames()?

Yeah, that's a bug. (Technically, since activation objects have no prototypes, it's a harmless bug, but a bug nonetheless.)

> JSC::Bindings::Instance and friends: When enumerating such objects, the
> properties in the prototype chain will now be included as well. I'm not sure
> whether this is desirable or not. E.g. if you use an NPObject in a for-in
> statement, should the properties in the prototype chain be included?

Yes, I think so. You can write a test for this behavior by adding a property to Object.prototype, and then checking to see that the property shows up when enumerating the properties of a plugin object.

> The RuntimeObjectImp::getOwnPropertyNames() implementation seemed broken; it
> simply called itself recursively with the same arguments, causing a stack
> overflow if you called e.g. Object.keys() on such an object.

Yes, that looks broken. 

> JSDOMWindow: There will no longer be an "early cut-off" when getPropertyNames()
> is called on a JSDOMWindow that's not accessible from the current context;
> instead the prototype chain will be followed. This is bad and reason to reject
> this patch, but I wish there was a different way to handle the security checks
> (e.g. SpiderMonkey has a checkAccess callback in the base JS "class", so the
> check can be initiated from the engine side). Well, as it stands, I guess this
> disproves the claim in the task description. :]
> From looking at the JSC C API, it seems that JSObjectGetPrototype() doesn't do
> a security check, so it's going to behave differently from
> JSObjectGetProperty("__proto__")? Is that fine? Having the security check in
> JSObject::prototype() would fix the issue with this part of the patch as
> well... (I realize there would probably be performance implications to
> investigate.)
> 
> JSQuarantinedObjectWrapper: Same issue as JSDOMWindow.

These issues seem to be deal-breakers for making getPropertyNames nonvirtual.

You could add a separate checkAccess-style virtual function, but then you've just traded one virtual function for another. What's the benefit?

It's OK for JSObjectGetPrototype to behave differently from JavaScript access to __proto__, because clients of the C API are not subject to security checks in the same way that web pages are.

Adding a security check to JSObject::prototype() seems likely to be a performance problem, but maybe there's a clever way to do it.

[Kent Hansen]

(In reply to comment #6)
> > JSDOMWindow: There will no longer be an "early cut-off" when getPropertyNames()
> > is called on a JSDOMWindow that's not accessible from the current context;
[snip]
> These issues seem to be deal-breakers for making getPropertyNames nonvirtual.
> 
> You could add a separate checkAccess-style virtual function, but then you've
> just traded one virtual function for another. What's the benefit?

Yep. I was far into the patch when realizing this issue. I took another look at the patch that introduced getOwnPropertyNames(), and indeed, the reason it kept getPropertyNames() virtual was to keep the security behavior intact. So I'd say it's not justified to change this as it stands, since introducing the virtual access-check would require refactoring elsewhere and potentially slow things down.

Oh well, at least I found some other bugs & interesting things in the process; thanks for verifying those!

[Kent Hansen]

(In reply to comment #6)
> > JSC::Bindings::Instance and friends: When enumerating such objects, the
> > properties in the prototype chain will now be included as well. I'm not sure
> > whether this is desirable or not. E.g. if you use an NPObject in a for-in
> > statement, should the properties in the prototype chain be included?
> 
> Yes, I think so. You can write a test for this behavior by adding a property to
> Object.prototype, and then checking to see that the property shows up when
> enumerating the properties of a plugin object.
> 
> > The RuntimeObjectImp::getOwnPropertyNames() implementation seemed broken; it
> > simply called itself recursively with the same arguments, causing a stack
> > overflow if you called e.g. Object.keys() on such an object.
> 
> Yes, that looks broken. 

Spun off https://bugs.webkit.org/show_bug.cgi?id=33371 for the infinite recursion bug.
I also hoped that it would, as a nice bonus, fix the "properties in the prototype chain should be included when enumerating an NPObject" issue, but no.
I checked in FireFox, and there the prototype properties are included in the enumeration.
The problem in JSC/WebCore seems to be that the __proto__ property of a plugin object can't be assigned to, and it always reads as null.
I'm investigating whether this is on purpose or if it's a bug as well.

[Kent Hansen]

(In reply to comment #8)
> The problem in JSC/WebCore seems to be that the __proto__ property of a plugin
> object can't be assigned to, and it always reads as null.
> I'm investigating whether this is on purpose or if it's a bug as well.

Spun off https://bugs.webkit.org/show_bug.cgi?id=33595 with more information about this one.