Bug 42363 - [Qt] QScriptValue::equals asserts
Summary: [Qt] QScriptValue::equals asserts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jędrzej Nowacki
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 31863
  Show dependency treegraph
 
Reported: 2010-07-15 06:12 PDT by Jędrzej Nowacki
Modified: 2010-07-23 09:11 PDT (History)
4 users (show)

See Also:


Attachments
Fix v1 (3.63 KB, patch)
2010-07-15 06:56 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff
Fix v2 (3.01 KB, patch)
2010-07-23 06:25 PDT, Jędrzej Nowacki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jędrzej Nowacki 2010-07-15 06:12:24 PDT
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
Comment 1 Jędrzej Nowacki 2010-07-15 06:56:26 PDT
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.
Comment 2 Kent Hansen 2010-07-16 08:10:33 PDT
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.
Comment 3 Jędrzej Nowacki 2010-07-23 06:25:39 PDT
Created attachment 62421 [details]
Fix v2

New version:
- JSCompare function was removed
- The assert was replaced by a more verbose one.
Comment 4 WebKit Commit Bot 2010-07-23 09:11:03 PDT
Comment on attachment 62421 [details]
Fix v2

Clearing flags on attachment: 62421

Committed r63980: <http://trac.webkit.org/changeset/63980>
Comment 5 WebKit Commit Bot 2010-07-23 09:11:08 PDT
All reviewed patches have been landed.  Closing bug.