Menu
▾
▴
#101 Loki::COMRefCounted doesn't call AddRef() on assignment
Version: loki-0.1.4
If you try to assign a COM object to a Loki smart
pointer it is wrapped into a smart pointer before it is
assigned. This wrapping smart pointer doesn't call
AddRef() but Release() when it goes out of scope and
deletes the object.
typedef ::Loki::SmartPtr<CMyObject,
::Loki::COMRefCounted, ::Loki::AllowConversion>
MyObjectPtr;
...
{
MyObjectPtr ptr;
ptr = obj; // obj is raw COM object
}
// CRASH!
Internally it's calling looks like:
...
{
MyObjectPtr ptr;
ptr = MyObjectPtr(obj); // obj is raw COM object
}
Unfortunately SmartPtr(ImplicitArg p) uses default
constructor of ownership policy which doesn't call
AddRef().
I added an additional constructor to each ownership
policy which allows special treatment of that case.
Best regards,
Thomas Albrecht
Diff with changes to fix Loki::COMRefCounted.
Logged In: YES
user_id=1159765
Thanks Thomas,
when I've more time I'll have a more closer look at it.
Peter
Logged In: YES
user_id=749922
I tested the behavior of SmartPtr with the COMRefCounted
Ownership policy. I did verify that it does not call the
shared object's AddRef function when constructor is passed a
pointer to shared object.
I reviewed the diff file, and tried the fix suggested by
Thomas Albrecht. His changes do fix the bug as intended.
I added more tests to the SmartPtr test suite to check for
this specific bug. I can add the new tests and the fix to
CVS later. (Can't connect to CVS right now.)
And many thanks to Thomas!
Cheers,
Rich
Logged In: YES
user_id=749922
Fixed in Rev 1.29.
Logged In: YES
user_id=479642
This looks very wrong to me. I believe the original behavior
was by design. It's conventional that when a function gives
out a (raw) COM interface pointer it is the responsibility
of the giving function to do the AddRef(). From the
"Managing a COM Object's Lifetime" section in the
crash-course guide to COM that's included in the Microsoft
DirectX SDK:
"Whenever you obtain a new interface pointer, the reference
count must be incremented by a call to AddRef. However, your
application does not usually need to call this method. If
you obtain an interface pointer by calling an object
creation method or by calling QueryInterface, the object
will automatically increment the reference count. However,
if you create an interface pointer in some other way, such
as copying an existing pointer, you must explicitly call
AddRef. Otherwise, when you release the original interface
pointer, the object may be destroyed even though you may
still need to use the copy of the pointer."
As such, it makes sense to me that constructing a COM
SmartPtr with a raw pointer will take ownership of the raw
pointer without doing an AddRef(). However, I will grant
that many of the other COM smart pointer classes out there
do as the patch does and AddRef() when assigned a raw COM
pointer value. If, for some reason, you did want this
behavior with SmartPtr then it could be achieved by
implementing your own StoragePolicy.
So, as I said, I'm pretty sure the original SmartPtr
behavior was a design decision and not a bug. It could be
argued that since it is different to many other COM smart
pointer classes it should be changed but:
a) I think it makes more sense not to AddRef() for the
reasons mentioned above (I can go into more detail on this
if needed),
b) it changes the established behavior for all existing
users of COMRefCounted, who will be used to the
non-AddRef()ing behavior,
c) this proposed "fix" makes a change to the OwnershipPolicy
interface, affecting every user that has written a custom
OwnershipPolicy (whether they care about COM or not).
Since it breaks backwards compatibility, surely this patch
should have been discussed before being committed?
Logged In: YES
user_id=749922
I did a Google search on QueryInterface, COM, and AddRef.
I checked several links in the search result about whether
QueryInterface does (or should) automatically call AddRef.
All the ones I checked said that QueryInterface is supposed
to call AddRef, and that AddRef is explicitly called only
when one pointer is assigned from another. I found no
websites in the search results which suggested otherwise -
which confirms what jjc1138 wrote.
Thomas, please check if the QueryInterface functions your
code calls already call AddRef. I suspect the problem you
found might be in the QueryInterface functions your code
calls, rather than in the SmartPtr policy.
jjc1138, since the change to SmartPtr.h was after the last
release of Loki, it would only affect those who download
directly from CVS. Because no changes were made to
SmartPtr.h after that, we should be able to correct this
with minimal fuss.
Thanks!
Rich
Logged In: YES
user_id=1159765
+
+ COMRefCounted(const P& val)
+ {
+ if(val!=0)
+ val->AddRef();
+ }
+
causes a regression in RegressionTest/SmartPtrTest.h
Logged In: YES
user_id=1159765
Hi Rich,
this sounds like you don't wanna apply Thomas patch:
> which confirms what jjc1138 wrote.
>
> Thomas, please check if the QueryInterface functions your
> code calls already call AddRef. I suspect the problem you
> found might be in the QueryInterface functions your code
> calls, rather than in the SmartPtr policy.
Because of this, your last message, jjc1138 arguments and
the regression bug we maybe should undo the 1.29 patch
before the next release (at least the AddRef in the
COMRefCounted constructor).
Logged In: YES
user_id=749922
Hi Peter,
> this sounds like you don't wanna apply Thomas patch:
Yes, that is correct. I now think the change was made when
I had inaccurate knowledge of the problem. Let's undo the
whole change, rather than just removing the COMRefCounted
constructor. (Without that constructor, the other changes
are entirely superfluous.)
Rich
Logged In: YES
user_id=1159765
I've reverted to version 1.28:
Modified Files:
SmartPtr.h
Log Message:
undo commit 1.29, reject bug: [ 1459838 ]
Loki::COMRefCounted doesn't call AddRef() on assignment
Index: SmartPtr.h
{}
@@ -337,16 +329,6 @@
ThreadingModel<RefCountedMT,
MX>::AtomicAssign(*pCount_, 1);
}
@@ -544,10 +514,7 @@
public:
DestructiveCopy()
{}
-
- DestructiveCopy( const P & )
- {}
-
+
template <class P1>
DestructiveCopy(const DestructiveCopy<P1>&)
{}
@@ -583,10 +550,7 @@
public:
NoCopy()
{}
-
- NoCopy( const P & )
- {}
-
+
template <class P1>
NoCopy(const NoCopy<P1>&)
{}
@@ -655,8 +619,6 @@
/// Well, it's clear what it does :o)
////////////////////////////////////////////////////////////////////////////////
- template < class P > struct AssertCheck;
-
template <class P>
struct NoCheck
{
@@ -667,10 +629,6 @@
NoCheck(const NoCheck<P1>&)
{}
- template <class P1>
- NoCheck(const AssertCheck<P1>&)
- {}
-
static void OnDefault(const P&)
{}
@@ -1005,10 +963,10 @@
{ KP::OnDefault(GetImpl(*this)); }
explicit
- SmartPtr(ExplicitArg p) : SP(p), OP(p)
+ SmartPtr(ExplicitArg p) : SP(p)
{ KP::OnInit(GetImpl(*this)); }
- SmartPtr(ImplicitArg p) : SP(p), OP(p)
+ SmartPtr(ImplicitArg p) : SP(p)
{ KP::OnInit(GetImpl(*this)); }
SmartPtr(CopyArg& rhs)
@@ -1561,7 +1519,11 @@
#endif // SMARTPTR_INC_
+
// $Log$
+// Revision 1.30 2006/05/17 16:04:32 syntheticpp
+// undo commit 1.29, reject bug: [ 1459838 ]
Loki::COMRefCounted doesn't call AddRef() on assignment
+//
// Revision 1.29 2006/04/05 22:41:43 rich_sposato
// Fixed bug 1459838 using fix made by Thomas Albrecht.
(Thanks!)
// Also added a constructor for NoCheck policy.