[Qt] QScriptValue::equals asserts

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jędrzej Nowacki]

There is an assert in the QScriptValue::equals benchmarks. 


#0  0xb7fe2424 in __kernel_vsyscall ()
#1  0xb69a7751 in *__GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0xb69aab82 in *__GI_abort () at abort.c:92
#3  0xb6ca946b in qt_message_output (msgType=QtFatalMsg, buf=0x812ed88 "ASSERT: \"isStringBased()\" in file /home/nierob/dev/WebKit-script/JavaScriptCore/qt/api/qscriptvalue_p.h, line 705") at global/qglobal.cpp:2271
#4  0xb6ca962a in qt_message (msgType=QtFatalMsg, msg=0xb6e589c4 "ASSERT: \"%s\" in file %s, line %d", ap=0xbfffe6f4 ",h\344\267`f\344\267\301\002") at global/qglobal.cpp:2317
#5  0xb6ca9a47 in qFatal (msg=0xb6e589c4 "ASSERT: \"%s\" in file %s, line %d") at global/qglobal.cpp:2500
#6  0xb6ca8fcc in qt_assert (assertion=0xb7e4682c "isStringBased()", file=0xb7e46660 "/home/nierob/dev/WebKit-script/JavaScriptCore/qt/api/qscriptvalue_p.h", line=705) at global/qglobal.cpp:2016
#7  0xb7c8bb04 in QScriptValuePrivate::equals (this=0x80f6fe0, other=0x80f6fe0) at /home/nierob/dev/WebKit-script/JavaScriptCore/qt/api/qscriptvalue_p.h:705
#8  0xb7c89843 in QScriptValue::equals (this=0xbfffe78c, other=...) at /home/nierob/dev/WebKit-script/JavaScriptCore/qt/api/qscriptvalue.cpp:598
#9  0x0804d9b3 in tst_QScriptValue::equals (this=0xbffff16c) at /home/nierob/dev/WebKit-script/JavaScriptCore/qt/benchmarks/qscriptvalue/tst_qscriptvalue.cpp:407
#10 0x0804e123 in tst_QScriptValue::qt_metacall (this=0xbffff16c, _c=QMetaObject::InvokeMetaMethod, _id=43, _a=0xbfffe844) at ./tst_qscriptvalue.moc:179
#11 0xb6ddc6e4 in QMetaObject::metacall (object=0xbffff16c, cl=QMetaObject::InvokeMetaMethod, idx=47, argv=0xbfffe844) at kernel/qmetaobject.cpp:237
#12 0xb6dded79 in QMetaMethod::invoke (this=0xbfffebb8, object=0xbffff16c, connectionType=Qt::DirectConnection, returnValue=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...) at kernel/qmetaobject.cpp:1581
#13 0xb6dde2af in QMetaObject::invokeMethod (obj=0xbffff16c, member=0x810d978 "equals", type=Qt::DirectConnection, ret=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...) at kernel/qmetaobject.cpp:1148
#14 0xb7c3ab7c in QMetaObject::invokeMethod (obj=0xbffff16c, member=0x810d978 "equals", type=Qt::DirectConnection, val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...)
    at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs.h:408
#15 0xb7c3846e in qInvokeTestMethodDataEntry (slot=0x810d978 "equals") at qtestcase.cpp:1244
#16 0xb7c389ca in qInvokeTestMethod (slotName=0x804f733 "equals()", data=0x0) at qtestcase.cpp:1352
#17 0xb7c38fad in qInvokeTestMethods (testObject=0xbffff16c) at qtestcase.cpp:1497
#18 0xb7c39727 in QTest::qExec (testObject=0xbffff16c, argc=2, argv=0xbffff244) at qtestcase.cpp:1716
#19 0x0804dd4a in main (argc=2, argv=0xbffff244) at /home/nierob/dev/WebKit-script/JavaScriptCore/qt/benchmarks/qscriptvalue/tst_qscriptvalue.cpp:441

[Jędrzej Nowacki]

Created attachment 61651 [details]
Fix v1

I don't provide an additional tests, the crash is in our benchmarks so in a teory it is tested. I think we should test the case using existing tests, so I created bug 42366.

[Kent Hansen]

Comment on attachment 61651 [details]
Fix v1

> +inline bool JSCompare(qsreal a, qsreal b)
> +{
> +    return a == b
> +        || (qIsInf(a) && qIsInf(b))
> +        || (qIsNaN(a) && qIsNaN(b));
> +}

The Inf and NaN parts aren't correct. NaN should not be equal to NaN. -Inf should not be equal to +Inf. So this helper function isn't needed, == can be used inplace.
Instead of Q_ASSERT(false) in the default case, I'd prefer a Q_ASSERT_X(false, "equals()", Not all states are included in the previous switch statement."), like existing switches do.
Otherwise it looks good.

[Jędrzej Nowacki]

Created attachment 62421 [details]
Fix v2

New version:
- JSCompare function was removed
- The assert was replaced by a more verbose one.

[WebKit Commit Bot]

Comment on attachment 62421 [details]
Fix v2

Clearing flags on attachment: 62421

Committed r63980: <http://trac.webkit.org/changeset/63980>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.