Last Comment Bug 658968 - javascript ternary operator evaluation bug
: javascript ternary operator evaluation bug
Status: VERIFIED FIXED
fixed-in-tracemonkey
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
:
Mentors:
Depends on:
Blocks: Jaeger
  Show dependency treegraph
 
Reported: 2011-05-23 04:05 PDT by ms
Modified: 2011-07-12 00:49 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
+
fixed


Attachments
test script (1.68 KB, text/html)
2011-05-24 01:43 PDT, ms
no flags Details
screen output (66.82 KB, application/octet-stream)
2011-05-24 01:44 PDT, ms
no flags Details
Patch (2.34 KB, patch)
2011-05-24 05:21 PDT, Jan de Mooij [:jandem]
dvander: review+
dmandelin: approval‑mozilla‑aurora+
dvander: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Patch (2.37 KB, patch)
2011-05-25 08:19 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review

Description ms 2011-05-23 04:05:23 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Result of expression is allways '1' ('element' is checkbox):

tarr[tidx] = element.checked == true ? '1' : '0';

however, if-else construction works well:

if(element.checked == true)   
  tarr[tidx] = '1';
else  
  tarr[tidx] = '0';

Firefox/3.6.3 works with both constructions well.


Reproducible: Always
Comment 1 Benjamin Smedberg [:bsmedberg] 2011-05-23 05:34:41 PDT
Can you write a complete minimal testcase? I tried this with:

<html>
<body>

<input type="checkbox"id="foo">

<script type="text/javascript">
var bar = document.getElementById('foo').checked == true ? '1' : '0';
alert(bar)
</script>

</body>
</html>

and it works correctly.

This is not a security-sensitive issue, so please don't abuse that flag.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-05-23 06:10:51 PDT
Heck, a testcase of any sort, even nonminimal, would be useful.  I also can't reproduce this problem.
Comment 3 Nicholas Nethercote [:njn] 2011-05-23 19:43:38 PDT
Closing.  ms, please reopen if you can post a test-case that shows the problem.  Thanks!
Comment 4 ms 2011-05-24 01:43:04 PDT
Created attachment 534695 [details]
test script
Comment 5 ms 2011-05-24 01:44:02 PDT
Created attachment 534696 [details]
screen output
Comment 6 ms 2011-05-24 02:06:34 PDT
Testcase and screen outputs added. Change of any checkbox outputs current status of all checkboxes. Each additional change shows incorect checkboxes status.
Comment 7 Jan de Mooij [:jandem] 2011-05-24 04:14:54 PDT
Thanks ms! This is a bug in JM, introduced in FF4.
Comment 8 Jan de Mooij [:jandem] 2011-05-24 05:21:23 PDT
Created attachment 534736 [details] [diff] [review]
Patch

The syncAndForgetEverything in emitStubCmp could clobber Registers::ReturnReg. This also fixes a similar problem in jsop_relational_double.

The TI-branch does not have this problem because it does not sync between the stub call and the branch. Do we want to wait for the TI merge or land this in the meantime? Maybe for FF 6?
Comment 9 Benjamin Smedberg [:bsmedberg] 2011-05-24 06:06:12 PDT
This looks safe and important enough that I'd take it for FF5 beta if reviews are completed promptly.
Comment 10 David Anderson [:dvander] 2011-05-24 12:48:47 PDT
Comment on attachment 534736 [details] [diff] [review]
Patch

Nice catch.
Comment 11 Jan de Mooij [:jandem] 2011-05-25 08:16:35 PDT
http://hg.mozilla.org/tracemonkey/rev/68bc1acb7f3e

I don't have level 3 commit access, so I'll need somebody to push this to mozilla-aurora and mozilla-beta (if it's green on the TM tinderbox).
Comment 12 Jan de Mooij [:jandem] 2011-05-25 08:19:18 PDT
Created attachment 535068 [details] [diff] [review]
Patch

This is the patch that landed on TM (I added "and jsop_relational_double" to the commit message).
Comment 13 David Mandelin [:dmandelin] 2011-05-26 12:39:40 PDT
http://hg.mozilla.org/mozilla-beta/rev/c9ed832189df
Comment 14 David Mandelin [:dmandelin] 2011-05-26 12:42:06 PDT
http://hg.mozilla.org/mozilla-aurora/rev/747dc81e161b
Comment 15 Simona B [:simonab ] 2011-05-31 05:27:40 PDT
Mozilla/5.0 (Windows NT 6.1; rv:5.0) Gecko/20100101 Firefox/5.0

I verified this issue and it looks as being fixed, but the status of this bug is Assigned. Shouldn't the status be: Resolved Fixed?
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-31 06:05:12 PDT
It's waiting for a merge from the tracemonkey repository to the mainline mozilla-central repository before it's marked fixed.  The bug status refers to the status on the trunk, the branches have their own status-firefoxN flags (which are correctly set to fixed).
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2011-07-05 10:09:57 PDT
It looks like this is on m-c and has been for a while.  Is there a reason the bot didn't mark this fixed?
Comment 18 Chris Leary [:cdleary] (not checking bugmail) 2011-07-05 21:41:03 PDT
There were a few times I marked it by the tbpl log on the merge push and it seems like that misses some things.

http://hg.mozilla.org/mozilla-central/rev/68bc1acb7f3e
Comment 19 Simona B [:simonab ] 2011-07-12 00:49:34 PDT
Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110711 Firefox/8.0a1

Verified issue using the test case from Comment 4 on Windows 7.

Setting resolution to VERIFIED FIXED.

Note You need to log in before you can comment on or make changes to this bug.