[webkit-dev] JavascriptCore questions

[Geoffrey Garen]

Hi again!

>>> - ConvertValueToNPVariant() and convertValueToObjcValue(), which are
>>> used in the plugin interface binding code, both contain special  
>>> logic
>>> 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 :).

>>> - The JavaScriptObject struct contains pointers to RootObjects. I  
>>> may
>>> be missing something, but it appears to me that a JavaScriptObject
>>> struct can outlive the RootObjects that it points at, which could  
>>> lead
>>> 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?

Sure.

>>> - There are a variety of places where
>>> exec->dynamicInterpreter()->globalObject() is called. For example,
>>> this
>>> 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
>>> 15.3.4.4
>>> 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 8.6.2.6 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  
>>> file
>>> 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  
PropertyIsEnumerable virtual.

> 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()  
returns the wrong prototype. A RuntimeObjectImp is a JavaScript  
object that communicates with an object in another language runtime  
and, as a JavaScript object, it has as its prototype the default  
object prototype.

Your case is unique, since you want a JavaScript object that not only  
to communicates with, but embodies, a JavaScript object in another  
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.

Geoff