Keyword Hashing

[Thomas Heller]

Hey,

I'm running into alot of

java.lang.IllegalArgumentException: Duplicate key: :display-as
at clojure.lang.PersistentHashMap.createWithCheck(PersistentHashMap.java:67)
at clojure.lang.RT.map(RT.java:1462)
at clojure.lang.EdnReader$MapReader.invoke(EdnReader.java:631)
at clojure.lang.EdnReader.read(EdnReader.java:142)
at clojure.lang.EdnReader.read(EdnReader.java:108)
at clojure.lang.EdnReader.readString(EdnReader.java:64)
at clojure.edn$read_string.invoke(edn.clj:46)

lately with the new clojurescript-0.0-2277 release, I'm reading maps that where pr-str'd on the clojurescript side of things and then transmitted over the wire.

It shouldn't happen that cljs prints a map with duplicated keys since there should not be a way to insert the same key twice.

I wasn't able to reproduce this yet as it doesn't happen for everybody, I guess its only a subset of browsers or other conditions but the only way I can trace this back is to faulty keyword hashing.

Will try to produce a minimal test-case but it seems there is a serious issue somewhere.

YMMV,
/thomas

[David Nolen]

I've heard this from a couple of sources. Minimal case would be very
welcome and we'll cut a release as soon as we have a patch.

David

> --
> Note that posts from new members are moderated - please be patient with your first post.
> ---
> You received this message because you are subscribed to the Google Groups "ClojureScript" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clojurescrip...@googlegroups.com.
> To post to this group, send email to clojur...@googlegroups.com.
> Visit this group at http://groups.google.com/group/clojurescript.

[Thomas Heller]

FWIW I started logging User Agents that send the "faulty" hash-maps and it seems to be related to mobile Safari. Can reproduce it on my iPhone now. Safari 7.0.5 on my mac doesnt show this problem though.

Will look some more.

Cheers,
/thomas

PS: Some of the affected user agents

Mozilla/5.0 (iPad; CPU OS 7_1_2 like Mac OS X) AppleWebKit/537.51.2 (KHTML, like Gecko) Version/7.0 Mobile/11D257 Safari/9537.53
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
Mozilla/5.0 (Linux; U; Android 4.1.2; de-de; GT-N8010 Build/JZO54K) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Safari/534.30

[Thomas Heller]

Not much closer but some log output

keyword a, keyword b, hash a, hash b
:tags :tags 250931621 1771418977
:contract-type :contract-type -585196394 580786577
:voice-de-o2 :voice-de-o2 1348274488 1352616807
:voice-de-telekom :voice-de-telekom 475690556 2143804784
:voice-de-eplus :voice-de-eplus -1670546243 1420630636
:voice-de-vodafone :voice-de-vodafone 19026943 613155916

Found by converting the cljs map to string-keys and logging the duplicates.

Not any closer to a test-case though since it only happens on the production server and not on my dev machine although I'm using the same optimized cljs build and HTML. Weird.

[Thomas Heller]

Thought it might be related to

https://github.com/clojure/clojurescript/commit/e92e8064813ed9a74c6dcf5bfd3adf5b85df1aea

but can confirm that that is not the issue. At least the (imul 0xffffffff 5) outputs the correct -5.

[David Nolen]

Yes this was my thought as well. Perhaps there's another edge case
that we're missing?

David

[Thomas Heller]

Gave up yesterday, will try more today.

Last thing I tried

(hash :test)
(hash (keyword "test"))
(hash (reader/read-string ":test"))

and comparing those hashes, the error didn't show this way even when tried with several different test strings.

Running out of ideas to test, but its reasonable to assume that only mobile Safari/Webkit is affected since no other user agent was logged so far.

[David Nolen]

Are you testing only mobile Safari? At this point seems like the thing
to focus on.

David

[Thomas Heller]

Yes, I'm only testing Safari on iOS. Which looking at my logs seems to be the only one affected. Only Mobile Safari User Agents were sending maps with duplicate keys.

No luck producing a more compact example though.

[Thomas Heller]

Hmm it might be related to the way keywords are generated.

The compiler will emit ":test" as new cljs.Keyword(null,"test","test",577538877) where the hash is precomputed in Clojure, if now for some reason ClojureScript generates another Hash for that keyword we have our problem.

I'm not a math guy but I wouldn't put it out of the question that javascript math works differently than java math, especially given how weird javascripts Number type is.

But I'm guessing here since the precompute only happens when :emit-constants is not used (why is that?) but I'm using :emit-constants and the bug still shows.

Running out of ideas ...

[Thomas Heller]

Created a patch to address that :emit-constants issue.

http://dev.clojure.org/jira/browse/CLJS-829

Can't say wether its related to the issue at hand but less duplicate code is always good.

[David Nolen]

Applied to master thanks.

David

[Thomas Heller]

Thanks.

I give up.

I have no more ideas what to test, whats even more annoying is that when I attach the remote debugger BEFORE navigating to the page producing the faulty maps the bug doesn't show. If I do the exact same steps but attach the remote debugger AFTER loading the page the bug happens.

At least I can tell you that the release 0.0-2234 doesn't have this problem.

So it was introduced some time after

https://github.com/clojure/clojurescript/compare/b8c4a40d9a795899b86fd33e6024a19b899640db...master

But since the murmur3 hashing was introduced after 2234 thats no surprise.

Happy to test any ideas you might have, but I can't think of anything I haven't tried.

[David Nolen]

So you or do not have a minimal case for Mobile Safari that I can test with?

Thanks,
David

[Thomas Heller]

No I don't.

The smallest "test" I was able to come up was my production app.

I was not able to isolate the issue and the app does way too much stuff to pinpoint anything.

I can give you an URL and the steps to reproduce the issue but thats about it.

Sorry,
/thomas