907169 - On big-endian platform, methodjit stricteq array element comparison always false (except -127)

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[t_mrc-ct]

User Agent: Mozilla/5.0 (Macintosh; PPC Mac OS X 10.5; rv:17.0) Gecko/20130805 Firefox/17.0 TenFourFox/G5 (Nightly/Aurora)
Build ID: 20130805171245

Steps to reproduce:

On xpcshell with option -m, run following script:

let a=new Array(20), b=new Array(20);
for(let i=0; i < a.length; i++) {a[i] = i; b[i] = -127;}
for(let i=0; i < a.length; i++) {a[i] === a[i];}
for(let i=0; i < b.length; i++) {b[i] === b[i];}
for(let i=0; i < b.length; i++) {a[i] === b[i];}
for(let i=0; i < b.length; i++) {b[i] === a[i];}


Actual results:

$ ./xpcshell -m
js> let a=new Array(20), b=new Array(20);
js> for(let i=0; i < a.length; i++) {a[i] = i; b[i] = -127;}
-127
js> for(let i=0; i < a.length; i++) {a[i] === a[i];}
false
js> for(let i=0; i < b.length; i++) {b[i] === b[i];}
true
js> for(let i=0; i < b.length; i++) {a[i] === b[i];}
false
js> for(let i=0; i < b.length; i++) {b[i] === a[i];}
true


Expected results:

$ ./xpcshell -m
js> let a=new Array(20), b=new Array(20);
js> for(let i=0; i < a.length; i++) {a[i] = i; b[i] = -127;}
-127
js> for(let i=0; i < a.length; i++) {a[i] === a[i];}
true
js> for(let i=0; i < b.length; i++) {b[i] === b[i];}
true
js> for(let i=0; i < b.length; i++) {a[i] === b[i];}
false
js> for(let i=0; i < b.length; i++) {b[i] === a[i];}
false

Comment 1

[t_mrc-ct]

Attachment: patch for methodjit stricteq bug

On little-endian(32bit), jsval_layout defined as {payload; tag;}.
On big-endian(32bit) ,   jsval_layout defined as {tag; payload;}.

On little-endian platform, accessing address without masm.payloadOf() does not cause bug because there is payload data.
But on big-endian, there is tag data (tag of int32 is -127). So stricteq compares array element to -127.

Comment 2

[Nicolas B. Pierron [:nbp]]

JaegerMonkey (methodjit directory) does not exists anymore, not even in release builds, except in b2g18 and in esr17.

What branch are you working on?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> What branch are you working on?

(In reply to t_mrc-ct from comment #0)
> User Agent: Mozilla/5.0 (Macintosh; PPC Mac OS X 10.5; rv:17.0)
> Gecko/20130805 Firefox/17.0 TenFourFox/G5 (Nightly/Aurora)

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 792813 [details] [diff] [review]
patch for methodjit stricteq bug

Review of attachment 792813 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 792813 [details] [diff] [review]
patch for methodjit stricteq bug

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Fixes a JIT bug on PowerPC processors.

> User impact if declined: 
Broken websites on PowerPC hardware.

> Fix Landed on Version:
N/A, JaegerMonkey is disabled in Firefox 23 and has been removed in Firefox 24.

> Risk to taking this patch (and alternatives if risky): 
Low risk. Should have no effect on x86/x64/ARM platforms.

> String or UUID changes made by this patch: 
None.

Comment 6

[Jan de Mooij [:jandem]]

CC'ing Cameron from TenFourFox.

Comment 7

[Cameron Kaiser [:spectre]]

Yes, that looks correct. Thanks, t_mrc-ct!

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 792813 [details] [diff] [review]
patch for methodjit stricteq bug

This does not meet ESR landing criteria (https://wiki.mozilla.org/Release_Management/ESR_Landing_Process) and if this is a case of something being disabled in FF24, we'll be putting out our first ESR24 on this coming cycle so that will be a viable workaround for any orgs impacted by this on the ESR17 branch - migrating to ESR24 is enforced after two release cycles so this will be moot in 2 more cycles regardless.

Comment 9

[Jan de Mooij [:jandem]]

OK, marking this WONTFIX then per comment 8.

Comment 10

[Cameron Kaiser [:spectre]]

We'll adopt this at the local level then. t_mrc-ct, this will be in the 17.0.9 sets; see http://code.google.com/p/tenfourfox/issues/detail?id=239