Bug 39834 – [Qt] JavaScript logical expressions

• Component: New Bugs
• Status: RESOLVED WORKSFORME

[Jay Tucker]

There seems to be a bug affecting JavaScript logical expressions which appears only on Symbian hardware. It occurs when a non-empty String variable is compared to a Boolean variable (which admittedly is not a good thing to do, but people do it nonetheless).

var str = "hello";
if (str == false) // Does this expression evaluate to true or false?
{
// do stuff here
}

On all the desktop browsers I tried as well as on Anomaly running on the Symbian emulator, the expression evaluated to false, so the stuff inside the if-block was not executed. However, on a Symbian device, the expression evaluated to true, so the stuff inside the if-block was executed.

I've attached a tiny HTML file that contains some test cases for these types of logical expressions.

[Jay Tucker]

Created attachment 57237 [details]
HTML file containing some test cases for logical expressions

[Simon Hausmann]

Janne, have you seen something like this before? Could this be a miscompilation?

[Janne Koskinen]

I don't think it is a miscompilation. I have a theory what this could be. I'll test it out to see what happens.

[Janne Koskinen]

Can you clarify what is wrong?
I ran the test case on IE6,FF,Chrome and my own webview client on top of Qt 4.7 in hardware (5800) and emulator (5.0) and all returned following results:

s=
s == true false
s == false true
s.length == true false
s.length == false true

s=Hello!
s == true false
s == false false
s.length == true false
s.length == false false


So your test if (str == false) evaluated false on all platforms and browsers that I tested.

I added following to the test:

s = "Hello!";
if (s == false)
{
	alert('false');
}
if (s == true)
{
	alert('true');
}

This was to verify that RVCT won't do anything funny with the temp variables which was my first assumption what is wrong but nope the result is still the same i.e. no alert popups was shown on Nokia 5800.

[Jay Tucker]

Hmmm. Maybe it's been fixed in Qt 4.7? We were testing on 4.6.2 and a pre-release version of 4.6.3 on an N97.

[Janne Koskinen]

(In reply to comment #5)
> Hmmm. Maybe it's been fixed in Qt 4.7? We were testing on 4.6.2 and a pre-release version of 4.6.3 on an N97.

Took 4.6.3 release and compiled QtWebkit2.0 on top of it and can reproduce this now. Additionally this configuration is really really slow. loading attached local html page takes around ~1min to render. I think there was a bug about this already..

[Janne Koskinen]

Weird stuff. Using 4.7 and building QtWebkit2.0 on top of it the bug doesn't reproduce. Not this makes me wonder at lot of things :) Maybe it is afterall about the build process ... I'll try using sbsv1 instead of sbsv2.

[Misha]

It looks like the actual comparison is happening in 
ALWAYS_INLINE bool JSValue::equalSlowCaseInline(ExecState* exec, JSValue v1, JSValue v2)

In the beginning of that function we are checking both operands if they are strings:

bool s1 = v1.isString();
bool s2 = v2.isString();

Then later we do following:
 if (s1 || s2) {
     double d1 = v1.toNumber(exec);
     double d2 = v2.toNumber(exec);
     return d1 == d2;
}

So v1 is the JSString and v1.toNumber(exec) returns NaN for "Hello!". V2 is false and v2.toNumber(exec) returns 0.0. 
So depending on how NaN is implemented we might get different results. On Symbian it defined as nanval(void ) while on the other platforms it's a quiet_NaN().

What I don't understand is that on emulator d1 (from the code snippet above) equals NaN which is 0.0 and so d2. So d1 ==d2 is true. 
But on emulator we are getting false.

[Laszlo Gombos]

This could be a miscompilation for RVCT - that is why the problem does not show on the emulator. Here is two more of this that was fixed before (although on both of ChangeSets I think the correct guard is COMPILER(RVTC).

http://trac.webkit.org/changeset/51307
http://trac.webkit.org/changeset/54539

[Janne Koskinen]

Cannot reproduce this using SBSv2 and building 4.6 from git repo. I can reproduce this only by using pre-built 4.6.3 and building webkit on top of that. So my money is on building with abld on 4.6.3 release causes wrong math header to be picked up.
Each test round-trip takes half a day when having to build everything from ground up so be patient with this one. I know I have to :S

[Janne Koskinen]

just quick update for workaround:

If you build 4.6.3 and QtWebkit 2.0 using raptor you won't get this bug.
Same exact source from clean built with abld causes the bug.

I've had few dead ends, one good one as it fixed QtScript a bit. I cloned my environment and built another one with raptor and another one with abld and now comparing them side by side. nanval thing is still my major suspect. MMP files only differ by tmp directories we had to create for abld source lookup (which propably picks up a wrong file).

--
A New issue needs to be raised based on investigation. We add flags based on config(release, debug|release) .. this doesn't work and we end up with debug all the time i.e. no -OTime -O3 optimisation and no QT_NO_DEBUG

[Simon Hausmann]

(In reply to comment #11)
> just quick update for workaround:
> 
> If you build 4.6.3 and QtWebkit 2.0 using raptor you won't get this bug.
> Same exact source from clean built with abld causes the bug.

That would indeed explain why we saw this problem with some builds and not with others. I shall try to ban the use of abld from my builds :)

> I've had few dead ends, one good one as it fixed QtScript a bit. I cloned my environment and built another one with raptor and another one with abld and now comparing them side by side. nanval thing is still my major suspect. MMP files only differ by tmp directories we had to create for abld source lookup (which propably picks up a wrong file).

Do you think it's worth continuing the investigation?

From your analysis - which makes sense to me - it sounds like we should close this bug as INVALID (as there's no bug in the WebKit code) and instead make sure that Qt and WebKit packages are never build with abld. That might require backporting raptor in some of the Qt packaging SDKs.

What do you think?

> --
> A New issue needs to be raised based on investigation. We add flags based on config(release, debug|release) .. this doesn't work and we end up with debug all the time i.e. no -OTime -O3 optimisation and no QT_NO_DEBUG

Ah, I guess that's bug 40840 :)

[Janne Koskinen]

(In reply to comment #12)
> From your analysis - which makes sense to me - it sounds like we should close >this bug as INVALID (as there's no bug in the WebKit code) and instead make >sure that Qt and WebKit packages are never build with abld. That might require >backporting raptor in some of the Qt packaging SDKs.

This would be ideal but since we chose to support from 3.x onwards I don't think we can drop abld support on a whim. We already tried dropping abld support for Qt for Symbian port... it will eventually, but how long that will take I don't know.

> > A New issue needs to be raised based on investigation. We add flags based >>on config(release, debug|release) .. this doesn't work and we end up with >>debug all the time i.e. no -OTime -O3 optimisation and no QT_NO_DEBUG
> 
> Ah, I guess that's bug 40840 :)

Nope :S 40840 just deals with Symbian single binary target directory and the d in the name.
This issue is that we end up building our releases as debug config.

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> ... we end up with >>debug all the time i.e. no -OTime -O3 optimisation and no QT_NO_DEBUG
> > 
> > Ah, I guess that's bug 40840 :)
> 
> Nope :S 40840 just deals with Symbian single binary target directory and the d in the name.
> This issue is that we end up building our releases as debug config.

Have you filed another bug for that?

[Janne Koskinen]

Created attachment 61009 [details]
simplified test case

Issues is when building with abld that string is not NaN.

[Simon Hausmann]

I can now confirm this issue.

[Simon Hausmann]

Janne's suspicion is correct, NaN (the constant) is picked up from the wrong header file when building with abld.

[Janne Koskinen]

(In reply to comment #17)
> Janne's suspicion is correct, NaN (the constant) is picked up from the wrong header file when building with abld.

Yep, and I have already a workaround that does work.. but doesn't really explain what goes wrong.

[Misha]

And I still don't understand why the expression (s == false), where s is "Hello!", is evaluated to false. As least on emulator it looks like it should be true (see comment #8) but it's not. Can anybody explain this?

[Janne Koskinen]

(In reply to comment #19)
> And I still don't understand why the expression (s == false), where s is "Hello!", is evaluated to false. As least on emulator it looks like it should be true (see comment #8) but it's not. Can anybody explain this?

Carbide debugger variable view wrongly displays 0.0 as the value. This is propably a rounding error. If you check the real value from memory view you will see bit pattern : 00000000  0000F87F = quiet NaN, double, little endian repr. Also if you click on the variable / hover on top of it you'll see string "nan". Hmm.. should file a bug about that.

[Janne Koskinen]

Created attachment 61654 [details]
ugly workaround

When using abld extern const double NaN is 0 at the point of use in UString.cpp. Value is assigned in JSCell.cpp, doesn't matter what value is assigned to it.
Issue is reproducible when we have jscore built as static library. 
Reason why it happens in abld and not in raptor is bit mystery. I ran object dump with fromelf in both build environments and extern name can be found in both cases from JSCell.o and in disassembly of UString.o I see no difference in data addressing. In abld name is linked from intermediate qtwebkit.in file which doesn't exist in raptor so I think this is where the issue is.

I'll attach my really ugly hack as this is marked as a blocker for release and I'm heading for my vacation.

Real fix in Webkit would be to unify NaN usage in both webcore and jscore. Currently NaN definitions are spread all over webkit and they don't even match each other. What I've read from code NaN usage in webkit is always quiet NaN and a single definition would be sufficient e.g. in wtf/MathExtras.h.
As a bonus we would get rid of this bug.

[Simon Hausmann]

Let's face it, this is critical only for older Symbian platforms which are becoming less relevant every day, with the rise of Symbian3.

If we can find a solution for it in the future, let's include it.

[Janne Koskinen]

Closing this as there is no support for SBSv1 in Symbian Foundation bugzilla.
If someone is interested in pursuing this e.g. in Forum Nokia send me email and I can help in creating isolated test case.