Last Comment Bug 651940 - When hashing a jsid, assert that js_CheckForStringIndex(id) == id
: When hashing a jsid, assert that js_CheckForStringIndex(id) == id
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-21 12:14 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-08-03 17:28 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (8.39 KB, patch)
2011-04-21 12:15 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review-
Details | Diff | Splinter Review
v2 (8.30 KB, patch)
2011-06-27 13:36 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-04-21 12:14:04 PDT
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] 2011-04-21 12:15:23 PDT
Created attachment 527613 [details] [diff] [review]
v1
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-27 15:20:16 PDT
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] 2011-06-27 13:36:54 PDT
Created attachment 542254 [details] [diff] [review]
v2

Good save. Here's the updated patch.
Comment 4 Jason Orendorff [:jorendorff] 2011-07-27 15:47:06 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/cad13a541e30

Note You need to log in before you can comment on or make changes to this bug.