The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 527613 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #527613 - Flags: review?(jwalden+bmo)
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;
Attachment #527613 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 542254 [details] [diff] [review]
v2

Good save. Here's the updated patch.
Attachment #527613 - Attachment is obsolete: true
Attachment #542254 - Flags: review?(jwalden+bmo)
Attachment #542254 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/cad13a541e30
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/cad13a541e30
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Depends on: 675581
(Assignee)

Updated

6 years ago
No longer depends on: 675581
You need to log in before you can comment on or make changes to this bug.