Closed Bug 1510724 Opened 2 years ago Closed 2 years ago

Add more diagnostics for PLHashTables and PLDHashTables


(Core Graveyard :: Web Replay, enhancement)

Not set


(firefox65 fixed)

Tracking Status
firefox65 --- fixed


(Reporter: bhackett1024, Assigned: bhackett1024)


(Blocks 1 open bug)



(5 files)

Attached patch patchSplinter Review
There are still a fair number of web replay crashes related to PLHashTable and PLDHashTable operations.  These generally seem related to corruption of the custom callbacks generated for these tables.  The attached patch adds some diagnostics to check for this, by redirecting the functions operating on these tables to check that callbacks are used with consistent tables and that their contents remain intact.

This uses redirections to avoid instrumenting the hashtable code directly, which can't be done for plhash.c (it is in NSPR) and would be ugly for PLDHashTable.cpp.  This risks missing instrumentation if these functions are inlined somewhere, but I haven't seen cases where these functions are inlined and in any case it is OK to not run the diagnostics sometimes (crashing call stacks can be inspected to see if the diagnostics were called on that path).
Add an accessor so that diagnostics can get the raw operations in a PLDHashTable before any record/replay modified version of the ops has been unwrapped.
Attachment #9028406 - Flags: review?(nfroyd)
Add methods to check the integrity of the operations in a PLHashTable or PLDHashTable, by making sure the operations are used with the same table consistently and that any generated code for them has not been corrupted.
Attachment #9028407 - Flags: review?(lsmyth)
Add a new PreambleResult kind, IgnoreRedirect, that can be used in redirections that don't modify the calling thread's event stream at all but don't also prevent events within the call from being recordsed (as PreambleResult::PassThrough would do).
Attachment #9028408 - Flags: review?(lsmyth)
Add redirections for PLHashTable and PLDHashTable interfaces.  This uses some new mechanisms to specify the redirections as we can't use dlsym() to look up internal methods in XUL.  (We can use dlsym to lookup the PLHashTable interfaces since they are in a different library, but it still seems better to keep the diagnostics separate here.)
Attachment #9028409 - Flags: review?(lsmyth)
Attachment #9028406 - Flags: review?(nfroyd) → review+
Attachment #9028407 - Flags: review?(lsmyth) → review+
Attachment #9028408 - Flags: review?(lsmyth) → review+
Attachment #9028409 - Flags: review?(lsmyth) → review+
Pushed by
Part 1 - Add accessor for wrapped PLDHashTable operations, r=froydnj.
Part 2 - Add new HashTable diagnostics, r=lsmyth.
Part 3 - Add PreambleResult kind for redirected functions which don't generate events, r=lsmyth.
Part 4 - Add diagnostic redirections for hashtable operations, r=lsmyth.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.