Bug 136508 – Nitro JIT produces incorrect math on 32-bit iOS devices

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Lee Byron]

Please follow along with the current public investigation happening at https://github.com/facebook/immutable-js/issues/92

This Javascript library creates Trie data structures and uses bit-shifting math to operate correctly. A contributor discovered that getting data from a Trie started to fail after a few dozen iterations and reduced to simple test case.

On investigation, it was found that the expression "(1 << ((hash >>> shift) & MASK)" was evaluating to "1" for values hash: 3556498, shift: 0, MASK: 31. This expression should evaluate to "262144". More specifically, "(hash >>> 0)" starts evaluating to 0 after a few dozen passes.

On further investigation to narrow the issue, 64-bit devices such as the iPhone 5S are not affected. Desktops of any kind do not seem to be affected. Alternative iOS browsers like Chrome are not affected (they do not have JIT). Plugging the device in to use Safari's debugger causes the error to vanish. Adding in too much logging causes the error to vanish.

This seems to narrow the cause down to a bug with the JIT'd code. Because changing the code slightly can cause the error to vanish, we haven't been able to create a more minimal failing case.

Steps to Reproduce:
Please read the steps at https://github.com/facebook/immutable-js/issues/92

Opening this test case http://codepen.io/conradz/pen/bKClx in Safari on a 32bit iOS device

Expected Results:
Test case works correctly, each iteration of the loop yields the same result

Actual Results:
Test case fails. First few dozen iterations of the loop succeed, but then the loop fails, presumably after a JIT optimization has taken place

Version:
iOS 7.1.2

[Lee Byron]

Also filed RDAR #18220947

[Michael Saboff]

I have tried to reproduce this issue on an iPad mini, iPad 3rd gen and iPad 4th gen all running 7.1.2 using the websites http://codepen.io/conradz/pen/bKClx and http://codepen.io/conradz/full/bKClx/ and I am unable.  I always get "done".

My agent is Mozilla/5.0 (iPhone; CPU iPhone OS 7_1_2 like Mac OS X) AppleWebKit/537.51.2 (KHTML, like Gecko) Version/7.0 Mobile/11D257 Safari/9537.53.

Is there another website that reproduces the bug?

[Lee Byron]

Are you hooked up to a Safari debugger when loading the page? That seems to prevent the issue from appearing. It was a 100% repro on 32bit iOS devices for the handful of us investigating this.

It seems that small changes to the function body can also result in the error evaporating, but this URL was a consistent repro for us.

[Lee Byron]

I also forked the codepen at http://codepen.io/anon/pen/ElLah to start logging more and got a repro

[Michael Saboff]

(In reply to comment #4)
> I also forked the codepen at http://codepen.io/anon/pen/ElLah to start logging more and got a repro

I am not using the Safari debugger (Web Inspector),  I am just using Safari directly.  Neither the original nor this URL repros the problem.

We think the defect is in one of the JIT engines which we tier up to after executing code repeatedly.  That fully explains why it fails after so many iterations.

It could be that I need to sign up / log in.  Can you repro without being logged in?

[Lee Byron]

I wasn't logged in when getting this to repro. 

I agree with your theory. For those running it, some see it fail at iteration 37, others at 25. There's some externality that's causing one of these two (or no repro at all). Also, when trying to narrow this down, unrelated changes such as adding a console.log would cause it to stop appearing. 

I was only able to narrow it down to an `n >>> 0`. It also failed with >>. If the second operand was not 0, the error vanished again. I patched the issue with `s === 0 ? n : n >>> s`, so my guess is that this is a JIT optimization around >> always being applied to a 0 value, but my guess is that it's incorrectly looking at the second operand instead of the first when short circuiting to 0.

[Michael Saboff]

(In reply to comment #6)
> I wasn't logged in when getting this to repro. 
> 
> I agree with your theory. For those running it, some see it fail at iteration 37, others at 25. There's some externality that's causing one of these two (or no repro at all). Also, when trying to narrow this down, unrelated changes such as adding a console.log would cause it to stop appearing. 
> 
> I was only able to narrow it down to an `n >>> 0`. It also failed with >>. If the second operand was not 0, the error vanished again. I patched the issue with `s === 0 ? n : n >>> s`, so my guess is that this is a JIT optimization around >> always being applied to a 0 value, but my guess is that it's incorrectly looking at the second operand instead of the first when short circuiting to 0.

So I looked at the page source for http://codepen.io/anon/pen/ElLah, and I believe it has the workaround:
1381:   var bit = (1 << ((shift === 0 ? hash : hash >>> shift) & MASK));

The other websites said to reproduce that I checked seem to have the workaround as well.

Can you either provide a website without the workaround or attach a tar file with all the files I would need to make a local "failing" website?

[Lee Byron]

OHHH, crap of course. 

Those demos are pointed at a master build of immutable and I've already committed the work around. If you use tag 2.0.16 instead of master then I'm sure you'll see the repro. Sorry for the moving target.

[Lee Byron]

Ok, sorry about that. I've edited http://codepen.io/anon/pen/ElLah such that it loads https://rawgit.com/facebook/immutable-js/2.0.17/dist/Immutable.js which is before my workaround for this issue and should now repro on a 32bit iOS device.

[Michael Saboff]

(In reply to comment #9)
> Ok, sorry about that. I've edited http://codepen.io/anon/pen/ElLah such that it loads https://rawgit.com/facebook/immutable-js/2.0.17/dist/Immutable.js which is before my workaround for this issue and should now repro on a 32bit iOS device.

NOW I can repro.  Thanks.

[Michael Saboff]

I have verified that the top WebKit trunk JavaScript does not have this issue, including verifying that 32 bit iOS devices work correctly with the updated test URL.

Given that the issue exists in iOS 7.1.2, it is suggested that the workaround be used at least in the near term.