Created attachment 607538 [details] [diff] [review]
patch, check the user function pointer for null
The comments in nsBaseHashtable.h and nsTHashtable.h imply that it would be OK to call SizeOfExcludingThis() with a null sizeOfEntryExcludingThis:
263 * Measure the size of the table's entry storage, and if
264 * |sizeOfEntryExcludingThis| is non-nsnull, measure the size of things pointed
265 * to by entries.
and similarly at http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTHashtable.h#277.
However, the s_SizeOfStub functions in those classes actually call the provided sizeOfEntryExcludingThis pointer *without* checking that it is non-null, and will therefore crash.
Created attachment 607775 [details] [diff] [review]
Apologies for the false advertising!
Your patch works, but I'd rather handle the NULL case within SizeOfExcludingThis instead of in s_SizeOfStub, as the attached patch does. IMHO it's easier to understand, and it doesn't result in unnecessary enumeration of the hash table in the NULL case.
(BTW, I've tested this lightly in a trunk build, but not with any code that passes in NULL. I assume your patch in bug 688125 needs this? Could you please test this patch with that patch?)
Comment on attachment 607775 [details] [diff] [review]
Review of attachment 607775 [details] [diff] [review]:
Looks fine. The second patch (forthcoming...) in bug 688125 will rely on this behavior, so I'll test it together with that, but I'm confident it'll work as required.
One of the patches in this push caused winxp debug mochitests to perma-hang in test_memoryReporters.xul. Backed the entire push out.
I don't think this patch can be the culprit there, so I re-landed it: