Can the assignment of a shared_ptr trash the `this` pointer

Question

Let's take the following example of a data structure (Node) that represents a tree of child nodes. The set of child nodes for each object is stored in a map>

class Node;
typedef std::shared_ptr<Node> NodePtr;

class Node
{
    std::map<const std::string, NodePtr> _childNodes;
    void SomeOtherMethod();

public:
    bool GetChildByKeyName(/*In*/ const std::string& key, /*Out*/ NodePtr& spChild)
    {
        bool result = false;
        auto itor = _childNodes.find(key);
        if (itor != _childNodes.end())
        {
            spChild = itor->second;
            result = true;
            SomeOtherMethod();
        }
        return result;
    }
};

And the following code sample as users would typically call it.

NodePtr spNode, spChildNode;
bool result;
...
result = spNode->GetChildByKeyName(strChildKeyName, spChildNode);

So far so good.

It occurred to me that callers might way to traverse the tree without having to deal with extra variables for each depth into the tree

NodePtr spNode;
bool result;

result = spNode->GetChildItem(strChildKeyName, spNode);
if (result)
   spNode->GetChildItem(strSubKeyName, spNode);

In the above case, if spNode is the final remaining reference to the object, then I'm worried about this block of code in the GetChildItem method:

            spChild = itor->second;
            result = true;
            SomeOtherMethod();

Does the assignment of spChild (which is really the caller's spNode instance) inadvertently destruct "this" Node since the last reference just went away? (And hence calling other methods after the spChild assignment is dangerous). Do I have a potential bug here?

I think the workaround is to simply add this line at the top of the method call:

NodePtr spChildRef = spChild; // maintain a reference to the caller's original node during the scope of the method

Thoughts?

Answer 1

You are correct that if the outermost spNode pointer in your second example is the only reference to the root item, GetChildByKeyName will replace that reference causing the object to destruct (essentially "delete this").

I realize this might not be the full code and there may be reasons for why you have designed it like this, but I would personally suggest to change the interface to return the found child instead of using an out parameter. (You can still distinguish between success & failure in finding the child by testing for null.)

Not only does the actual find code become simpler:

NodePtr GetChildByKeyName(/*In*/ const std::string& key)
{
    auto itor = _childNodes.find(key);
    if (itor != _childNodes.end())
    {
        SomeOtherMethod();
        return itor->second;
    }
    return nullptr;
}

You can then also reuse the pointer to your heart's content:

NodePtr spNode; 
....

spNode = spNode->GetChildItem(strChildKeyName);
if (spNode)
    spNode = spNode->GetChildItem(strSubKeyName);

Answer 2

+1 to @Dentoid's answer. I'm not going to duplicate his answer here. I'll only show if your existing code has a problem.

---

Can the assigment of a shared_ptr trash the this pointer?

Yes, it does.

I have made a test to determine it: http://coliru.stacked-crooked.com/a/ef0d4f92902b4dee

Its output (formatted for clarity):

spNode before: 0x15a1028
   this: 0x15a1028 <---------------------------------------------------------|
   spChild.get() == this                                                     |
                                                                             |
   spChild before: 0x15a1028 <-------- Notice: They're the same -------------|
      Node 0x15a1028 destroyed.                                              |
             ^---------------------------------------------------------------|
   spChild after: 0x15a1078                                                  |
                                                                             |
   SomeOtherMethod() @0x15a1028; size: 1 |                                   |
spNode after: 0x15a1078     ^------------------------------------------------|

Node 0x15a1078 destroyed.

So SomeOtherMethod() is called on an already destroyed object, though the runtime hasn't been able to detect this.

Applying your work-around, http://coliru.stacked-crooked.com/a/f0042d4b46fed340

spNode before: 0x19a7028
   this: 0x19a7028
   spChild.get() == this

   spChild before: 0x19a7028
   spChild after: 0x19a7078

   SomeOtherMethod() @0x19a7028; size: 1

   Node 0x19a7028 destroyed.
spNode after: 0x19a7078

Node 0x19a7078 destroyed.

It doesn't have the problem anymore.