[webkit-reviews] review denied: [Bug 16545] KJS::Bindings::Instance type conversions are not safe if multiple language bindings are used : [Attachment 18231] Slightly different patch - don't bother returning Instance*

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Michael Goddard
<michael.goddard at trolltech.com>'s request for review:
Bug 16545: KJS::Bindings::Instance type conversions are not safe if multiple
language bindings are used
http://bugs.webkit.org/show_bug.cgi?id=16545

Attachment 18231: Slightly different patch - don't bother returning Instance* 
http://bugs.webkit.org/attachment.cgi?id=18231&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good. Thanks for tackling this.

We really need a way to test this! We require tests that demonstrate bugs when
we fix them. Let figure out a way to come up with a test case.

The additional void* pointer is unnecessary. I agree that we need to check the
binding language, but this particular idiom, moving the pointers up into the
shared base class, doesn't seem good to me. I don't think it really makes
things all that much simpler. Here's what I suggest:

+	 NPObject* instance = static_cast<NPObject
*>(Instance::extractNativeInstanceFromObject(object, Instance::CLanguage));

Lines of code like the above, should be simply:

    NPObject* instance = getNPObject(object);

We can write the function getNPObject like this:

    static Instance* getInstance(JSObject* object, BindingLanguage language)
    {
	if (!object)
	    return 0;
	if (!object->inherits(&RuntimeObjectImp::info))
	    return 0;
	Instance* instance = static_cast<RuntimeObjectImp*>(object);
	if (instance->bindingLanguage() != language)
	    return 0;
	return instance;
    }

    static CInstance getCInstance(JSObject* object)
    {
	return static_cast<CInstance>(getInstance(object, CLanguage));
    }

    static NPObject* getNPObject(JSObject* object)
    {
	CInstance* instance = getCInstance(object);
	if (!instance)
	    return 0;
	return instance->getObject();
    }

We can put these functions wherever we think is appropriate. They can be static
member functions or free functions or whatever. The functions that have to be
written per-language are very simple -- boilerplate that's only a couple lines
long. If we really thought it necessary we could even make them use templates
so we don't have to write them multiple times. But I see no real benefit to
adding an additional field storing a redundant copy of the binding object
pointer.

Smaller comments that may be obviated by the above:

-    : Instance(rootObject)
+    : Instance((void*) o, rootObject)

-    : Instance(rootObject)
+    : Instance((void*) instance, rootObject)

-    : Instance(rootObject),
+    : Instance((void*) o, rootObject),

The type void* is a sort of "universal recipient", so there's no need to
typecast at sites like these.

+/*!
+   If the supplied \a object is a runtime object for a native object of the
specified
+   \a instanceLanguage, return the corresponding native object pointer as a
void*.	If the \a object is not
+   a runtime object, or is not of the specified \a instanceLanguage, returns
0.
+*/

We don't use this syntax for comments. I assume it's Doxygen or something like
that. It doesn't make sense to have this one comment with a different
formatting and the \a characters in it in a project where none of the other
comments do. Unless we have a plan of doing this sort of thing going forward
across the project.

+    static void* extractNativeInstanceFromObject(JSObject* object,
BindingLanguage instanceLanguage);

In declarations like this, we omit the argument names if their types make them
unambiguous.