927272 - Stale fixme comment in MapObject.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

builtin/MapObject.cpp, l. 1010-1011:
>  // So as a specially gruesome hack, overwrite the key in place.
>  // FIXME bug 769504.

But bug 769504 is already fixed, comment originates from bug 743107.

Comment 1

[André Bargull [:anba]]

Setting needinfo from jorendorff based on commit logs.

Comment 2

[Jason Orendorff [:jorendorff]]

Whoa. I don't think this code is right.

It's calling rekeyFrontWithSameHashCode but the hash code of a string value is val.asRawBits() ...which would have changed.

Terrence, is this busted with GGC? Or perhaps is the deal that we never move atoms, and so all this can be replaced with an assertion that it doesn't happen?

Comment 3

[Terrence Cole [:terrence]]

Attachment: simplify_mapobject_rekey-v0.diff

Ah, I did not know that we only looked a the value bits and not the string's chars: that makes things much simpler.

The problem here is that "marking" the key (both objects and in future strings) overwrites their contents in the heap with a forwarding pointer and GC metadata. If we then go on to access the data to get a hash, we are doomed. The other hashtable gets around this because it stores the hash inline in the table so we don't have to re-compute the value on the unmoved object.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 818142 [details] [diff] [review]
simplify_mapobject_rekey-v0.diff

Review of attachment 818142 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 5

[Jason Orendorff [:jorendorff]]

Comment 3 makes me think this code is a little tricky. A comment on that rekeyFront() call explaining why it's safe would be nice.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Comment 3 makes me think this code is a little tricky. A comment on that
> rekeyFront() call explaining why it's safe would be nice.

Added.

https://hg.mozilla.org/integration/mozilla-inbound/rev/645c5dd062fb

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/645c5dd062fb