1510724 - Add more diagnostics for PLHashTables and PLDHashTables

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

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).

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add accessor for wrapped PLDHashTable operations.

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.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Add new HashTable diagnostics.

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.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Add PreambleResult kind for redirected functions which don't generate events.

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).

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Add diagnostic redirections for hashtable operations.

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.)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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.

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/c1a7bed23922
https://hg.mozilla.org/mozilla-central/rev/a69ff8282539
https://hg.mozilla.org/mozilla-central/rev/5aa50800cd8b
https://hg.mozilla.org/mozilla-central/rev/c5e4fbda309e