[Webkit-unassigned] [Bug 16545] KJS::Bindings::Instance type conversions are not safe if multiple language bindings are used

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=16545


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #18231|review?                     |review-
               Flag|                            |




------- Comment #6 from darin at apple.com  2008-01-02 10:57 PDT -------
(From update of attachment 18231)
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.


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