Last Comment Bug 737412 - nsTHashtable and nsBaseHashtable memory reporter functions crash if given a null sizeOfEntryExcludingThis function
: nsTHashtable and nsBaseHashtable memory reporter functions crash if given a n...
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla14
Assigned To: Nicholas Nethercote [:njn]
: Nathan Froyd [:froydnj][high latency until 6 March]
Depends on:
Blocks: 688125
  Show dependency treegraph
Reported: 2012-03-20 07:21 PDT by Jonathan Kew (:jfkthame)
Modified: 2012-03-24 13:42 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch, check the user function pointer for null (1.59 KB, patch)
2012-03-20 07:21 PDT, Jonathan Kew (:jfkthame)
n.nethercote: review-
Details | Diff | Splinter Review
alternative patch (2.61 KB, patch)
2012-03-20 16:54 PDT, Nicholas Nethercote [:njn]
jfkthame: review+
Details | Diff | Splinter Review

Description User image Jonathan Kew (:jfkthame) 2012-03-20 07:21:48 PDT
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

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.
Comment 1 User image Nicholas Nethercote [:njn] 2012-03-20 16:54:16 PDT
Created attachment 607775 [details] [diff] [review]
alternative patch

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 2 User image Jonathan Kew (:jfkthame) 2012-03-21 01:28:44 PDT
Comment on attachment 607775 [details] [diff] [review]
alternative patch

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.
Comment 4 User image Ryan VanderMeulen [:RyanVM] 2012-03-23 14:06:43 PDT
One of the patches in this push caused winxp debug mochitests to perma-hang in test_memoryReporters.xul. Backed the entire push out.
Comment 5 User image Jonathan Kew (:jfkthame) 2012-03-23 15:15:56 PDT
I don't think this patch can be the culprit there, so I re-landed it:
Comment 6 User image Ed Morley [:emorley] 2012-03-24 13:42:35 PDT

Note You need to log in before you can comment on or make changes to this bug.