ggaren at apple.com
Thu Dec 21 13:33:53 PST 2006
>>> - ConvertValueToNPVariant() and convertValueToObjcValue(), which are
>>> used in the plugin interface binding code, both contain special
>>> that basically says "if wrapping up a window object, store the
>>> RootObject associated with the window object in the wrapper rather
>>> than the current RootObject". Can you tell me the purpose of this
>>> logic? I have theories, but nothing definitive.
>> I'm not sure.
> Can you give me any advice on how to find out?
I would suggest testing the NPAPI with this feature enabled and
disabled, and seeing if you can suss out any difference. We have a
test Netscape plug-in in WebKitTools/DumpRenderTree that you can
modify to suite your needs. If you have some theories about why the
code is written as it is, I'd be happy to entertain them :).
>>> struct can outlive the RootObjects that it points at, which could
>>> to the use of freed memory. Can this happen?
>> Yes, I think so. That's a bug. We're tracking some bugs along
>> those lines.
> Should I enter a bug for this, or is it already covered?
If you have reproducible steps for causing a crash, that's probably
worth its own bug. Otherwise, I think the existing bug is sufficient.
>>> - When a plugin is unloaded, what happens if there are existing
>>> RuntimeObjectImp instances that reference objects provided by the
>>> plugin? Is there logic to prevent a plugin from being unloaded if
>>> there are extant objects that it has provided? I haven't been
>>> able to
>>> find any code that ensures this.
>> No. I believe this is a bug.
> Should I enter a bug?
>>> - There are a variety of places where
>>> exec->dynamicInterpreter()->globalObject() is called. For example,
>>> happens in FunctionProtoFunc::callAsFunction() to handle the apply
>>> case when the this arg is NULL. In such cases, is it correct to use
>>> the global obj from the current ExecState's Interpreter as
>>> opposed to
>>> the global object at the root of the scope chain? ECMA 262 section
>>> seems somewhat ambiguous on this subject, but my reading leads me to
>>> think that it would be more correct to pull from the scope chain.
>> Filed as http://bugs.webkit.org/show_bug.cgi?id=11884.
> I'm aware of several other places that may also be similar
> problems. Would you like me to update the bug you entered? Enter
> separate bugs for each? Just send my findings to this list?
A bug report is always better than an email. In theory, each instance
of this mistake produces a different buggy behavior, so separate
reports for each would probably be best.
>>> - The default implementation of JSObject::defaultValue() and
>>> JSObject::toPrimitive() can return an Error object if an exception
>>> occurs. This would seem to violate ECMA 262 sections 22.214.171.124 and 9.1
>>> which state that defaultvalue() and toPrimitive() return non-Object
>>> types. Looking at some of the callers of toPrimitive(), it appears
>>> that some are not prepared to handle getting an object back. I'll
>>> this as a bug if you guys like.
>> defaultValue() and toPrimitive() must be able to throw exceptions
>> because they call arbitrary functions. When the >spec says, "The
>> above specification of [[DefaultValue]] for native objects can
>> return only primitive values," I think it's just attempting to
>> explain that the purpose of defaultValue() is to return a
>> primitive value, although it can throw an exception as well. If
>> there's code that might fail because an exception is thrown, I
>> think that warrants a bug.
> DateObjectImp::construct(), equal(), relation(), add() and
> UserObjectImp::getOwnPropertySlot() all call toPrimitive() and do
> not appear to be prepared to handle the case where an Error object
> is returned/an exception is set in the exec. Bugs?
Technically, all of these except for getOwnPropertySlot are probably
bugs. (Whether [[get]] should throw an exception when it encounters a
bad value is up to the class doing the getting.)
However, it's hard to imagine a situation where things would go
wrong. If I called 'new Date(valueThatDoesNotConvertToPrimitive)',
DateObjectImp::construct() would not return an error object, but the
exception would be set in the ExecState, and the Node tree would
catch and report it as a part of evaluation. (Remember
KJS_CHECKEXCPETION*.) Whether a function wants to return immediately
when an exception is thrown is, I guess, up to the function.
>> If you want to override those functions, you're probably (although
>> not definitely) doing things wrong. Object has default behavior,
>> allowing you to add and remove properties with attributes.
>> Subclasses can implement custom properties, overriding get, put,
>> getPropertyNames, etc. So there's no need to muck around in
>> Object's implementation of its default behavior, when you can
>> override that behavior, instead. In other words, the functions you
>> mention should really be private, instead of virtual.
> I'd prefer to make them both private, but it looks like
> JSObject::propertyIsEnumerable() is called by
> ObjectProtoFunc::callAsFunction() in the "PropertyIsEnumerable"
> case. Since a caller can use Function.call to set the "this" object
> used by ObjectProtoFunc::callAsFunction(), and thus we can't assume
> this obj is any particular subclass of JSObject, it would appear
> that PropertyIsEnumerable needs to remain public. If you concur
> with this analysis, any objection to making
> JSObject::PropertyIsEnumerable() virtual? As it is only called by
> ObjectProtoFunc::callAsFunction()'s PropertyIsEnumerable handler,
> it seems unlikely that there will be any interesting perf hit from
> doing so.
Ah! I missed that case. Yes, I think it's correct to make
> Along the same lines, JSObject::prototype() is not virtual, and
> this prevents me from overriding it to return the correct prototype
> (the proto for the object in the other engine) in the case I'm
> dealing with. Instead, a caller gets the local prototype (the
> prototype for the wrapper) which is incorrect. Note that
> RuntimeObjectImp (in the binding code) also suffers from this
> affliction. Any objection to making JSObject::prototype() virtual,
> or suggestions for other approaches?
I hesitate to make prototype access virtual. It's a part of property
lookup, which is our hottest code.
I'm not sure I understand why you think RuntimeObjectImp::prototype()
object that communicates with an object in another language runtime
language runtime, prototype and all. To accomplish that, I think you
need to do a one-time fix-up of the object's prototype chain at
create time. This approach is not ideal, but the cost it incurs is
about equal to the cost of a single property lookup, and it happens
during a much more expensive object allocation, anyway.
More information about the webkit-dev