[webkit-reviews] review denied: [Bug 34730] [Qt] Null QObjects properties cause Segmentation Fault : [Attachment 53370] Patch with the corrections requested by Kenneth Rohde Christiansen

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Bruno Schmidt
<bruno.schmidt at gmail.com>'s request for review:
Bug 34730: [Qt] Null QObjects properties cause Segmentation Fault
https://bugs.webkit.org/show_bug.cgi?id=34730

Attachment 53370: Patch with the corrections requested by Kenneth Rohde
Christiansen
https://bugs.webkit.org/attachment.cgi?id=53370&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>

>  Class* QtInstance::getClass() const
>  {
> -    if (!m_class)
> +    if (!m_class) {
> +	   if (!m_object)
> +	       return 0;
>	   m_class = QtClass::classForObject(m_object);
> +    }
>      return m_class;
>  }

The code would break other code in the file such as:

MethodList methodList = getClass()->methodsNamed(propertyName, this);

so that might need fixing as well.


> @@ -257,11 +260,14 @@ JSValue QtInstance::defaultValue(ExecState* exec,
PreferredPrimitiveType hint) c
>  
>  JSValue QtInstance::stringValue(ExecState* exec) const
>  {
> +    QObject* obj = getObject();
> +    if (!obj)
> +	   return jsNull();

Good change!

> +
>      // Hmm.. see if there is a toString defined
>      QByteArray buf;
>      bool useDefault = true;
>      getClass();
> -    QObject* obj = getObject();
>      if (m_class && obj) {

You do not need the obj check here any longer

>  JSValue QtInstance::booleanValue() const
>  {
>      // ECMA 9.2
> -    return jsBoolean(true);
> +    return jsBoolean(getObject());
>  }

Confirmed to be in line with the ECMA 9.2.


> @@ -871,6 +871,8 @@ JSValue convertQVariantToValue(ExecState* exec,
PassRefPtr<RootObject> root, con
>  
>      if (type == QMetaType::QObjectStar || type == QMetaType::QWidgetStar) {
>	   QObject* obj = variant.value<QObject*>();
> +	   if (!obj)
> +	       return jsNull();
>	   return QtInstance::getQtInstance(obj, root,
QScriptEngine::QtOwnership)->createRuntimeObject(exec);
>      }

Awesome!

Good testing!