From: Julien Heanley Date: Wed, 30 Apr 2014 12:33:40 +0000 (+0100) Subject: (Observers)Fix memory issues during observer iteration X-Git-Tag: dali-2014-wk20-release~23 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=refs%2Fchanges%2F94%2F20294%2F1;p=platform%2Fcore%2Fuifw%2Fdali-core.git (Observers)Fix memory issues during observer iteration [Issue#] N/A [Problem] Crash could occur when adding a previously removed actor back into the scene [Cause] Observer classes were using SceneObjectRemoved callback to remove themselves from that object's observer list while the object is in process of iterating over the list [Solution] Mark object as disconnected from scene and remove observer later Change-Id: I552017a0d8284973f565cd08275f1270fc8e9668 Signed-off-by: David Steele --- diff --git a/dali/internal/event/events/gesture-processor.cpp b/dali/internal/event/events/gesture-processor.cpp index 1858db5..f08ef86 100644 --- a/dali/internal/event/events/gesture-processor.cpp +++ b/dali/internal/event/events/gesture-processor.cpp @@ -30,7 +30,8 @@ namespace Internal { GestureProcessor::GestureProcessor() -: mCurrentGesturedActor( NULL ) +: mCurrentGesturedActor( NULL ), + mGesturedActorDisconnected(false) { } @@ -156,6 +157,7 @@ void GestureProcessor::SetActor( Dali::Actor actor ) mCurrentGesturedActor = &GetImplementation( actor ); mCurrentGesturedActor->AddObserver( *this ); } + mGesturedActorDisconnected = false; } void GestureProcessor::ResetActor() @@ -164,18 +166,25 @@ void GestureProcessor::ResetActor() { mCurrentGesturedActor->RemoveObserver( *this ); mCurrentGesturedActor = NULL; + mGesturedActorDisconnected = false; } } +Actor* GestureProcessor::GetCurrentGesturedActor() +{ + return mGesturedActorDisconnected ? NULL : mCurrentGesturedActor; +} + void GestureProcessor::SceneObjectRemoved(ProxyObject& proxy) { - if ( mCurrentGesturedActor == &proxy ) + if ( mCurrentGesturedActor == &proxy && + !mGesturedActorDisconnected ) { // Inform deriving classes. OnGesturedActorStageDisconnection(); - proxy.RemoveObserver( *this ); - mCurrentGesturedActor = NULL; + // do not call proxy.RemoveObserver here, proxy is currently iterating through observers... you wouldnt want to upset proxy now would you? + mGesturedActorDisconnected = true; } } diff --git a/dali/internal/event/events/gesture-processor.h b/dali/internal/event/events/gesture-processor.h index 6ad6977..9441542 100644 --- a/dali/internal/event/events/gesture-processor.h +++ b/dali/internal/event/events/gesture-processor.h @@ -146,6 +146,13 @@ protected: // Construction & Destruction */ void ResetActor(); + /** + * Returns the current gestured actor if it is on stage + * + * @return The current gestured actor + */ + Actor* GetCurrentGesturedActor(); + // For derived classes to override /** @@ -191,9 +198,10 @@ private: */ void OnStageDisconnection( Dali::Actor actor ); -protected: // Data +private: // Data - Actor* mCurrentGesturedActor; ///< The current actor that has been gestured. + Actor* mCurrentGesturedActor; ///< The current actor that has been gestured. + bool mGesturedActorDisconnected; ///< Indicates whether the gestured actor has been disconnected from the scene }; } // namespace Internal diff --git a/dali/internal/event/events/long-press-gesture-processor.cpp b/dali/internal/event/events/long-press-gesture-processor.cpp index 474b454..1a83a67 100644 --- a/dali/internal/event/events/long-press-gesture-processor.cpp +++ b/dali/internal/event/events/long-press-gesture-processor.cpp @@ -180,12 +180,13 @@ void LongPressGestureProcessor::Process( const Integration::LongPressGestureEven case Gesture::Started: { - if ( mCurrentGesturedActor ) + Actor* currentGesturedActor = GetCurrentGesturedActor(); + if ( currentGesturedActor ) { HitTestAlgorithm::Results hitTestResults; HitTestAlgorithm::HitTest( mStage, longPressEvent.point, hitTestResults ); - if ( hitTestResults.actor && ( mCurrentGesturedActor == &GetImplementation( hitTestResults.actor ) ) ) + if ( hitTestResults.actor && ( currentGesturedActor == &GetImplementation( hitTestResults.actor ) ) ) { // Record the current render-task for Screen->Actor coordinate conversions mCurrentRenderTask = hitTestResults.renderTask; @@ -212,21 +213,22 @@ void LongPressGestureProcessor::Process( const Integration::LongPressGestureEven // Only send subsequent long press gesture signals if we processed the gesture when it started. // Check if actor is still touchable. - if ( mCurrentGesturedActor ) + Actor* currentGesturedActor = GetCurrentGesturedActor(); + if ( currentGesturedActor ) { - if ( mCurrentGesturedActor->IsHittable() && !mCurrentEmitters.empty() && mCurrentRenderTask ) + if ( currentGesturedActor->IsHittable() && !mCurrentEmitters.empty() && mCurrentRenderTask ) { // Ensure actor is still attached to the emitters, if it is not then remove the emitter. - LongPressGestureDetectorContainer::iterator endIter = std::remove_if( mCurrentEmitters.begin(), mCurrentEmitters.end(), IsNotAttachedFunctor(mCurrentGesturedActor) ); + LongPressGestureDetectorContainer::iterator endIter = std::remove_if( mCurrentEmitters.begin(), mCurrentEmitters.end(), IsNotAttachedFunctor(currentGesturedActor) ); mCurrentEmitters.erase( endIter, mCurrentEmitters.end() ); if ( !mCurrentEmitters.empty() ) { Vector2 actorCoords; RenderTask& renderTaskImpl( GetImplementation( mCurrentRenderTask ) ); - mCurrentGesturedActor->ScreenToLocal( renderTaskImpl, actorCoords.x, actorCoords.y, longPressEvent.point.x, longPressEvent.point.y ); + currentGesturedActor->ScreenToLocal( renderTaskImpl, actorCoords.x, actorCoords.y, longPressEvent.point.x, longPressEvent.point.y ); - EmitLongPressSignal( Dali::Actor( mCurrentGesturedActor ), mCurrentEmitters, longPressEvent, actorCoords ); + EmitLongPressSignal( Dali::Actor( currentGesturedActor ), mCurrentEmitters, longPressEvent, actorCoords ); } } diff --git a/dali/internal/event/events/pan-gesture-processor.cpp b/dali/internal/event/events/pan-gesture-processor.cpp index 1644f7b..0d76ea9 100644 --- a/dali/internal/event/events/pan-gesture-processor.cpp +++ b/dali/internal/event/events/pan-gesture-processor.cpp @@ -256,7 +256,7 @@ void PanGestureProcessor::Process( const Integration::PanGestureEvent& panEvent case Gesture::Started: { - if ( mCurrentGesturedActor ) + if ( GetCurrentGesturedActor() ) { // The pan gesture should only be sent to the gesture detector which first received it so that // it can be told when the gesture ends as well. @@ -264,7 +264,7 @@ void PanGestureProcessor::Process( const Integration::PanGestureEvent& panEvent HitTestAlgorithm::Results hitTestResults; HitTestAlgorithm::HitTest( mStage, mPossiblePanPosition, hitTestResults ); // Hit test original possible position... - if ( hitTestResults.actor && ( mCurrentGesturedActor == &GetImplementation( hitTestResults.actor ) ) ) + if ( hitTestResults.actor && ( GetCurrentGesturedActor() == &GetImplementation( hitTestResults.actor ) ) ) { // Record the current render-task for Screen->Actor coordinate conversions mCurrentRenderTask = hitTestResults.renderTask; @@ -290,27 +290,28 @@ void PanGestureProcessor::Process( const Integration::PanGestureEvent& panEvent // Only send subsequent pan gesture signals if we processed the pan gesture when it started. // Check if actor is still touchable. - if ( mCurrentGesturedActor ) + Actor* currentGesturedActor = GetCurrentGesturedActor(); + if ( currentGesturedActor ) { - if ( mCurrentGesturedActor->IsHittable() && !mCurrentPanEmitters.empty() && mCurrentRenderTask ) + if ( currentGesturedActor->IsHittable() && !mCurrentPanEmitters.empty() && mCurrentRenderTask ) { PanGestureDetectorContainer outsideTouchesRangeEmitters; // Removes emitters that no longer have the actor attached // Also remove emitters whose touches are outside the range of the current pan event and add them to outsideTouchesRangeEmitters PanGestureDetectorContainer::iterator endIter = std::remove_if( mCurrentPanEmitters.begin(), mCurrentPanEmitters.end(), - IsNotAttachedAndOutsideTouchesRangeFunctor(mCurrentGesturedActor, panEvent.numberOfTouches, outsideTouchesRangeEmitters) ); + IsNotAttachedAndOutsideTouchesRangeFunctor(currentGesturedActor, panEvent.numberOfTouches, outsideTouchesRangeEmitters) ); mCurrentPanEmitters.erase( endIter, mCurrentPanEmitters.end() ); Vector2 actorCoords; if ( !outsideTouchesRangeEmitters.empty() || !mCurrentPanEmitters.empty() ) { - mCurrentGesturedActor->ScreenToLocal( GetImplementation( mCurrentRenderTask ), actorCoords.x, actorCoords.y, panEvent.currentPosition.x, panEvent.currentPosition.y ); + currentGesturedActor->ScreenToLocal( GetImplementation( mCurrentRenderTask ), actorCoords.x, actorCoords.y, panEvent.currentPosition.x, panEvent.currentPosition.y ); // EmitPanSignal checks whether we have a valid actor and whether the container we are passing in has emitters before it emits the pan. - EmitPanSignal(Dali::Actor(mCurrentGesturedActor), outsideTouchesRangeEmitters, panEvent, actorCoords, Gesture::Finished, mCurrentRenderTask); - EmitPanSignal(Dali::Actor(mCurrentGesturedActor), mCurrentPanEmitters, panEvent, actorCoords, panEvent.state, mCurrentRenderTask); + EmitPanSignal(Dali::Actor(currentGesturedActor), outsideTouchesRangeEmitters, panEvent, actorCoords, Gesture::Finished, mCurrentRenderTask); + EmitPanSignal(Dali::Actor(currentGesturedActor), mCurrentPanEmitters, panEvent, actorCoords, panEvent.state, mCurrentRenderTask); } if ( mCurrentPanEmitters.empty() ) diff --git a/dali/internal/event/events/pinch-gesture-processor.cpp b/dali/internal/event/events/pinch-gesture-processor.cpp index 6dd9809..de3970e 100644 --- a/dali/internal/event/events/pinch-gesture-processor.cpp +++ b/dali/internal/event/events/pinch-gesture-processor.cpp @@ -184,21 +184,22 @@ void PinchGestureProcessor::Process( const Integration::PinchGestureEvent& pinch // Only send subsequent pinch gesture signals if we processed the pinch gesture when it started. // Check if actor is still touchable. - if ( mCurrentGesturedActor ) + Actor* currentGesturedActor = GetCurrentGesturedActor(); + if ( currentGesturedActor ) { - if ( mCurrentGesturedActor->IsHittable() && !mCurrentPinchEmitters.empty() && mCurrentRenderTask ) + if ( currentGesturedActor->IsHittable() && !mCurrentPinchEmitters.empty() && mCurrentRenderTask ) { // Ensure actor is still attached to the emitters, if it is not then remove the emitter. - PinchGestureDetectorContainer::iterator endIter = std::remove_if( mCurrentPinchEmitters.begin(), mCurrentPinchEmitters.end(), IsNotAttachedFunctor(mCurrentGesturedActor) ); + PinchGestureDetectorContainer::iterator endIter = std::remove_if( mCurrentPinchEmitters.begin(), mCurrentPinchEmitters.end(), IsNotAttachedFunctor(currentGesturedActor) ); mCurrentPinchEmitters.erase( endIter, mCurrentPinchEmitters.end() ); if ( !mCurrentPinchEmitters.empty() ) { Vector2 actorCoords; RenderTask& renderTaskImpl( GetImplementation(mCurrentRenderTask) ); - mCurrentGesturedActor->ScreenToLocal( renderTaskImpl, actorCoords.x, actorCoords.y, pinchEvent.centerPoint.x, pinchEvent.centerPoint.y ); + currentGesturedActor->ScreenToLocal( renderTaskImpl, actorCoords.x, actorCoords.y, pinchEvent.centerPoint.x, pinchEvent.centerPoint.y ); - EmitPinchSignal( Dali::Actor(mCurrentGesturedActor), mCurrentPinchEmitters, pinchEvent, actorCoords ); + EmitPinchSignal( Dali::Actor(currentGesturedActor), mCurrentPinchEmitters, pinchEvent, actorCoords ); } else { diff --git a/dali/internal/event/events/tap-gesture-processor.cpp b/dali/internal/event/events/tap-gesture-processor.cpp index 97f9e11..77b83be 100644 --- a/dali/internal/event/events/tap-gesture-processor.cpp +++ b/dali/internal/event/events/tap-gesture-processor.cpp @@ -138,12 +138,12 @@ void TapGestureProcessor::Process( const Integration::TapGestureEvent& tapEvent case Gesture::Started: { - if ( mCurrentGesturedActor ) + if ( GetCurrentGesturedActor() ) { HitTestAlgorithm::Results hitTestResults; HitTestAlgorithm::HitTest( mStage, tapEvent.point, hitTestResults ); - if ( hitTestResults.actor && ( mCurrentGesturedActor == &GetImplementation( hitTestResults.actor ) ) ) + if ( hitTestResults.actor && ( GetCurrentGesturedActor() == &GetImplementation( hitTestResults.actor ) ) ) { TapEventFunctor functor( tapEvent ); GestureDetectorContainer gestureDetectors; diff --git a/dali/internal/event/events/touch-event-processor.cpp b/dali/internal/event/events/touch-event-processor.cpp index eddc888..f54668e 100644 --- a/dali/internal/event/events/touch-event-processor.cpp +++ b/dali/internal/event/events/touch-event-processor.cpp @@ -450,7 +450,8 @@ void TouchEventProcessor::ProcessTouchEvent( const Integration::TouchEvent& even } TouchEventProcessor::ActorObserver::ActorObserver() -: mActor ( NULL ) +: mActor ( NULL ), + mActorDisconnected(false) { DALI_LOG_TRACE_METHOD( gLogFilter ); } @@ -463,7 +464,7 @@ TouchEventProcessor::ActorObserver::~ActorObserver() Actor* TouchEventProcessor::ActorObserver::GetActor() { - return mActor; + return mActorDisconnected ? NULL : mActor; } void TouchEventProcessor::ActorObserver::SetActor( Actor* actor ) @@ -472,10 +473,7 @@ void TouchEventProcessor::ActorObserver::SetActor( Actor* actor ) if ( mActor != actor ) { - if ( mActor ) - { - SceneObjectRemoved( *mActor ); - } + ResetActor(); mActor = actor; @@ -487,15 +485,25 @@ void TouchEventProcessor::ActorObserver::SetActor( Actor* actor ) } } +void TouchEventProcessor::ActorObserver::ResetActor() +{ + if ( mActor ) + { + DALI_LOG_INFO(gLogFilter, Debug::Verbose, "Stop Observing: %p\n", mActor); + mActor->RemoveObserver( *this ); + mActor = NULL; + mActorDisconnected = false; + } +} + void TouchEventProcessor::ActorObserver::SceneObjectRemoved( ProxyObject& proxy ) { DALI_LOG_TRACE_METHOD( gLogFilter ); if ( mActor == &proxy ) { - proxy.RemoveObserver( *this ); - DALI_LOG_INFO(gLogFilter, Debug::Verbose, "Stop Observing: %p\n", mActor); - mActor = NULL; + // do not call proxy.RemoveObserver here, proxy is currently iterating through observers... you wouldnt want to upset proxy now would you? + mActorDisconnected = true; } } diff --git a/dali/internal/event/events/touch-event-processor.h b/dali/internal/event/events/touch-event-processor.h index a6d9df1..2b73ce1 100644 --- a/dali/internal/event/events/touch-event-processor.h +++ b/dali/internal/event/events/touch-event-processor.h @@ -112,6 +112,11 @@ private: */ void SetActor( Actor* actor ); + /** + * Resets the set actor and disconnects any connected signals. + */ + void ResetActor(); + private: // Undefined @@ -143,7 +148,8 @@ private: virtual void ProxyDestroyed(ProxyObject& proxy); private: - Actor* mActor; ///< Raw pointer to an Actor. + Actor* mActor; ///< Raw pointer to an Actor. + bool mActorDisconnected; ///< Indicates whether the actor has been disconnected from the scene }; Stage& mStage; ///< Used to deliver touch events