ClojureScript

Keyword hashing mobile safari (ios7) w/ advanced compilation

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None

Description

I've been debugging some really funky behavior from mobile safari and found the root cause to be keyword hashing.

Version >= 2261 seems to be affected. Please see https://github.com/ckarlsen84/cljs-keyword-hashtest for a quick and dirty minimal test app (`lein ring server`).

Output from some of my runs: https://gist.github.com/ckarlsen84/4c16684c1f9e5ce26711

It seems to only happen with advanced compilation and mobile safari.

Activity

Hide
Thomas Heller added a comment -

Duplicate of CLJS-830. Nice to have a better example, was way too hard to reproduce in my app.

Show
Thomas Heller added a comment - Duplicate of CLJS-830. Nice to have a better example, was way too hard to reproduce in my app.
Hide
David Nolen added a comment - - edited

This makes me think it's an operator precedence issue. But who knows, will look into it. Oh can we get details on the hardware? I was actually unable to repro this at least in Thomas Heller's case on an iPhone 5S running iOS 7.

Thanks for the minimal case!

Show
David Nolen added a comment - - edited This makes me think it's an operator precedence issue. But who knows, will look into it. Oh can we get details on the hardware? I was actually unable to repro this at least in Thomas Heller's case on an iPhone 5S running iOS 7. Thanks for the minimal case!
Hide
Christian Karlsen added a comment -

I ran the tests with an iPhone4 (MC603KN/A).

Show
Christian Karlsen added a comment - I ran the tests with an iPhone4 (MC603KN/A).
Hide
David Nolen added a comment -

Christian, am I correct in interpreting the results as actually hashing incorrectly only after several initial runs?

Show
David Nolen added a comment - Christian, am I correct in interpreting the results as actually hashing incorrectly only after several initial runs?
Hide
David Nolen added a comment - - edited

In order to investigate, this ticket needs more information. Specifically it would be useful to know if native Math.imul or the shim is being used and to confirm whether the behavior occurs only after multiple runs.

Show
David Nolen added a comment - - edited In order to investigate, this ticket needs more information. Specifically it would be useful to know if native Math.imul or the shim is being used and to confirm whether the behavior occurs only after multiple runs.
Hide
Christian Karlsen added a comment -

Only happens after multiple runs (usually after 10-12 runs iirc). I'm travelling atm but I'll do some more tests during the weekend.

Easiest way to test if i'm using the shim or not?

Christian

Show
Christian Karlsen added a comment - Only happens after multiple runs (usually after 10-12 runs iirc). I'm travelling atm but I'll do some more tests during the weekend. Easiest way to test if i'm using the shim or not? Christian
Hide
David Nolen added a comment - - edited

We def a imul fn based on the presence and correctness of js/Math.imul. Just do imul.toString() or something like that to figure out what you have.

The multiple runs thing makes me suspect the bug only manifests when JavaScriptCore JIT (or more efficient interpreters) kick in. This would also explain why people can't repro with the debugger attached.

Show
David Nolen added a comment - - edited We def a imul fn based on the presence and correctness of js/Math.imul. Just do imul.toString() or something like that to figure out what you have. The multiple runs thing makes me suspect the bug only manifests when JavaScriptCore JIT (or more efficient interpreters) kick in. This would also explain why people can't repro with the debugger attached.
Hide
David Nolen added a comment - - edited

I've attached a patch (which I am unable to test since I don't have a suitable device), it would be great if anyone encountering this issue could test the patch, thanks.

Show
David Nolen added a comment - - edited I've attached a patch (which I am unable to test since I don't have a suitable device), it would be great if anyone encountering this issue could test the patch, thanks.
Hide
Thomas Heller added a comment -

I did not try the patch, but I used the imul.js file and just included it in my HTML before ANY other generated javascript. Which should have the same effect as the patch.

It seems to fix the issue, but I'm too tired to do a complete check. Will do a complete check tommorrow. Looks promising though.

Show
Thomas Heller added a comment - I did not try the patch, but I used the imul.js file and just included it in my HTML before ANY other generated javascript. Which should have the same effect as the patch. It seems to fix the issue, but I'm too tired to do a complete check. Will do a complete check tommorrow. Looks promising though.
Hide
Thomas Heller added a comment -

I had the fix deployed over night, but it seems clients are still sending me maps with duplicate keys which means it doesn't work.

Will investigate later.

Show
Thomas Heller added a comment - I had the fix deployed over night, but it seems clients are still sending me maps with duplicate keys which means it doesn't work. Will investigate later.
Hide
Kris Jenkins added a comment - - edited

I've tried this test-case on an iPhone 5 (MD299B/A), and it breaks as expected. With the patch against 4b81710, it works.

Show
Kris Jenkins added a comment - - edited I've tried this test-case on an iPhone 5 (MD299B/A), and it breaks as expected. With the patch against 4b81710, it works.
Hide
Kris Jenkins added a comment -

Yep, this patch also fixes the bug we were seeing on our production builds under 2311. Woo-hoo!

Show
Kris Jenkins added a comment - Yep, this patch also fixes the bug we were seeing on our production builds under 2311. Woo-hoo!
Hide
David Nolen added a comment -

Kris thanks for the feedback. Looking forward to hearing if Thomas Heller sees positive results after further investigation.

Show
David Nolen added a comment - Kris thanks for the feedback. Looking forward to hearing if Thomas Heller sees positive results after further investigation.
Hide
Thomas Heller added a comment -

So far I have not been able to reproduce the bug when the fix is applied.

I forgot to include the fix on some pages which explains why I was still getting invalid maps from Safari clients.

Will keep an eye on the log but so far it seems to be resolved.

Show
Thomas Heller added a comment - So far I have not been able to reproduce the bug when the fix is applied. I forgot to include the fix on some pages which explains why I was still getting invalid maps from Safari clients. Will keep an eye on the log but so far it seems to be resolved.
Hide
David Nolen added a comment -

Thanks everyone for the help on this one, fixed in master and I cut a new release 0.0-2322 https://github.com/clojure/clojurescript/commit/3c0b8e71e09a971a07e3eef70bd74df6f1104d92

Show
David Nolen added a comment - Thanks everyone for the help on this one, fixed in master and I cut a new release 0.0-2322 https://github.com/clojure/clojurescript/commit/3c0b8e71e09a971a07e3eef70bd74df6f1104d92
Hide
Kris Jenkins added a comment -

Fantastic! Thanks David.

Show
Kris Jenkins added a comment - Fantastic! Thanks David.

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: