Page MenuHomeWildfire Games

Fix crash in dynamic subscriptions when components unsubscribe during deletion.
ClosedPublic

Authored by wraitii on Mar 26 2017, 5:22 PM.

Details

Reviewers
fatherbushido
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP19424: Fix crash in dynamic subscriptions when components unsubscribe during deletion.
Trac Tickets
#4480
Summary

Per #4480, we discovered a rare bug with packing and global auras (the exact mechanism that leads to this crash is a little awkward to understand, however).

After analysis by fatherbushido and I, and some discussion with Philip, I've concluded that the following is the best way to fix the bug.

Test Plan

To repeat the bug in #4480, load the Macedonian test map (Macedonians have a hero with a global aura on siege) and unpack one of the siege. It should segfault at the end.

This stops that.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 894
Build 1401: Vulcan BuildJenkins
Build 1400: arc lint + arc unit

Event Timeline

wraitii created this revision.Mar 26 2017, 5:22 PM
Vulcan added a subscriber: Vulcan.Mar 26 2017, 6:06 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/601/ for more details.

A test for this would be nice.

source/simulation2/system/ComponentManager.cpp
921

s/re-//

Possibly kill the entire second line of the comment. Then add something about ensuring no dangling references to the first.

wraitii added inline comments.Mar 28 2017, 8:47 AM
source/simulation2/system/ComponentManager.cpp
921

I don't get what you refer to as "the first" here. The first line?

leper added inline comments.Mar 28 2017, 11:40 AM
source/simulation2/system/ComponentManager.cpp
921

There are only two possibilities, and one of them is already denoted as the second.

fatherbushido added inline comments.Mar 28 2017, 2:11 PM
source/simulation2/system/ComponentManager.cpp
921

(I guess wraitii kept the comment of the previous patch where he keeps also the flattening before the loop)

wraitii updated this revision to Diff 1071.Apr 2 2017, 9:24 AM

Better?

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/679/ for more details.

fatherbushido edited edge metadata.EditedApr 3 2017, 10:06 AM

A test for this would be nice.

I looked at adding that to the ComponentManager tests. But I am a bit slow as I need to learn things.
What is better, to make a test resulting in a crash or a test checking that m_Removed is actually empty with the patch (and not without)?
I was stuck at mocking a component unsubscribing on Destroy message. (For js, I would just have redefined the PostMessage function).
(edit: mocking is not really the good word here. Perhaps we must add a component like Test3A or extend another one).
If someone faster can do it :-) Else I will continue to look at it, at least to learn (yes, learning is not in weekly reports).

fatherbushido added a comment.EditedApr 11 2017, 9:59 PM

Well, I succeed to write a test to reproduce it. I'll clean it a bit and upload it.

Test in P35.
I can't upload it in a separate diff as tests would fail!

wraitii updated this revision to Diff 1217.Apr 12 2017, 11:09 AM

Re-upload with fatherbushido's tests included.

I can confirm that the tests fail in the expected manner without the fix, so I think this is now good to commit.

elexis added a subscriber: elexis.Apr 12 2017, 11:34 AM

Test in P35.
I can't upload it in a separate diff as tests would fail!

From http://trac.wildfiregames.com/ticket/4316#comment:12

(The test can actually be committed before the fix, or in case someone wants to test this.)

rP18941 rP18942

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/750/ for more details.

fatherbushido added a comment.EditedApr 12 2017, 11:55 AM
In D264#12498, @elexis wrote:

From http://trac.wildfiregames.com/ticket/4316#comment:12

(The test can actually be committed before the fix, or in case someone wants to test this.)

rP18941 rP18942

That's how I would like to do in case you didn't understand. See my forum post too.

That's how I would like to do in case you didn't understand. See my forum post too.

Looked through 100 forum posts but I couldn't find a related one. Link?
Why not commit a failing test to prove that the game is already broken, ignore Vulcan and commit the fix right afterwards? I recall you wanted to fix some other sim components, still doesn't mean we can't have a differnetial revision proposal for the tests.

In D264#12510, @elexis wrote:

Looked through 100 forum posts but I couldn't find a related one. Link?

https://wildfiregames.com/forum/index.php?/topic/21650-bug-catching-test-how-to-commit/

Why not commit a failing test to prove that the game is already broken, ignore Vulcan and commit the fix right afterwards? I recall you wanted to fix some other sim components, still doesn't mean we can't have a differnetial revision proposal for the tests.

I don't understand why you try to explain that to someone convinced...

I'm personally OK with committing the tests separately or as part of the same revision, not sure why there's a debate going.

Ok I recall + see. Thanks. Well. @coin.

Since here the tests were done by a different author than the fix, it would be better to have it in two commits, to give proper credit IMO
The important part is committing the fix right after the broken test, so as not to leave SVN in a state where the tests fail for some weeks, in case this will be seen as another precedent to refer to.

Sure, let's just do it when fatherbushido and I are both on IRC so we can synchronise then.

fatherbushido accepted this revision.Apr 17 2017, 9:40 AM

I splitted the test in D346

This revision is now accepted and ready to land.Apr 17 2017, 9:40 AM
This revision was automatically updated to reflect the committed changes.