338362 - Detect live |this| pointer on the stack when deleting a XPCOM object

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Feng Qian (Google)]

When deleting a XPCOM object, walk the stack and check if the first element on each stack frame is the same as the deleting pointer. Many kungFuDeathGrip(this) in the code base can be detected.

It only works on Linux debug mode, and the output should be post-processed by tools/rb/fix-linux-stack.pl to get readable stack traces.

It is optimized to skip scoped nsCOMPtr. It is fast and has low false positive rate.

Comment 1

[Feng Qian (Google)]

Attachment: part I

Comment 2

[Feng Qian (Google)]

Attachment: nsLiveStackRefCheck.cpp

nsLiveStackRefCheck.h should be put in xpcom/base

Comment 3

[Feng Qian (Google)]

Attachment: nsLiveStackRefCheck.h

should be put in xpcom/base

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm not particularly qualified to review this, and more importantly I'm not an XPCOM peer.  Please request review from an XPCOM peer.

Comment 5

[Benjamin Smedberg]

I'm not going to have time to review this until later next week.

Comment 6

[timeless]

some comments:
1. cvsdo add with cvs diff -uN would be good.
2. don't add trailing whitespace to files
3. the files you're adding are Unix only which means their file names are inappropriate.
4. plasticmillion has a serviced version of stack walking, and this code should use it. if that means someone should help him get it landed, then that's what should be done.
5. this is the nth fork i've seen of this walking code which is getting really really really annoying (see 4).

Comment 7

[Graydon Hoare :graydon]

Bsmedberg asked me to review. Here are some notes:

 - House style issues: 
   - use PRBool, PRUintn/PRIntn, PR_TRUE/PR_FALSE, etc.
   - Indent early returns.
   - Untabify.

 - Diff to nsStackFrameUnix.cpp appears to be dead code and/or
   whitespace diffs.

 - Likewise nsTraceRefcntImpl.h

 - Not sure what the purpose of changes to tools/Makefile.in and
   tools/registry/Makefile.in are for; look like dead code?

 - Remove commented-out code in the middle of nsLiveStackRefCheck::Release.

 - Use more informative names: ptr and fp should be dyingPtr and
   fpOfLastAddRefCaller (or something similarly useful) in
   nsLiveStackRefCheck::Release.

 - Name and explain magic constants (2, 2, 2 and 4) in stack-frame
   walker; I can imagine why you believe they are correct, but it's 
   not stated anywhere.

 - If I understand your scanner correctly, you're only looking at the slot
   two words beneath the frame pointer, which on i386 ABI is either the
   last argument word or (if it's a __thiscall__ frame) the 'this' 
   pointer of the callee. So it seems to me you're only going to notice
   when there's an existing call in progress to your target 'this', not
   when there locals or argument pointers containing your target 'this'.
   Am I correct? It's not clear. 

   If I'm correct above, then your initial comment seems inaccurate:
   you don't *find* kungFuDeathGrip(this) instances; you find places where
   the code *needs* a kungFuDeathGrip(this). Correct? I'm uncertain
   what you're aiming to detect, exactly. Please clarify.

 - Optimization logic is obscure. It takes several reads to make it
   obvious what the purpose of storing mCallerFP is. Add a comment that 
   explains the motivation and reasoning. Something like this:

     We are going to search for a 'this' pointer in frames underneath us.
     The frame immediately beneath us is _class::Release(); let's call the
     frame beneath _class::Release() "frame X". Note that we also saved the 
     frame pointer underneath the most recent call to __class::AddRef(), 
     stored it in mCallerFP, and passed it into nsLiveStackRefCheck::Release
     as the 'fp' parameter.

     Therefore if frame X is the same frame as fp, and we are
     at refcount zero, we are probably looking at a short-lived object:
     it was last AddRef'ed in the same frame (or rather, a frame with
     the same address) as the frame we're releasing it from. So we assume 
     that it's actually the same frame, and that it's unlikely that
     anyone stored an escaping 'this' pointer into a stack variable underneath 
     frame X, and abort our scan early. This should save us scanning for most
     short-lived objects.

 - Perhaps add some commentary to the file nsLiveStackRefCheck.cpp
   that notes, again, all the assumptions you're making about ABI
   (gnuc, i386, libdl, frame pointers present); casual reader such as
   myself may find it disconcerting to see ABI assumptions sprinkled 
   through code. NS_CHECK_LIVE_REF_ON_STACK looks like a "feature request" 
   macro, not a "this code is only defined on one particular platform" macro.
   Initial gut reaction to ::GetCallerFP is "this might work, on one very
   specific platform..."

   As per comment above, you may also wish to rename the guard 
   NS_CHECK_STACK_FOR_CALL_IN_PROGRESS_TO_DYING_REF or such, since 
   it doesn't seem to scan locals.

Comment 8

[Benjamin Smedberg]

Comment on attachment 222422 [details] [diff] [review]
part I

Cancelling reviews, please clarify the points Graydon mentioned.