Closed Bug 948516 Opened 6 years ago Closed 6 years ago

Add more correctness assertions to HashTable


(Core :: JavaScript Engine, defect)

Not set





(Reporter: terrence, Assigned: terrence)



(Whiteboard: [qa-])


(1 file)

Currently, changes to HashTable::generation can only happen after bumping HashTable::mutationCount. With generational GC enabled, however, a MinorGC can change a table's generation through rekey without changing its mutationCount. Since AddPtr only checks mutationCount, this meant that we were sometimes not catching a MinorGC across the lifetime of an AddPtr, later accessing freed memory.

We could "fix" this by making generation changes also bump mutationCount, but I was worried there might be other holes I wasn't seeing. Instead, since this is all debug code, I just added full correctness assertions to all the things. With this patch we assert the correctness of mutationCount and generation in Ptr, AddPtr, Range, and Enum, hopefully preventing any further instances of this class of errors.
Attachment #8345378 - Flags: review?(luke)
Comment on attachment 8345378 [details] [diff] [review]

Review of attachment 8345378 [details] [diff] [review]:

Nice!  Can you wrap all the DebugOnly member variable declarations in #ifdef DEBUG?  Otherwise they'll consume real space (even if it's dead) hash tables are relatively hot enough for this to matter.
Attachment #8345378 - Flags: review?(luke) → review+

You are right, it's probably worth it to make sure Ptr fits in a word even if it is a bit uglier.

And backed out because apparently layout is constructing their own Ptrs(!)?
Was not able to reproduce the build bustage locally with linux-clang. Clang on mac seems to be complaining that |table| in HashTable::Ptr(HashTable &table) masks HashTable::table. O_o Likewise for the constructors of AddPtr and Range. AFAICT, clang is off it's meds.

I've renamed the argument to |tableArg| and the nested |table| to |table_| in the vague hope that clang doesn't flip out again.
Looks green on the builds that failed before, relanding:
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.