All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

FIX STORM-1417 viewer may crash when inviting people to join group.

Review Request #390 - Created July 12, 2011 and updated

Alain Linden Reviewers
https://bitbucket.org/squire_linden/viewer-development-crash-horde/
STORM-1471 stone.linden, nat_linden
None viewer-development
I changed the avatar name cache callbacks to accept weak pointers to prevent crashing when the invite is sent before the name lookup completes.  

 
Posted (July 12, 2011, 1:05 p.m.)

   

  
indra/newview/llpanelgroupinvite.cpp (Diff revision 1)
 
 
But wait. From http://www.boost.org/doc/libs/1_47_0/libs/smart_ptr/weak_ptr.htm :

"When the last shared_ptr to the object goes away and the object is deleted, the attempt to obtain a shared_ptr from the weak_ptr instances that refer to the deleted object will fail: the constructor will throw an exception of type boost::bad_weak_ptr, and weak_ptr::lock will return an empty shared_ptr."

Wouldn't panel.lock()->addUsers(...) fail in that case? Shouldn't we test with an idiom like this?

boost::shared_ptr<impl> spanel(panel.lock());
if (spanel) { spanel->addUsers(...); }

Or hmm! The weak_ptr page cited above suggests this cool syntax:

if(boost::shared_ptr<impl> spanel = panel.lock())
{
    spanel->addUsers(...);
}
Posted (July 12, 2011, 1:53 p.m.)

   

  
indra/newview/llpanelgroupinvite.cpp (Diff revision 1)
 
 
note that this block is protected by a if(!panel.expired()) above.  Expired means the object pointed to is no longer in use, so if it isn't expired the weak pointer will return a shared pointer to the real object.
  1. Sorry, I spaced on the expired() call -- thought that was an operation on the panel rather than on the weak pointer.
    
    I'd still be more comfortable if the lock(), the creation of the temp shared_ptr, happened up where you have the expired() call -- whether or not you use the cool if (decl) syntax. Then you wouldn't need a separate expired() call.