651940 - When hashing a jsid, assert that js_CheckForStringIndex(id) == id

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

It is always a mistake to hash a jsid of the string "0" or "-1".

Asserting to that effect and running the test suite reveals just one bug.

I need to make HASH0 public for bug 637985 so I'm going to do it here while I'm touching the definition.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 527613 [details] [diff] [review]
v1

>diff --git a/js/src/jsatom.h b/js/src/jsatom.h

>+extern jsid
>+js_CheckForStringIndex(jsid id);

I assume this method wasn't actually being inlined before, but I'd be interested to know whether that was really the case in general or not, if you investigated.  No need to do so if you didn't, just curiosity.


>+static JS_ALWAYS_INLINE JSHashNumber
>+HashId(jsid id)
>+{
>+    JS_ASSERT(js_CheckForStringIndex(id) == id);
>+    return JS_GOLDEN_RATIO *
>+#if JS_BYTES_PER_WORD == 4
>+        JSHashNumber(JSID_BITS(id));
>+#elif JS_BYTES_PER_WORD == 8
>+        JSHashNumber(JSID_BITS(id)) ^ JSHashNumber(JSID_BITS(id) >> 32);
>+#else
>+# error "Unsupported configuration"
>+#endif
>+}

(m * a ^ b) === ((m * a) ^ b), but it was m * (a ^ b) before.  Also, I'd rather not see an #if nested within an expression like this, because then it's harder to see which part of the #if actually ended up being used.  I suggest this instead, to address both concerns in one swell foop:

>     JSHashNumber n =
> #if JS_BYTES_PER_WORD == 4
>         JSHashNumber(JSID_BITS(id));
> #elif JS_BYTES_PER_WORD == 8
>         JSHashNumber(JSID_BITS(id)) ^ JSHashNumber(JSID_BITS(id) >> 32);
> #else
> # error "Unsupported configuration"
> #endif
> return n * JS_GOLDEN_RATIO;

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v2

Good save. Here's the updated patch.

Comment 4

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cad13a541e30

Comment 5

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/cad13a541e30