From de82a95c8c837dd2acd3b541554495389a2f1ec9 Mon Sep 17 00:00:00 2001 From: "scc%netscape.com" Date: Sat, 20 Nov 1999 08:19:24 +0000 Subject: [PATCH] fixed an annoying, but rare, bug where an |nsCOMPtr| in an ownership ring could |Release()| twice. See the comment in the code for details. r=waterson git-svn-id: svn://10.0.0.236/trunk@54061 18797224-902f-48f8-a5cc-f745e15eee43 --- mozilla/xpcom/base/nsCOMPtr.cpp | 15 ++------ mozilla/xpcom/base/nsCOMPtr.h | 61 ++++++++++++++++++--------------- mozilla/xpcom/glue/nsCOMPtr.cpp | 15 ++------ mozilla/xpcom/glue/nsCOMPtr.h | 61 ++++++++++++++++++--------------- 4 files changed, 74 insertions(+), 78 deletions(-) diff --git a/mozilla/xpcom/base/nsCOMPtr.cpp b/mozilla/xpcom/base/nsCOMPtr.cpp index a78b188733d..c95f17579e0 100644 --- a/mozilla/xpcom/base/nsCOMPtr.cpp +++ b/mozilla/xpcom/base/nsCOMPtr.cpp @@ -50,9 +50,7 @@ nsCOMPtr_base::assign_with_AddRef( nsISupports* rawPtr ) { if ( rawPtr ) NSCAP_ADDREF(rawPtr); - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = rawPtr; + assign_assuming_AddRef(rawPtr); } void @@ -61,19 +59,12 @@ nsCOMPtr_base::assign_from_helper( const nsCOMPtr_helper& helper, const nsIID& i nsISupports* newRawPtr; if ( !NS_SUCCEEDED( helper(iid, NS_REINTERPRET_CAST(void**, &newRawPtr)) ) ) newRawPtr = 0; - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = newRawPtr; + assign_assuming_AddRef(newRawPtr); } void** nsCOMPtr_base::begin_assignment() { - if ( mRawPtr ) - { - NSCAP_RELEASE(mRawPtr); - mRawPtr = 0; - } - + assign_assuming_AddRef(0); return NS_REINTERPRET_CAST(void**, &mRawPtr); } diff --git a/mozilla/xpcom/base/nsCOMPtr.h b/mozilla/xpcom/base/nsCOMPtr.h index 505bf343ac5..945d05f6f44 100644 --- a/mozilla/xpcom/base/nsCOMPtr.h +++ b/mozilla/xpcom/base/nsCOMPtr.h @@ -261,7 +261,7 @@ class NS_EXPORT nsQueryInterface : public nsCOMPtr_helper private: nsISupports* mRawPtr; - nsresult* mErrorPtr; + nsresult* mErrorPtr; }; inline @@ -373,6 +373,23 @@ class nsCOMPtr_base protected: nsISupports* mRawPtr; + + void + assign_assuming_AddRef( nsISupports* newPtr ) + { + /* + |AddRef()|ing the new value (before entering this function) before + |Release()|ing the old lets us safely ignore the self-assignment case. + We must, however, be careful only to |Release()| _after_ doing the + assignment, in case the |Release()| leads to our _own_ destruction, + which would, in turn, cause an incorrect second |Release()| of our old + pointer. Thank waterson@netscape.com for discovering this. + */ + nsISupports* oldPtr = mRawPtr; + mRawPtr = newPtr; + if ( oldPtr ) + NSCAP_RELEASE(oldPtr); + } }; // template class nsGetterAddRefs; @@ -389,6 +406,15 @@ class nsCOMPtr void assign_from_helper( const nsCOMPtr_helper&, const nsIID& ); void** begin_assignment(); + void + assign_assuming_AddRef( T* newPtr ) + { + T* oldPtr = mRawPtr; + mRawPtr = newPtr; + if ( oldPtr ) + NSCAP_RELEASE(oldPtr); + } + private: T* mRawPtr; @@ -495,10 +521,7 @@ class nsCOMPtr operator=( const nsDontAddRef& rhs ) // assign from |dont_AddRef(expr)| { - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = rhs.mRawPtr; - + assign_assuming_AddRef(rhs.mRawPtr); NSCAP_ASSERT_NO_QUERY_NEEDED(); return *this; } @@ -565,9 +588,7 @@ class nsCOMPtr #ifndef NSCAP_FEATURE_INLINE_STARTASSIGNMENT return NS_REINTERPRET_CAST(T**, begin_assignment()); #else - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = 0; + assign_assuming_AddRef(0); return NS_REINTERPRET_CAST(T**, &mRawPtr); #endif } @@ -658,10 +679,7 @@ class nsCOMPtr operator=( const nsDontAddRef& rhs ) // assign from |dont_AddRef(expr)| { - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = rhs.mRawPtr; - + assign_assuming_AddRef(rhs.mRawPtr); return *this; } @@ -726,9 +744,7 @@ class nsCOMPtr #ifndef NSCAP_FEATURE_INLINE_STARTASSIGNMENT return NS_REINTERPRET_CAST(nsISupports**, begin_assignment()); #else - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = 0; + assign_assuming_AddRef(0); return NS_REINTERPRET_CAST(nsISupports**, &mRawPtr); #endif } @@ -741,9 +757,7 @@ nsCOMPtr::assign_with_AddRef( nsISupports* rawPtr ) { if ( rawPtr ) NSCAP_ADDREF(rawPtr); - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = NS_REINTERPRET_CAST(T*, rawPtr); + assign_assuming_AddRef(NS_REINTERPRET_CAST(T*, rawPtr)); } template @@ -753,21 +767,14 @@ nsCOMPtr::assign_from_helper( const nsCOMPtr_helper& helper, const nsIID& aII T* newRawPtr; if ( !NS_SUCCEEDED( helper(aIID, NS_REINTERPRET_CAST(void**, &newRawPtr)) ) ) newRawPtr = 0; - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = newRawPtr; + assign_assuming_AddRef(newRawPtr); } template void** nsCOMPtr::begin_assignment() { - if ( mRawPtr ) - { - NSCAP_RELEASE(mRawPtr); - mRawPtr = 0; - } - + assign_assuming_AddRef(0); return NS_REINTERPRET_CAST(void**, &mRawPtr); } #endif diff --git a/mozilla/xpcom/glue/nsCOMPtr.cpp b/mozilla/xpcom/glue/nsCOMPtr.cpp index a78b188733d..c95f17579e0 100644 --- a/mozilla/xpcom/glue/nsCOMPtr.cpp +++ b/mozilla/xpcom/glue/nsCOMPtr.cpp @@ -50,9 +50,7 @@ nsCOMPtr_base::assign_with_AddRef( nsISupports* rawPtr ) { if ( rawPtr ) NSCAP_ADDREF(rawPtr); - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = rawPtr; + assign_assuming_AddRef(rawPtr); } void @@ -61,19 +59,12 @@ nsCOMPtr_base::assign_from_helper( const nsCOMPtr_helper& helper, const nsIID& i nsISupports* newRawPtr; if ( !NS_SUCCEEDED( helper(iid, NS_REINTERPRET_CAST(void**, &newRawPtr)) ) ) newRawPtr = 0; - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = newRawPtr; + assign_assuming_AddRef(newRawPtr); } void** nsCOMPtr_base::begin_assignment() { - if ( mRawPtr ) - { - NSCAP_RELEASE(mRawPtr); - mRawPtr = 0; - } - + assign_assuming_AddRef(0); return NS_REINTERPRET_CAST(void**, &mRawPtr); } diff --git a/mozilla/xpcom/glue/nsCOMPtr.h b/mozilla/xpcom/glue/nsCOMPtr.h index 505bf343ac5..945d05f6f44 100644 --- a/mozilla/xpcom/glue/nsCOMPtr.h +++ b/mozilla/xpcom/glue/nsCOMPtr.h @@ -261,7 +261,7 @@ class NS_EXPORT nsQueryInterface : public nsCOMPtr_helper private: nsISupports* mRawPtr; - nsresult* mErrorPtr; + nsresult* mErrorPtr; }; inline @@ -373,6 +373,23 @@ class nsCOMPtr_base protected: nsISupports* mRawPtr; + + void + assign_assuming_AddRef( nsISupports* newPtr ) + { + /* + |AddRef()|ing the new value (before entering this function) before + |Release()|ing the old lets us safely ignore the self-assignment case. + We must, however, be careful only to |Release()| _after_ doing the + assignment, in case the |Release()| leads to our _own_ destruction, + which would, in turn, cause an incorrect second |Release()| of our old + pointer. Thank waterson@netscape.com for discovering this. + */ + nsISupports* oldPtr = mRawPtr; + mRawPtr = newPtr; + if ( oldPtr ) + NSCAP_RELEASE(oldPtr); + } }; // template class nsGetterAddRefs; @@ -389,6 +406,15 @@ class nsCOMPtr void assign_from_helper( const nsCOMPtr_helper&, const nsIID& ); void** begin_assignment(); + void + assign_assuming_AddRef( T* newPtr ) + { + T* oldPtr = mRawPtr; + mRawPtr = newPtr; + if ( oldPtr ) + NSCAP_RELEASE(oldPtr); + } + private: T* mRawPtr; @@ -495,10 +521,7 @@ class nsCOMPtr operator=( const nsDontAddRef& rhs ) // assign from |dont_AddRef(expr)| { - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = rhs.mRawPtr; - + assign_assuming_AddRef(rhs.mRawPtr); NSCAP_ASSERT_NO_QUERY_NEEDED(); return *this; } @@ -565,9 +588,7 @@ class nsCOMPtr #ifndef NSCAP_FEATURE_INLINE_STARTASSIGNMENT return NS_REINTERPRET_CAST(T**, begin_assignment()); #else - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = 0; + assign_assuming_AddRef(0); return NS_REINTERPRET_CAST(T**, &mRawPtr); #endif } @@ -658,10 +679,7 @@ class nsCOMPtr operator=( const nsDontAddRef& rhs ) // assign from |dont_AddRef(expr)| { - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = rhs.mRawPtr; - + assign_assuming_AddRef(rhs.mRawPtr); return *this; } @@ -726,9 +744,7 @@ class nsCOMPtr #ifndef NSCAP_FEATURE_INLINE_STARTASSIGNMENT return NS_REINTERPRET_CAST(nsISupports**, begin_assignment()); #else - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = 0; + assign_assuming_AddRef(0); return NS_REINTERPRET_CAST(nsISupports**, &mRawPtr); #endif } @@ -741,9 +757,7 @@ nsCOMPtr::assign_with_AddRef( nsISupports* rawPtr ) { if ( rawPtr ) NSCAP_ADDREF(rawPtr); - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = NS_REINTERPRET_CAST(T*, rawPtr); + assign_assuming_AddRef(NS_REINTERPRET_CAST(T*, rawPtr)); } template @@ -753,21 +767,14 @@ nsCOMPtr::assign_from_helper( const nsCOMPtr_helper& helper, const nsIID& aII T* newRawPtr; if ( !NS_SUCCEEDED( helper(aIID, NS_REINTERPRET_CAST(void**, &newRawPtr)) ) ) newRawPtr = 0; - if ( mRawPtr ) - NSCAP_RELEASE(mRawPtr); - mRawPtr = newRawPtr; + assign_assuming_AddRef(newRawPtr); } template void** nsCOMPtr::begin_assignment() { - if ( mRawPtr ) - { - NSCAP_RELEASE(mRawPtr); - mRawPtr = 0; - } - + assign_assuming_AddRef(0); return NS_REINTERPRET_CAST(void**, &mRawPtr); } #endif