[Webkit-unassigned] [Bug 33159] Make JSObject::getPropertyNames() non-virtual

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33159





--- Comment #6 from Geoffrey Garen <ggaren at apple.com>  2010-01-05 13:20:57 PST ---
> 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.