1303174 - Correct gdb nsTHashtable pretty printer's understanding of table capacity to not be the entry count

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

In bug 1286467 I introduced an nsTHashtable pretty-printer and made the most obvious error possible. I treated the number of populated slots (mEntryCount) as the capacity/total number of slots.  The capacity is instead found via CapacityFromHashShift and the computation ((uint32_t)1 << (kHashBits - mHashShift)).  Because of the high levels of embarrassment associated with this level of slip-up, I instead redirect all blame to PLDHashTable's poor choice of terminology or possible solar flares.

Patch up once I have a bug number.

My extensive proof of correctness is:
== Before:
(gdb) p mDomainMap
$8 = nsClassHashtable<nsCStringHashKey, mozilla::dom::workers::RuntimeService::WorkerDomainInfo>

== After:
(gdb) p '(anonymous namespace)::gRuntimeService'->mDomainMap
$1 = nsClassHashtable<nsCStringHashKey, mozilla::dom::workers::RuntimeService::WorkerDomainInfo> = {
  ["localhost"] = [(mozilla::dom::workers::RuntimeService::WorkerDomainInfo *) 0x7fffb6d749e0],
  [<gNullChar> ""] = [(mozilla::dom::workers::RuntimeService::WorkerDomainInfo *) 0x7fffc6bb8040]
}

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Use the capacity to find out how many slots there are, not the entry count. v1

Let's see if :froydnj is available for reviews this time :)

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8791785 [details] [diff] [review]
Use the capacity to find out how many slots there are, not the entry count. v1

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

Hopefully there has not been much hair-pulling because of this bug.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Hopefully there has not been much hair-pulling because of this bug.

Thanks for the quick review!  And yes, the potential for misleading gdb-users was VERY bad.  The one upside is that prior to introduction of the pretty printer in bug 1286467 the hashtables were effectively non-introspectable so hopefully people weren't relying on them yet.

Ideally we can introduce tests like the JS pretty printers have in https://dxr.mozilla.org/mozilla-central/source/js/src/gdb/tests because of the importance of a reliable debugging experience.  I'd like to file a bug on that especially so we can discuss if there's a way to minimize code duplication of the excellent JS setup, but I feel like this is a bad bugzilla component for tracking such things; it's got way too much stuff that's not pretty-printer related.  I'll try and see about getting an appropriately scoped and located component created and announced on dev-platform to accompany a "whoops!" to inform people about the pretty printer's betrayal of us all.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0283d52d9caa7e58aa3b7c5a3dc25da3e80ca58
Bug 1303174 - Correct gdb nsTHashtable pretty printer's understanding of table capacity to not be the entry count. r=nfroyd

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f0283d52d9ca