Add more diagnostics for PLHashTables and PLDHashTables

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(5 attachments)

Posted 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 bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1a7bed23922
Part 1 - Add accessor for wrapped PLDHashTable operations, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69ff8282539
Part 2 - Add new HashTable diagnostics, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa50800cd8b
Part 3 - Add PreambleResult kind for redirected functions which don't generate events, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5e4fbda309e
Part 4 - Add diagnostic redirections for hashtable operations, r=lsmyth.
You need to log in before you can comment on or make changes to this bug.