Bug 34170 – [Qt] Javascript undefined > 0 returns true on Symbian

• Component: New Bugs
• Status: RESOLVED FIXED

[Janne Koskinen]

Created attachment 47416 [details]
test case

undefined > 0 returns true on Symbian while on other enviroments it returns false.
another failing comparison is undefined == false -> false

Bug is originated from http://bugreports.qt.nokia.com/browse/QTBUG-7423 .

[Laszlo Gombos]

Can this be still reproduced on QtWebKit 4.6.x trunk - just to make sure that this is not another reincarnation of https://bugs.webkit.org/show_bug.cgi?id=31773 (which was not fixed in QtWebkit 4.6.0 final) ?

[Janne Koskinen]

(In reply to comment #1)
> Can this be still reproduced on QtWebKit 4.6.x trunk - just to make sure that
> this is not another reincarnation of
> https://bugs.webkit.org/show_bug.cgi?id=31773 (which was not fixed in QtWebkit
> 4.6.0 final) ?

I had similar thougths that this might be NaN issue. Alas, the fix is in the build that I used and I can still reproduce this.

[Simon Hausmann]

Kent, any idea of this affects QtScript, too?

[Kent Hansen]

(In reply to comment #3)
> Kent, any idea of this affects QtScript, too?

Yes.
The comparison should be semantically equivalent to NaN > 0, since the > operator will try to convert its operands to numbers if they aren't already --> ToNumber(undefined) -> NaN. A quick (in)sanity check would be to see if replacing undefined by NaN in the test case gives the same result.

It looks like this is the same as what's causing http://bugreports.qt.nokia.com/browse/QTBUG-4621, then. For undefined, JSValue::getPrimitiveNumber() will get here:

        ASSERT(isUndefined());
        number = nonInlineNaN();
        value = *this;
        return true;
    }

From JSValue.cpp:

NEVER_INLINE double nonInlineNaN()
{
    return std::numeric_limits<double>::quiet_NaN();
}

Again, see Janne's comment @ http://bugreports.qt.nokia.com/browse/QTBUG-4621.

[Janne Koskinen]

insanity check Number.NaN > 0 returns true. Looks like we have a winner.

[Kent Hansen]

(In reply to comment #5)
> insanity check Number.NaN > 0 returns true. Looks like we have a winner.

So did the Open C guys respond regarding numeric_limits<double>::quiet_NaN() being buggy?

Here's a quick work-around for Qt you might want to try in JSValue::nonInlineNaN:

#if PLATFORM(QT)
    return qQNaN();
#endif

qQNaN and friends are declared in src/corelib/global/qnumeric.h.

I wonder why JSValue is the only place that depends on numeric_limits<double>::quiet_NaN? There's code near the top of JSCell.cpp that constructs NaN and Infinity from raw bits if necessary. It's basically what the qnumeric implementation does as well (see qnumeric_p.h).
Maybe the robust fix would be to make the distinction between quiet NaN and signaling NaN in that code, and change JSValue to use that constant.

[Janne Koskinen]

(In reply to comment #6)
> So did the Open C guys respond regarding numeric_limits<double>::quiet_NaN()
> being buggy?

Long story short. Open C has fixed their part for their next release (1.7), but there is issue with RVCT compiler that will still make this not to work.

"The problem comes from the fact that the following code is not equal in results:
double dnan = std::numeric_limits<double>::quiet_NaN();
int K = 13;
1.  if(!(dnan <= K))
is evaluated to false case;
bool temp = (dnan <= K);
if(!temp)
is evaluated to true case."

Above is happening if -02 optimatisation is used. with -O0 both cases evaluate the same way.

So unfortunately we need to do a workaround until RVCT is getting fixed.

[Janne Koskinen]

Created attachment 47838 [details]
proposed fix for jsvalue.cpp

Fix affects all Qt platforms. alternatively this could be flagged for PLATFORM(QT) && PLATFORM(SYMBIAN).
I don't think there are any projects implementing pure Symbian ports of webkit anymore?

[Darin Adler]

Comment on attachment 47838 [details]
proposed fix for jsvalue.cpp

> +#if PLATFORM(QT)
> +#include <qnumeric.h>
> +#endif

Includes with ifdefs go into a separate paragraph. Not sorted into the normal includes.

The ifdef should definitely be for Symbian rather than for Qt. This patch is all about working around a Symbian bug. And I am also really surprised it's broken!

> +    return qQNaN();

This seems OK, but ideally we'd work around the Symbian bug without requiring Qt here. Is there any other way to do it? For example, does 0.0 / 0.0 work? Or is there an NAN macro on Symbian?

review- but something like this would be fine -- just please consider my comments.

[Ivan De Marino]

Hi. Ivan De Marino.
I'm the (un)lucky guy that first discovered this bug while using QtWebKit on S60. If anyone is interested I can tell you the story that goes from jQuery.ajaxSetup down to discovering the actual comparison problem.

Anyway, let's go to the point.
I can add some meat here saying that the problem is probably not necessarily related with RCVT. Why? Because the place were I first discovered the bug was Symbian WINSCW, not the hardware phone (of course it happens there as well).

For who doesn't know, WINSCW is the Symbian SDK Simulator: everything is just running as Windows threads within the Simulator (that is the Process containing all of them).

Everything there is compiled with x86 compiler, so no RCVT involved anyhow.

The only thing I need to double check is which version of OpenC is inside my WINSCW, but being running the latest Qt 4.6.1 that depends on Open C 1.7 (the one supposed to solve the problem), I guess that is out of the picture as "source of problem".

Unfortunately I'm not experienced enough of the internals of WebKit to just jump in and build a Symbian specific bugfix. At least for now.

[Janne Koskinen]

> Anyway, let's go to the point.
> I can add some meat here saying that the problem is probably not necessarily
> related with RCVT. Why? Because the place were I first discovered the bug was
> Symbian WINSCW, not the hardware phone (of course it happens there as well).

Yes the Open C bug is reproducable both on HW and emulator. The fix that is to be released in next Open C addresses both platforms, but RVCT has a bug that will affect the results so that it won't work even after the patch for all cases depending on optimisation level applied.

Anyways.. I have another attempt coming up shortly after I've doublechecked something :) -> heads back to reading IEEE 754.

[Janne Koskinen]

> The only thing I need to double check is which version of OpenC is inside my
> WINSCW, but being running the latest Qt 4.6.1 that depends on Open C 1.7 (the
> one supposed to solve the problem), I guess that is out of the picture as
> "source of problem".

Open C 1.7 is not released yet. What you have is Open C 1.6 if you don't have internal pre-release from somewhere.

[Ivan De Marino]

(In reply to comment #12)
> > The only thing I need to double check is which version of OpenC is inside my
> > WINSCW, but being running the latest Qt 4.6.1 that depends on Open C 1.7 (the
> > one supposed to solve the problem), I guess that is out of the picture as
> > "source of problem".
> 
> Open C 1.7 is not released yet. What you have is Open C 1.6 if you don't have
> internal pre-release from somewhere.

Yeah, sorry. I got confused with version numbers.

[Ivan De Marino]

I believe that for the sake of completeness, it's good to have here a link back to the originator Bug on Qt Bug Tracker: http://bugreports.qt.nokia.com/browse/QTBUG-7423.

[Janne Koskinen]

Created attachment 48007 [details]
proposed fix for jsvalue.cpp

A new patch using epoc32/include/stdapis/math.h double nanval() - function to get the quietNaN value.
nanval() returns correct non-signalling value 0xfff8 as exp and has double type. There is NAN macro but that ends up calling nanvalf() which due to bug would also return double type quietNaN (0xfff8 instead of 0xffc0), but I opted not to use this if in future the bug will be fixed.

Fix tested on s60 5.0 emulator and 5800 express music.

Why the original std::numeric_limits<double>::quiet_NaN() fails is due to wrongly having big endian set for ARM and WINSCW instead of little endian in _limits.c.
so instead of getting _STLP_DOUBLE_QNAN_REP { 0, 0, 0, 0xfff8 } we get _STLP_DOUBLE_QNAN_REP { 0xfff8, 0, 0, 0 }. This is the most visible bug in Open C part. Open C guys said that there was another issue to this as well.
The last part is the RVCT optimisation issue which is still unresolved.

A task to revert this fix should be made once all fixes in the platform are in.

[Janne Koskinen]

Created attachment 48225 [details]
proposed fix for jsvalue.cpp

Changed PLATFROM macro to OS macro.

[Ivan De Marino]

Something tells me that we are not going to enjoy a proper fix of ARM compiler for this issue. At least, not where we need it.

Let me explain.
For who doesn't know, Symbian OS requires a version of RVCT compiler that is quite old: something around 2.x.

Current version of ARM navigates around 4.x (or even more, not sure).
I managed to contact folks in ARM and I received an answer from Peter Lewin (ARM Technical Support) and he says that:
<blockquote>
...
This has been reported to us, and the problem has been fixed in the latest patch release of RVCT 4.0 (build 697).
http://www.arm.com/support/downloads/info/26545.html
</blockquote>

So, unless my knowledge of the Symbian compilation requirements is faulty, I believe Symbian is not compatible/compilable with such "advanced" version of the ARM compiler.

And, as a consequence, even if the fix arrived, is not going to applicable.

BUT hopefully Symbian will get fully compilable by using "only" GCCE.
:(

[Laszlo Gombos]

Comment on attachment 48225 [details]
proposed fix for jsvalue.cpp

LGTM; r+.

[WebKit Commit Bot]

Comment on attachment 48225 [details]
proposed fix for jsvalue.cpp

Clearing flags on attachment: 48225

Committed r54539: <http://trac.webkit.org/changeset/54539>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

(In reply to comment #19)
> (From update of attachment 48225 [details])
> Clearing flags on attachment: 48225
> 
> Committed r54539: <http://trac.webkit.org/changeset/54539>

Cherry-picked into qtwebkit-4.6 with commit ffae5e11181a3961193fa21ea405851cad714d4b