ClojureScript

Hashing is broken when using iphone 4

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Declined
  • Affects Version/s: 0.0-3308
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    iphone 4 running iOS 7.1.2

Description

On a physical iPhone 4 running iOS 7.1.2, when running the `hash` function too many times at the top level, the result becomes inconsistent.

This only happens when not using the safari webinspector.

I have setup a minimal failing spec here: https://github.com/Jell/iphone-bug

Instructions to reproduce in the README.

I must confess, this took me a very long time to debug! I hope someone can take it from here.

Activity

Hide
Jean-Louis Giordano added a comment - - edited

With the help of my colleague, we narrowed it down further to this:

(ns iphone-bug.core)

(def yyyy (hash "yyyy"))

(dotimes [i 1000] (hash "yyyy"))

(js/alert (str "Before: " yyyy " After: " (hash "yyyy")))
Show
Jean-Louis Giordano added a comment - - edited With the help of my colleague, we narrowed it down further to this:
(ns iphone-bug.core)

(def yyyy (hash "yyyy"))

(dotimes [i 1000] (hash "yyyy"))

(js/alert (str "Before: " yyyy " After: " (hash "yyyy")))
Hide
Jean-Louis Giordano added a comment -

Note: the bug only appears on iPhone 4, not iPhone 4S!

Show
Jean-Louis Giordano added a comment - Note: the bug only appears on iPhone 4, not iPhone 4S!
Hide
David Nolen added a comment -

I believe this is something to do with 32bit vs. 64bit JavaScriptCore. Super low priority but happy to take a patch if someone can devise one.

Show
David Nolen added a comment - I believe this is something to do with 32bit vs. 64bit JavaScriptCore. Super low priority but happy to take a patch if someone can devise one.
Hide
Thomas Heller added a comment -

Sounds like something which was fixed a long time ago and I haven't personally seen since. Maybe a recent change reverted the Math.imul fix?

http://dev.clojure.org/jira/browse/CLJS-839
http://dev.clojure.org/jira/browse/CLJS-830

Show
Thomas Heller added a comment - Sounds like something which was fixed a long time ago and I haven't personally seen since. Maybe a recent change reverted the Math.imul fix? http://dev.clojure.org/jira/browse/CLJS-839 http://dev.clojure.org/jira/browse/CLJS-830
Hide
David Nolen added a comment -

Thomas I'm not sure we solved this for older phones only relatively newer ones with specific versions of Safari. iPhone 4 was ARM Cortex A8, and the 4S was an ARM Cortex A9. Turns out both are 32bit, but I still suspect this is a JITing issue if it disappears with a debugger attached.

Show
David Nolen added a comment - Thomas I'm not sure we solved this for older phones only relatively newer ones with specific versions of Safari. iPhone 4 was ARM Cortex A8, and the 4S was an ARM Cortex A9. Turns out both are 32bit, but I still suspect this is a JITing issue if it disappears with a debugger attached.
Hide
Thomas Heller added a comment -

Just saying that the description exactly matched what I saw when investigating the keyword hashing issue. The problem never appeared with the devtools running and sometimes produced the correct hash without. After adding the imul.js the issue disappeared and clients stopped sending invalid maps. See CLJS-830 for a list of user agents [1] that were problematic before, the issue was with a specific webkit version and seemingly not hardware related (iOS 7.1.2 was the version at the time, user agents don't identify the device though).

Not claiming that this is the same issue, just saying that it sounds very very similar.

[1] https://gist.github.com/thheller/8cae79cabd9ac74958ca

Show
Thomas Heller added a comment - Just saying that the description exactly matched what I saw when investigating the keyword hashing issue. The problem never appeared with the devtools running and sometimes produced the correct hash without. After adding the imul.js the issue disappeared and clients stopped sending invalid maps. See CLJS-830 for a list of user agents [1] that were problematic before, the issue was with a specific webkit version and seemingly not hardware related (iOS 7.1.2 was the version at the time, user agents don't identify the device though). Not claiming that this is the same issue, just saying that it sounds very very similar. [1] https://gist.github.com/thheller/8cae79cabd9ac74958ca
Hide
Jean-Louis Giordano added a comment - - edited

We found a fix that works for us!

We redefine the imul polyfill (cljs/imul.js) with the following:

if(typeof Math.imul == "undefined" || (Math.imul(0xffffffff,5) == 0)) {
    Math.imul = function (a, b) {
      try {
        var ah  = (a >>> 16) & 0xffff;
        var al = a & 0xffff;
        var bh  = (b >>> 16) & 0xffff;
        var bl = b & 0xffff;
        // the shift by 0 fixes the sign on the high part
        // the final |0 converts the unsigned value into a signed value
        return ((al * bl) + (((ah * bl + al * bh) << 16) >>> 0)|0);
      } catch(err) { throw err; }
    }
}

The error is not really coming from inside this function, but adding the try/catch disables JIT optimisation for callers of imul as far as we understand, which solves the hashing issues we saw on Iphone 4.

The errors seems to come from bit operations, in particular the `x|0` operations (corresponding to (int x) calls or bit-or in CLJS).

Infuriating to pin the bug down, because observing intermediate variables will prevent the bug from happening!

Show
Jean-Louis Giordano added a comment - - edited We found a fix that works for us! We redefine the imul polyfill (cljs/imul.js) with the following:
if(typeof Math.imul == "undefined" || (Math.imul(0xffffffff,5) == 0)) {
    Math.imul = function (a, b) {
      try {
        var ah  = (a >>> 16) & 0xffff;
        var al = a & 0xffff;
        var bh  = (b >>> 16) & 0xffff;
        var bl = b & 0xffff;
        // the shift by 0 fixes the sign on the high part
        // the final |0 converts the unsigned value into a signed value
        return ((al * bl) + (((ah * bl + al * bh) << 16) >>> 0)|0);
      } catch(err) { throw err; }
    }
}
The error is not really coming from inside this function, but adding the try/catch disables JIT optimisation for callers of imul as far as we understand, which solves the hashing issues we saw on Iphone 4. The errors seems to come from bit operations, in particular the `x|0` operations (corresponding to (int x) calls or bit-or in CLJS). Infuriating to pin the bug down, because observing intermediate variables will prevent the bug from happening!
Hide
Francis Avila added a comment -

This is going to cause an enormous performance regression.

Show
Francis Avila added a comment - This is going to cause an enormous performance regression.
Hide
Jean-Louis Giordano added a comment -

Well a perf regression for the browsers that do not have a built-in or broken Math/imul, which might not be that big of a deal?

This is good enough for us at least, so we will probably not be investigating this further.

Show
Jean-Louis Giordano added a comment - Well a perf regression for the browsers that do not have a built-in or broken Math/imul, which might not be that big of a deal? This is good enough for us at least, so we will probably not be investigating this further.
Hide
David Nolen added a comment -

Thanks for the bug report good to have monkey-path solution for those that need it. Closing this one.

Show
David Nolen added a comment - Thanks for the bug report good to have monkey-path solution for those that need it. Closing this one.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: