From 79b5dff795052b8184493d495d688d5ea0f2ae24 Mon Sep 17 00:00:00 2001 From: Darragh Coy Date: Mon, 7 Apr 2014 16:19:26 -0700 Subject: [PATCH 1/3] CCEventDispatcher crash fix Fix possible crashes which could be caused by the EventDispatcher having listeners associated with nodes are destroyed. Catch the case where a node registers a listener while we are dispatching events and gets destroyed while that event is still being dispatched. Check the list of nodes to be added into the event dispatcher fully when we are cleaning listeners for a target node. This issue was found using the extra debug verification in this pull request: https://github.com/cocos2d/cocos2d-x/pull/6011 --- cocos/2d/CCEventDispatcher.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/cocos/2d/CCEventDispatcher.cpp b/cocos/2d/CCEventDispatcher.cpp index 15e3dfc2a0..c673f1c6b4 100644 --- a/cocos/2d/CCEventDispatcher.cpp +++ b/cocos/2d/CCEventDispatcher.cpp @@ -340,6 +340,26 @@ void EventDispatcher::removeEventListenersForTarget(Node* target, bool recursive } } + // Bug fix: ensure there are no references to the node in the list of listeners to be added. + // If we find any listeners associated with the destroyed node in this list then remove them. + // This is to catch the scenario where the node gets destroyed before it's listener + // is added into the event dispatcher fully. This could happen if a node registers a listener + // and gets destroyed while we are dispatching an event (touch etc.) + for (auto iter = _toAddedListeners.begin(); iter != _toAddedListeners.end(); ) + { + EventListener * listener = *iter; + + if (listener->getSceneGraphPriority() == target) + { + listener->release(); + iter = _toAddedListeners.erase(iter); + } + else + { + ++iter; + } + } + if (recursive) { const auto& children = target->getChildren(); From bb85df66b1b49705f120edc0ceceafeede1e7347 Mon Sep 17 00:00:00 2001 From: Darragh Coy Date: Mon, 7 Apr 2014 16:46:03 -0700 Subject: [PATCH 2/3] Add an event dispatcher test Add an event dispatcher test to help reproduce crashes fixed by the previous commit and to verify that the fix still works. --- .../NewEventDispatcherTest.cpp | 45 ++++++++++++++++++- .../NewEventDispatcherTest.h | 10 +++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.cpp b/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.cpp index 77249f91e0..337a8164a2 100644 --- a/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.cpp +++ b/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.cpp @@ -8,6 +8,7 @@ #include "NewEventDispatcherTest.h" #include "testResource.h" +#include "CCAutoreleasePool.h" namespace { @@ -27,7 +28,8 @@ std::function createFunctions[] = CL(PauseResumeTargetTest), CL(Issue4129), CL(Issue4160), - CL(DanglingNodePointersTest) + CL(DanglingNodePointersTest), + CL(RegisterAndUnregisterWhileEventHanldingTest) }; unsigned int TEST_CASE_COUNT = sizeof(createFunctions) / sizeof(createFunctions[0]); @@ -1413,3 +1415,44 @@ std::string DanglingNodePointersTest::subtitle() const "CC_NODE_DEBUG_VERIFY_EVENT_LISTENERS == 1\n&& COCOS2D_DEBUG > 0"; #endif } + + +RegisterAndUnregisterWhileEventHanldingTest::RegisterAndUnregisterWhileEventHanldingTest() +{ + Point origin = Director::getInstance()->getVisibleOrigin(); + Size size = Director::getInstance()->getVisibleSize(); + + auto callback1 = [=](DanglingNodePointersTestSprite * sprite) + { + auto callback2 = [](DanglingNodePointersTestSprite * sprite) + { + CCASSERT(false, "This should never get called!"); + }; + + { + AutoreleasePool pool; + + auto sprite2 = DanglingNodePointersTestSprite::create(callback2); + sprite2->setTexture("Images/CyanSquare.png"); + sprite2->setPosition(origin+Point(size.width/2, size.height/2)); + + addChild(sprite2, 0); + removeChild(sprite2); + } + }; + + auto sprite1 = DanglingNodePointersTestSprite::create(callback1); + sprite1->setTexture("Images/CyanSquare.png"); + sprite1->setPosition(origin+Point(size.width/2, size.height/2)); + addChild(sprite1, -10); +} + +std::string RegisterAndUnregisterWhileEventHanldingTest::title() const +{ + return "RegisterAndUnregisterWhileEventHanldingTest"; +} + +std::string RegisterAndUnregisterWhileEventHanldingTest::subtitle() const +{ + return "Tap the square multiple times - should not crash!"; +} diff --git a/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.h b/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.h index 74c692d49c..64b52ed037 100644 --- a/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.h +++ b/tests/cpp-tests/Classes/NewEventDispatcherTest/NewEventDispatcherTest.h @@ -217,4 +217,14 @@ public: virtual std::string subtitle() const override; }; +class RegisterAndUnregisterWhileEventHanldingTest : public EventDispatcherTestDemo +{ +public: + CREATE_FUNC(RegisterAndUnregisterWhileEventHanldingTest); + RegisterAndUnregisterWhileEventHanldingTest(); + + virtual std::string title() const override; + virtual std::string subtitle() const override; +}; + #endif /* defined(__samples__NewEventDispatcherTest__) */ From 546db2de4afd65c01f94a0a2975066b36e740731 Mon Sep 17 00:00:00 2001 From: James Chen Date: Tue, 8 Apr 2014 11:17:38 +0800 Subject: [PATCH 3/3] Unregistered listener when it's removed from _toAddedListeners. --- cocos/2d/CCEventDispatcher.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/cocos/2d/CCEventDispatcher.cpp b/cocos/2d/CCEventDispatcher.cpp index c673f1c6b4..93e3c85bd7 100644 --- a/cocos/2d/CCEventDispatcher.cpp +++ b/cocos/2d/CCEventDispatcher.cpp @@ -351,6 +351,7 @@ void EventDispatcher::removeEventListenersForTarget(Node* target, bool recursive if (listener->getSceneGraphPriority() == target) { + listener->setRegistered(false); listener->release(); iter = _toAddedListeners.erase(iter); }