diff --git a/cocos/2d/CCEventDispatcher.cpp b/cocos/2d/CCEventDispatcher.cpp index 6b12aa58e1..38ef104175 100644 --- a/cocos/2d/CCEventDispatcher.cpp +++ b/cocos/2d/CCEventDispatcher.cpp @@ -130,6 +130,16 @@ bool EventDispatcher::EventListenerVector::empty() const void EventDispatcher::EventListenerVector::push_back(EventListener* listener) { +#if CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS + 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!"); +#endif + if (listener->getFixedPriority() == 0) { if (_sceneGraphListeners == nullptr) @@ -314,6 +324,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 +455,69 @@ void EventDispatcher::addEventListenerWithSceneGraphPriority(EventListener* list addEventListener(listener); } +#if CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS && 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->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 CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS && COCOS2D_DEBUG > 0 + + void EventDispatcher::addEventListenerWithFixedPriority(EventListener* listener, int fixedPriority) { CCASSERT(listener, "Invalid parameters."); @@ -485,6 +563,7 @@ void EventDispatcher::removeEventListener(EventListener* listener) if (l->getSceneGraphPriority() != nullptr) { dissociateNodeAndEventListener(l->getSceneGraphPriority(), l); + l->setSceneGraphPriority(nullptr); // NULL out the node pointer so we don't have any dangling pointers to destroyed nodes. } if (_inDispatch == 0) @@ -519,6 +598,18 @@ void EventDispatcher::removeEventListener(EventListener* listener) setDirty(listener->getListenerID(), DirtyFlag::FIXED_PRIORITY); } } + +#if CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS + 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."); +#endif if (iter->second->empty()) { @@ -1127,6 +1218,7 @@ void EventDispatcher::removeEventListenersForListenerID(const EventListener::Lis if (l->getSceneGraphPriority() != nullptr) { dissociateNodeAndEventListener(l->getSceneGraphPriority(), l); + l->setSceneGraphPriority(nullptr); // NULL out the node pointer so we don't have any dangling pointers to destroyed nodes. } if (_inDispatch == 0) diff --git a/cocos/2d/CCEventDispatcher.h b/cocos/2d/CCEventDispatcher.h index 1178cb8444..002f42a224 100644 --- a/cocos/2d/CCEventDispatcher.h +++ b/cocos/2d/CCEventDispatcher.h @@ -140,6 +140,16 @@ public: /** Destructor of EventDispatcher */ ~EventDispatcher(); +#if CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS && 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; diff --git a/cocos/2d/CCNode.cpp b/cocos/2d/CCNode.cpp index b086275606..b739b58074 100644 --- a/cocos/2d/CCNode.cpp +++ b/cocos/2d/CCNode.cpp @@ -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 CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS && COCOS2D_DEBUG > 0 + _eventDispatcher->debugCheckNodeHasNoEventListenersOnDestruction(this); +#endif + + CC_SAFE_RELEASE(_eventDispatcher); } bool Node::init() diff --git a/cocos/2d/ccConfig.h b/cocos/2d/ccConfig.h index a58092458f..b9a7ef70a3 100644 --- a/cocos/2d/ccConfig.h +++ b/cocos/2d/ccConfig.h @@ -240,6 +240,17 @@ To enable set it to a value different than 0. Disabled by default. #define CC_LABELATLAS_DEBUG_DRAW 0 #endif +/** @def CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS + If enabled (in conjunction with assertion macros) will verify on Node destruction that the node being destroyed has no event + listeners still associated with it in the event dispatcher. This can be used to track down problems where the event dispatch + system has dangling pointers to destroyed nodes. + + Note: event listener verification will always be disabled in builds where assertions are disabled regardless of this setting. + */ +#ifndef CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS +#define CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS 0 +#endif + /** @def CC_ENABLE_PROFILERS If enabled, will activate various profilers within cocos2d. This statistical data will be output to the console once per second showing average time (in milliseconds) required to execute the specific routine(s). diff --git a/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.cpp b/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.cpp index 4e6cea901d..b7d7e72aa0 100644 --- a/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.cpp +++ b/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.cpp @@ -26,7 +26,8 @@ std::function createFunctions[] = CL(StopPropagationTest), CL(PauseResumeTargetTest), CL(Issue4129), - CL(Issue4160) + CL(Issue4160), + CL(DanglingNodePointersTest) }; unsigned int TEST_CASE_COUNT = sizeof(createFunctions) / sizeof(createFunctions[0]); @@ -1278,3 +1279,131 @@ std::string Issue4160::subtitle() const { return "Touch the red block twice \n should not crash and the red one couldn't be touched"; } + +// DanglingNodePointersTest +class DanglingNodePointersTestSprite : public Sprite +{ +public: + + typedef std::function TappedCallback; + + static DanglingNodePointersTestSprite * create(const TappedCallback & tappedCallback) + { + auto ret = new DanglingNodePointersTestSprite(tappedCallback); + + if (ret && ret->init()) + { + ret->autorelease(); + return ret; + } + + CC_SAFE_DELETE(ret); + return nullptr; + } + +protected: + + DanglingNodePointersTestSprite(const TappedCallback & tappedCallback) + : + _eventListener(nullptr), + _tappedCallback(tappedCallback) + { + + } + +public: + + void onEnter() override + { + Sprite::onEnter(); + + _eventListener = EventListenerTouchOneByOne::create(); + _eventListener->setSwallowTouches(false); + + _eventListener->onTouchBegan = [this](Touch* touch, Event* event) -> bool + { + _tappedCallback(this); + return false; // Don't claim the touch so it can propagate + }; + + _eventListener->onTouchEnded = [](Touch* touch, Event* event) + { + // Do nothing + }; + + _eventDispatcher->addEventListenerWithSceneGraphPriority(_eventListener, this); + } + + void onExit() override + { + _eventDispatcher->removeEventListenersForTarget(this); + _eventListener = nullptr; + Sprite::onExit(); + } + +private: + + EventListenerTouchOneByOne * _eventListener; + int _fixedPriority; + TappedCallback _tappedCallback; +}; + +DanglingNodePointersTest::DanglingNodePointersTest() +{ +#if CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS == 1 && COCOS2D_DEBUG > 0 + + Point origin = Director::getInstance()->getVisibleOrigin(); + Size size = Director::getInstance()->getVisibleSize(); + + auto callback2 = [](DanglingNodePointersTestSprite * sprite2) + { + CCASSERT(false, "This should never be called because the sprite gets removed from it's parent and destroyed!"); + exit(1); + }; + + auto callback1 = [callback2, origin, size](DanglingNodePointersTestSprite * sprite1) + { + DanglingNodePointersTestSprite * sprite2 = dynamic_cast(sprite1->getChildren().at(0)); + CCASSERT(sprite2, "The first child of sprite 1 should be sprite 2!"); + CCASSERT(sprite2->getReferenceCount() == 1, "There should only be 1 reference to sprite 1, from it's parent node. Hence removing it will destroy it!"); + sprite1->removeAllChildren(); // This call should cause sprite 2 to be destroyed + + // Recreate sprite 1 again + sprite2 = DanglingNodePointersTestSprite::create(callback2); + sprite2->setTexture("Images/MagentaSquare.png"); + sprite2->setPosition(origin+Point(size.width/2, size.height/2)); + sprite1->addChild(sprite2, -20); + }; + + auto sprite1 = DanglingNodePointersTestSprite::create(callback1); // Sprite 1 will receive touch before sprite 2 + sprite1->setTexture("Images/CyanSquare.png"); + sprite1->setPosition(origin+Point(size.width/2, size.height/2)); + addChild(sprite1, -10); + + auto sprite2 = DanglingNodePointersTestSprite::create(callback2); // Sprite 2 will be removed when sprite 1 is touched, should never receive an event. + sprite2->setTexture("Images/MagentaSquare.png"); + sprite2->setPosition(origin+Point(size.width/2, size.height/2)); + sprite1->addChild(sprite2, -20); + +#endif +} + +DanglingNodePointersTest::~DanglingNodePointersTest() +{ + +} + +std::string DanglingNodePointersTest::title() const +{ + return "DanglingNodePointersTest"; +} + +std::string DanglingNodePointersTest::subtitle() const +{ +#if CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS == 1 && COCOS2D_DEBUG > 0 + return "Tap the square - should not crash!"; +#else + return "For test to work, must be compiled with:\n" + "CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS == 1\n&& COCOS2D_DEBUG > 0"; +#endif +} diff --git a/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.h b/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.h index e35035b4c6..74c692d49c 100644 --- a/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.h +++ b/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.h @@ -206,4 +206,15 @@ public: private: }; +class DanglingNodePointersTest : public EventDispatcherTestDemo +{ +public: + CREATE_FUNC(DanglingNodePointersTest); + DanglingNodePointersTest(); + virtual ~DanglingNodePointersTest(); + + virtual std::string title() const override; + virtual std::string subtitle() const override; +}; + #endif /* defined(__samples__NewEventDispatcherTest__) */