Fix crashes in event dispatch other related safety checks/fixes

1: Add a fix to prevent events from being sent to scene nodes which are destroyed. Previously when a node was being destroyed it would try to unregister any event listeners it would have. This process would fail however in the case where event dispatching was already underway because we can't modify the listeners list while we are iterating through it. This is a pretty common occurrence since we often tear down & switch scenes in response to button clicks etc. Fix the problem by nulling out the slot for the listener we are removing in this case, so that it's node no longer receives any events after it is destroyed. The event dispatching code will cleanup this unused/nulled slot once the event processing loop has terminated.

2: When removing an event listener in cocos2d::EventDispatcher, ensure immediately it gets removed from the node priority map and dirty nodes set in order to ensure we don't inadvertently access any dangling pointers to nodes.

3: Add debug checks to ensure nodes have no associated event listeners after they are destroyed. This check will catch the problem fixed by (1) if it is still present.

4: Add some extra debug sanity checks in the event dispatcher to ensure nodes are not added to the lists twice and that they are being properly removed from the lists under certain conditions.

5: In cocos2d::Node's destructor NULL out members after releasing them in case they somehow happen to be accessed during the destructor. Move the release of the event dispatcher to the very end, since we need to check against it in debug.
This commit is contained in:
Darragh Coy 2014-03-26 13:12:44 -07:00
parent 5ba2cbb5a0
commit 0ee1095e37
3 changed files with 153 additions and 8 deletions

View File

@ -130,6 +130,14 @@ bool EventDispatcher::EventListenerVector::empty() const
void EventDispatcher::EventListenerVector::push_back(EventListener* listener)
{
CCASSERT(_sceneGraphListeners == nullptr ||
std::count(_sceneGraphListeners->begin(), _sceneGraphListeners->end(), listener) == 0,
"Listener should not be added twice!");
CCASSERT(_fixedListeners == nullptr ||
std::count(_fixedListeners->begin(), _fixedListeners->end(), listener) == 0,
"Listener should not be added twice!");
if (listener->getFixedPriority() == 0)
{
if (_sceneGraphListeners == nullptr)
@ -314,6 +322,11 @@ void EventDispatcher::resumeEventListenersForTarget(Node* target, bool recursive
void EventDispatcher::removeEventListenersForTarget(Node* target, bool recursive/* = false */)
{
// Ensure the node is removed from these immediately also.
// Don't want any dangling pointers or the possibility of dealing with deleted objects..
_nodePriorityMap.erase(target);
_dirtyNodes.erase(target);
auto listenerIter = _nodeListenersMap.find(target);
if (listenerIter != _nodeListenersMap.end())
{
@ -440,6 +453,79 @@ void EventDispatcher::addEventListenerWithSceneGraphPriority(EventListener* list
addEventListener(listener);
}
#if COCOS2D_DEBUG > 0
void EventDispatcher::debugCheckNodeHasNoEventListenersOnDestruction(Node* node)
{
// Check the listeners map
for (const auto & keyValuePair : _listenerMap)
{
const EventListenerVector * eventListenerVector = keyValuePair.second;
if (eventListenerVector)
{
if (eventListenerVector->getFixedPriorityListeners())
{
for (EventListener * listener : *eventListenerVector->getFixedPriorityListeners())
{
CCASSERT(!listener ||
listener->getSceneGraphPriority() != node,
"Node should have no event listeners registered for it upon destruction!");
}
}
if (eventListenerVector->getSceneGraphPriorityListeners())
{
for (EventListener * listener : *eventListenerVector->getSceneGraphPriorityListeners())
{
CCASSERT(!listener ||
listener->getSceneGraphPriority() != node,
"Node should have no event listeners registered for it upon destruction!");
}
}
}
}
// Check the node listeners map
for (const auto & keyValuePair : _nodeListenersMap)
{
CCASSERT(keyValuePair.first != node, "Node should have no event listeners registered for it upon destruction!");
if (keyValuePair.second)
{
for (EventListener * listener : *keyValuePair.second)
{
CCASSERT(listener->getSceneGraphPriority() != node,
"Node should have no event listeners registered for it upon destruction!");
}
}
}
// Check the node priority map
for (const auto & keyValuePair : _nodePriorityMap)
{
CCASSERT(keyValuePair.first != node,
"Node should have no event listeners registered for it upon destruction!");
}
// Check the to be added list
for (EventListener * listener : _toAddedListeners)
{
CCASSERT(listener->getSceneGraphPriority() != node,
"Node should have no event listeners registered for it upon destruction!");
}
// Check the dirty nodes set
for (Node * dirtyNode : _dirtyNodes)
{
CCASSERT(dirtyNode != node,
"Node should have no event listeners registered for it upon destruction!");
}
}
#endif // #if COCOS2D_DEBUG > 0
void EventDispatcher::addEventListenerWithFixedPriority(EventListener* listener, int fixedPriority)
{
CCASSERT(listener, "Invalid parameters.");
@ -492,7 +578,14 @@ void EventDispatcher::removeEventListener(EventListener* listener)
listeners->erase(iter);
CC_SAFE_RELEASE(l);
}
else
{
// Fix to prevent events propagating to nodes or listeners that are destroyed:
// Since we can't remove the listener while we're iterating the listeners list,
// null out the pointer. We'll erase the actual pointer storage later.
*iter = nullptr;
}
isFound = true;
break;
}
@ -519,6 +612,16 @@ void EventDispatcher::removeEventListener(EventListener* listener)
setDirty(listener->getListenerID(), DirtyFlag::FIXED_PRIORITY);
}
}
CCASSERT(_inDispatch != 0 ||
!sceneGraphPriorityListeners ||
std::count(sceneGraphPriorityListeners->begin(), sceneGraphPriorityListeners->end(), listener) == 0,
"Listener should be in no lists after this is done if we're not currently in dispatch mode.");
CCASSERT(_inDispatch != 0 ||
!fixedPriorityListeners ||
std::count(fixedPriorityListeners->begin(), fixedPriorityListeners->end(), listener) == 0,
"Listener should be in no lists after this is done if we're not currently in dispatch mode.");
if (iter->second->empty())
{
@ -613,6 +716,11 @@ void EventDispatcher::dispatchEventToListeners(EventListenerVector* listeners, c
// priority == 0, scene graph priority
for (auto& l : *sceneGraphPriorityListeners)
{
if (!l)
{
continue; // Event listener slot which was temporarily nulled because we removed the listener while dispatching events.
}
if (!l->isPaused() && l->isRegistered() && onEvent(l))
{
shouldStopPropagation = true;
@ -900,7 +1008,13 @@ void EventDispatcher::updateListeners(Event* event)
for (auto iter = sceneGraphPriorityListeners->begin(); iter != sceneGraphPriorityListeners->end();)
{
auto l = *iter;
if (!l->isRegistered())
if (!l)
{
// Event listener slot which was temporarily nulled because we removed the listener while dispatching events.
iter = sceneGraphPriorityListeners->erase(iter);
}
else if (!l->isRegistered())
{
iter = sceneGraphPriorityListeners->erase(iter);
l->release();
@ -917,7 +1031,13 @@ void EventDispatcher::updateListeners(Event* event)
for (auto iter = fixedPriorityListeners->begin(); iter != fixedPriorityListeners->end();)
{
auto l = *iter;
if (!l->isRegistered())
if (!l)
{
// Event listener slot which was temporarily nulled because we removed the listener while dispatching events.
iter = fixedPriorityListeners->erase(iter);
}
else if (!l->isRegistered())
{
iter = fixedPriorityListeners->erase(iter);
l->release();
@ -1123,6 +1243,14 @@ void EventDispatcher::removeEventListenersForListenerID(const EventListener::Lis
for (auto iter = listenerVector->begin(); iter != listenerVector->end();)
{
auto l = *iter;
if (!l)
{
// Listener slot which was erased by nulling out the slot.
// This happens when we try to remove a listener while dispatching events.
continue;
}
l->setRegistered(false);
if (l->getSceneGraphPriority() != nullptr)
{

View File

@ -140,6 +140,16 @@ public:
/** Destructor of EventDispatcher */
~EventDispatcher();
#if COCOS2D_DEBUG > 0
/**
* To help track down event listener issues in debug builds.
* Verifies that the node has no event listeners associated with it when destroyed.
*/
void debugCheckNodeHasNoEventListenersOnDestruction(Node* node);
#endif
protected:
friend class Node;

View File

@ -154,15 +154,15 @@ Node::~Node()
}
#endif
CC_SAFE_RELEASE(_actionManager);
CC_SAFE_RELEASE(_scheduler);
CC_SAFE_RELEASE_NULL(_actionManager);
CC_SAFE_RELEASE_NULL(_scheduler);
_eventDispatcher->removeEventListenersForTarget(this);
CC_SAFE_RELEASE(_eventDispatcher);
// attributes
CC_SAFE_RELEASE(_shaderProgram);
CC_SAFE_RELEASE(_userObject);
CC_SAFE_RELEASE_NULL(_shaderProgram);
CC_SAFE_RELEASE_NULL(_userObject);
for (auto& child : _children)
{
@ -175,7 +175,14 @@ Node::~Node()
#if CC_USE_PHYSICS
setPhysicsBody(nullptr);
#endif
#if COCOS2D_DEBUG > 0
_eventDispatcher->debugCheckNodeHasNoEventListenersOnDestruction(this);
#endif
CC_SAFE_RELEASE(_eventDispatcher);
}
bool Node::init()