There several places in our codebase where we're doing: |blah.SizeOfIncludingThis|. This is certainly incorrect. There are also many instances of: |blah->SizeOfExcludingThis| Some of those might be legit, but we should audit them. Worst case-scenario they skew our DMD/heap-unclassified metrics. I've attached a quick search I did with grep, it may not be exhaustive but it's a good start.

Mmm, yes!

> There several places in our codebase where we're doing: > |blah.SizeOfIncludingThis|. This is certainly incorrect. My experience is that executing calls like this tend to cause crashes pretty reliably. So any cases we have in the code must be cases where the code is rarely executed, e.g. bug 1001419, bug 969902.

Finished triage, found the following instances to be reasonable (mostly hash table callbacks): content/base/src/nsAttrAndChildArray.cpp:853: n += value->SizeOfExcludingThis(aMallocSizeOf); dom/base/nsScriptNameSpaceManager.cpp:857: return entry->SizeOfExcludingThis(aMallocSizeOf); image/src/FrameBlender.cpp:594: n += mAnim->compositingFrame->SizeOfExcludingThisWithComputedFallbackIfHeap(aLocation, aMallocSizeOf); image/src/FrameBlender.cpp:597: n += mAnim->compositingPrevFrame->SizeOfExcludingThisWithComputedFallbackIfHeap(aLocation, aMallocSizeOf); image/src/FrameSequence.cpp:99: n += frame->SizeOfExcludingThisWithComputedFallbackIfHeap(aLocation, aMallocSizeOf); image/src/RasterImage.cpp:1097: n += mScaleResult.frame->SizeOfExcludingThisWithComputedFallbackIfHeap(aLocation, aMallocSizeOf); layout/base/FramePropertyTable.h:162: n += array->SizeOfExcludingThis(aMallocSizeOf); layout/style/nsCSSDataBlock.cpp:272: n += ValueAtIndex(i)->SizeOfExcludingThis(aMallocSizeOf); modules/libpref/src/Preferences.cpp:221: n += gObserverTable->SizeOfExcludingThis(SizeOfObserverEntryExcludingThis, netwerk/cache2/CacheFileIOManager.cpp:515: return key->SizeOfExcludingThis(mallocSizeOf); netwerk/cache2/CacheFileIOManager.cpp:3770: return gInstance->SizeOfExcludingThisInternal(mallocSizeOf); netwerk/cache2/CacheIndex.cpp:3497: return aEntry->SizeOfExcludingThis(mallocSizeOf); netwerk/cache2/CacheIndex.cpp:3547: return gInstance->SizeOfExcludingThisInternal(mallocSizeOf); netwerk/cookie/nsCookieService.cpp:625: return aEntry->SizeOfExcludingThis(aMallocSizeOf); netwerk/cookie/nsCookieService.cpp:644: return aEntry->SizeOfExcludingThis(aMallocSizeOf); startupcache/StartupCache.cpp:398: return data->SizeOfExcludingThis(mallocSizeOf); xpcom/components/nsCategoryManager.cpp:498: return aData.get()->SizeOfExcludingThis(aMallocSizeOf); xpcom/glue/nsBaseHashtable.h:246: return mallocSizeOf(this) + this->SizeOfExcludingThis(sizeOfEntryExcludingThis,

This patch fixes the remaining |blah.SizeOfIncludingThis| usage. Additionally it switches several instances to using |blah->SizeOfIncludingThis| and adds the appropriate function where necessary.

try: https://tbpl.mozilla.org/?tree=Try&rev=947f92527ee1

Comment on attachment 8416638 [details] [diff] [review] Fix incorrect usage of SizeOfIncludingThis and SizeOfExcludingThis functions Review of attachment 8416638 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this. I hope you've done a DMD run; if not, please do. I'm trying to imagine a way to make these mistakes harder or impossible in the future. For the hashtable ones, where foo->SizeOfExcludingThis() is (confusingly) the right thing to do, I wonder if changing those functions to take a reference to the entry rather than a pointer would help -- at least they wouldn't show up as false positives when grepping for incorrect cases. For the others, I'm trying to imagine some kind of template that's specialized on <T*> and <T>, but failing.

> For the others, I'm trying to imagine some kind of template that's > specialized on <T*> and <T>, but failing. Actually, the attached example appears to work, at least for the simplest cases -- you just call SizeOf() and it calls onto the appropriate method. (If we changed the hashtable case to use references instead of pointers, I think it would even work in that case.) We'd need to also handle our pointer-like types such as nsRefPtr, but I think it could be done. It assumes that all pointers are to things on the heap, though. So if you have a pointer to the stack it'll get that wrong. That is probably rare or non-existent in practice, though?

(In reply to Nicholas Nethercote [:njn] from comment #6) > Thanks for doing this. I hope you've done a DMD run; if not, please do. I have, but I'll do a more thorough run on both linux and osx. > I'm trying to imagine a way to make these mistakes harder or impossible in > the future. For the hashtable ones, where foo->SizeOfExcludingThis() is > (confusingly) the right thing to do, I wonder if changing those functions to > take a reference to the entry rather than a pointer would help -- at least > they wouldn't show up as false positives when grepping for incorrect cases. I agree switching to a const reference is a good improvement. I'm happy to change that -- although in another bug of course! (In reply to Nicholas Nethercote [:njn] from comment #7) > We'd need to also handle our pointer-like types such as nsRefPtr, but I > think it could be done. nsRefPtr and friends should have implicit conversion to a pointer operators (whether that works with template specialization I'm not sure), see: http://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoPtr.h?from=nsRefPtr&case=true#1057 > It assumes that all pointers are to things on the heap, though. So if you > have a pointer to the stack it'll get that wrong. That is probably rare or > non-existent in practice, though? I think it is fair to assume that's a rare case, and in general whoever is doing that would know _not_ to use the helpers. And really it would be even rarer than you think, they'd be storing a pointer to a stack var as a _member_ variable. The few instances where |->SizeOfExcludingThis| was legit (and not a hashtable callback) were in some sort of cast on a member var that was large enough to hold two types. Think: enum type { a, b } mType; char mVar[5]; ... if (mType == a) return (TypeA*)mVar->SizeOfExcludingThis(); else return (TypeB*)mVar->SizeOfExcludingThis(); I was thinking about this previously, it would be really nice to have a version for arrays as well, something like: template <typename T> inline size_t SizeOf(const nsTArray_Impl<T>& t) { size_t n = t.sizeOfExcludingThis(); for (uint32_t i = 0; i < t.size(); i++) n += SizeOf(t[i]); return n; }

> I agree switching to a const reference is a good improvement. I'm happy to > change that -- although in another bug of course! Yes, both ideas would be follow-ups.

DMD runs on linux and mac look ok.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb14c610d90a