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.
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