Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

use UnitPtr (intrusive_ptr) in unit_animations

In this commit the unit * in unit_animation is changed to a smart
pointer. This is to prevent the unit_map or some other object
from destroying the unit and creating a dangling pointer
(reported to cause segfaults).

In this commit, we also change the fake_unit class to make sure to
increment the refcounter when the fake_unit is constructed. The
reason is that the fake_unit holder is supposed to own that unit
and let it expire when it goes out of scope. No smart pointer is
supposed to delete it.

This mechanism should possibly be revisited, for example the
fake units could also be held in a reference counted pointer.
With the current system it is still possible that the fake_unit
will go out of scope and be destoryed while the animation is still
alive, this change only prevents smart pointers from destroying it
before it goes out of scope.

I also comment out the reentry_preventer class uses in
unit_animation.cpp, to prevent assertions from happening when I
test for example with the "double debug kill" version of the bug.
The reentry_preventer seems to be unnecessary now.

This addresses bug #18921 and maybe others.
  • Loading branch information...
commit 823d8c2d38c021cda0970fcea60c20b15c4b073d 1 parent 3284867
@cbeck88 cbeck88 authored
View
6 src/fake_unit.hpp
@@ -29,12 +29,12 @@ class fake_unit_manager;
*/
class fake_unit : public unit {
public:
- explicit fake_unit(unit const & u) : unit(u), my_manager_(NULL) {}
- fake_unit(fake_unit const & u) : unit(u), my_manager_(NULL) {}
+ explicit fake_unit(unit const & u) : unit(u), my_manager_(NULL) {ref_count_ = ref_count_ + 1; } //prevent UnitPtr from deleting this
+ fake_unit(fake_unit const & u) : unit(u), my_manager_(NULL) {ref_count_ = ref_count_ + 1; }
fake_unit(const unit_type& t, int side, unit_race::GENDER gender = unit_race::NUM_GENDERS)
: unit(t, side, false, gender)
, my_manager_(NULL)
- {}
+ {ref_count_ = ref_count_ + 1; }
/// Assignment operator, taking a fake_unit.
/// If already in the queue, @a this will be moved to the end of the
/// queue (drawn last). The queue (if any) of the parameter is ignored.
View
3  src/unit.hpp
@@ -362,9 +362,10 @@ class unit
long ref_count() const { return ref_count_; }
friend void intrusive_ptr_add_ref(const unit *);
friend void intrusive_ptr_release(const unit *);
-private:
+protected:
mutable long ref_count_; //used by intrusive_ptr
+private:
void advance_to(const config &old_cfg, const unit_type &t,
bool use_traits);
View
14 src/unit_animation.cpp
@@ -1279,7 +1279,7 @@ void unit_animator::add_animation(const unit* animated_unit
if(!animated_unit) return;
anim_elem tmp;
display*disp = display::get_singleton();
- tmp.my_unit = animated_unit;
+ tmp.my_unit = UnitConstPtr(animated_unit);
tmp.text = text;
tmp.text_color = text_color;
tmp.src = src;
@@ -1300,7 +1300,7 @@ void unit_animator::add_animation(const unit* animated_unit
{
if(!animated_unit) return;
anim_elem tmp;
- tmp.my_unit = animated_unit;
+ tmp.my_unit = UnitConstPtr(animated_unit);
tmp.text = text;
tmp.text_color = text_color;
tmp.src = src;
@@ -1331,7 +1331,7 @@ void unit_animator::replace_anim_if_invalid(const unit* animated_unit
!animated_unit->anim_comp().get_animation()->animation_finished_potential() &&
animated_unit->anim_comp().get_animation()->matches(*disp,src,dst,animated_unit,event,value,hit_type,attack,second_attack,value2) >unit_animation::MATCH_FAIL) {
anim_elem tmp;
- tmp.my_unit = animated_unit;
+ tmp.my_unit = UnitConstPtr(animated_unit);
tmp.text = text;
tmp.text_color = text_color;
tmp.src = src;
@@ -1429,9 +1429,9 @@ class reentry_preventer {
void unit_animator::wait_for_end() const
{
if (game_config::no_delay) return;
- static reentry_preventer rp;
- reentry_preventer::entry rpe = rp.enter();
- assert(rpe || (false && "Reentered a unit animation. See bug #18921")); //Catches reentry
+ //static reentry_preventer rp;
+ //reentry_preventer::entry rpe = rp.enter();
+ //assert(rpe || (false && "Reentered a unit animation. See bug #18921")); //Catches reentry
bool finished = false;
display*disp = display::get_singleton();
while(!finished) {
@@ -1439,7 +1439,7 @@ void unit_animator::wait_for_end() const
// Replacing the below assert with a conditional break will fix the local segfault,
// but this just exposes a different one.
// It's also unnecessary given the one a few lines up.
- assert(rpe || (false && "Reentered a unit animation. See bug #18921")); //Catches a past reentry
+ //assert(rpe || (false && "Reentered a unit animation. See bug #18921")); //Catches a past reentry
disp->delay(10);
finished = true;
for(std::vector<anim_elem>::const_iterator anim = animated_units_.begin(); anim != animated_units_.end();++anim) {
View
3  src/unit_animation.hpp
@@ -17,6 +17,7 @@
#include "animated.hpp"
#include "config.hpp"
#include "unit_frame.hpp"
+#include "unit_ptr.hpp"
class attack_type;
class display;
@@ -213,7 +214,7 @@ class unit_animator
with_bars(false)
{}
- const unit *my_unit;
+ UnitConstPtr my_unit;
const unit_animation * animation;
std::string text;
Uint32 text_color;

0 comments on commit 823d8c2

Please sign in to comment.
Something went wrong with that request. Please try again.