Closed Bug 927272 Opened 6 years ago Closed 6 years ago

Stale fixme comment in MapObject.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: anba, Assigned: terrence)

Details

Attachments

(1 file)

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.
Setting needinfo from jorendorff based on commit logs.
Flags: needinfo?(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?
Flags: needinfo?(jorendorff)
Flags: needinfo?(terrence)
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.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #818142 - Flags: review?(jcoppeard)
Flags: needinfo?(terrence)
Comment on attachment 818142 [details] [diff] [review]
simplify_mapobject_rekey-v0.diff

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

Looks good!
Attachment #818142 - Flags: review?(jcoppeard) → review+
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.
(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
https://hg.mozilla.org/mozilla-central/rev/645c5dd062fb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.