Page MenuHomeWildfire Games

Fix unreported memory leaks, remove unused code and use c++ style casts in the glooxwrapper following rP19703
ClosedPublic

Authored by elexis on Jul 17 2019, 9:50 PM.

Details

Summary
Test Plan

Make sure that every pointer change is actually correct, make sure that every leak is identified and addressed (not only the reported ones).

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Jul 17 2019, 9:50 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lobby/glooxwrapper/glooxwrapper.h
|  88| namespace·glooxwrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language.

source/lobby/XmppClient.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/120/display/redirect

elexis planned changes to this revision.Jul 17 2019, 10:02 PM

(not done yet)

elexis edited the summary of this revision. (Show Details)Jul 17 2019, 10:11 PM
elexis retitled this revision from Fix some memory leaks in the glooxwrapper to Fix unreported memory leaks in the glooxwrapper following rP20582.Jul 18 2019, 1:38 PM
elexis retitled this revision from Fix unreported memory leaks in the glooxwrapper following rP20582 to Fix unreported memory leaks in the glooxwrapper following rP19703.Jul 18 2019, 2:25 PM
elexis updated this revision to Diff 8990.Jul 19 2019, 12:25 PM

See https://code.wildfiregames.com/rP19703#35141 for a comment on every line.
Fix identified and previously unidentified leaks.
Use references in many places.
Supersede c-style casts with static_cast and reinterpret_cast.
Remove unused classes.
Remove default arguments from implementation and especially headers.

elexis edited the summary of this revision. (Show Details)Jul 19 2019, 12:26 PM
elexis edited the test plan for this revision. (Show Details)

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/network/NetClient.h
|  59| class·CNetClient·:·public·CFsm
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCNetClient:' is invalid C code. Use --std or --language to configure the language.

source/lobby/XmppClient.h
|  24| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/network/StunClient.h
|  26| namespace·StunClient
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/lobby/glooxwrapper/glooxwrapper.h
|  88| namespace·glooxwrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language.

source/lobby/IXmppClient.h
|  24| namespace·StunClient·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/network/tests/test_Net.h
|  37| class·TestNetComms·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 210| 210| 				error("Unrecognised net message type: " + message.type);
| 211| 211| 			}
| 212| 212| 		else
| 213|    |-			// Not rejoining - just trying to connect to server
|    | 213|+		// Not rejoining - just trying to connect to server
| 214| 214| 
| 215| 215| 			switch (message.type)
| 216| 216| 			{
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/132/display/redirect

Angen added a subscriber: Angen.Jul 19 2019, 2:17 PM

builds windows vs2015 release and debug

elexis added inline comments.Jul 19 2019, 2:35 PM
source/lobby/XmppClient.cpp
1219 ↗(On Diff #8990)

(handleSessionInitiation sounds a lot like it has legitimate access to session, hence the UNUSED moved from handleSessionAction)

source/lobby/glooxwrapper/glooxwrapper.cpp
813 ↗(On Diff #8990)

Leak / Unused code: If the unused class wasn't removed (I did that too in D2094), it would probably be cleaner to return references, or tell the caller to delete the plugins.

850 ↗(On Diff #8990)
856 ↗(On Diff #8990)

Unused code / Leak: There would have been a destructor nuking m_Wrapped missing here.

880 ↗(On Diff #8990)

inline glooxSession, otherwise double-free is redundant (can be done when committing)

source/lobby/glooxwrapper/glooxwrapper.h
642 ↗(On Diff #8990)

This was the unused variable from D2090 fixed by the ~Session conditionally deleting m_Wrapped.
The leak didn't occur because it's only use is with m_Owned = false (and rightly so as gloox creates the message, calls this XmppClient implemented handleSessionAction with this session and deletes it afterwards).

elexis retitled this revision from Fix unreported memory leaks in the glooxwrapper following rP19703 to Fix unreported memory leaks, remove unused code and use c++ style casts in the glooxwrapper following rP19703.Jul 19 2019, 11:47 PM
elexis updated this revision to Diff 9003.Jul 19 2019, 11:49 PM
elexis edited the test plan for this revision. (Show Details)

Drop the juicy memory leak fixes committed in rP22514.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/network/StunClient.h
|  26| namespace·StunClient
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/lobby/IXmppClient.h
|  24| namespace·StunClient·{
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.

source/network/tests/test_Net.h
|  37| class·TestNetComms·:·public·CxxTest::TestSuite
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classTestNetComms:' is invalid C code. Use --std or --language to configure the language.

source/lobby/glooxwrapper/glooxwrapper.h
|  88| namespace·glooxwrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceglooxwrapper{' is invalid C code. Use --std or --language to configure the language.

source/network/NetClient.h
|  59| class·CNetClient·:·public·CFsm
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCNetClient:' is invalid C code. Use --std or --language to configure the language.

source/lobby/XmppClient.h
|  24| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceStunClient{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 210| 210| 				error("Unrecognised net message type: " + message.type);
| 211| 211| 			}
| 212| 212| 		else
| 213|    |-			// Not rejoining - just trying to connect to server
|    | 213|+		// Not rejoining - just trying to connect to server
| 214| 214| 
| 215| 215| 			switch (message.type)
| 216| 216| 			{
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/139/display/redirect

elexis added inline comments.Jul 20 2019, 3:29 AM
source/lobby/glooxwrapper/glooxwrapper.cpp
295 ↗(On Diff #9003)

(nitpick rename because m_Wrapped relates to the item wrapped while these variables refer to the wrapper, also "dromedary case" / lower-camel-case)

JoshuaJB accepted this revision.Jul 20 2019, 6:27 AM

I'm not entirely sure that all these changes are needed, but they seem okay. Coming from mostly writing C, it strikes me as particularly strange to modify something passed as a C++ reference. However, it doesn't seem that there's anything technically wrong with that. I assume you've tested that hosting a game and STUN still work?

source/lobby/XmppClient.cpp
1200 ↗(On Diff #9003)

const stunEndpoint?

This revision is now accepted and ready to land.Jul 20 2019, 6:27 AM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Aug 17 2019, 2:12 AM

Thank you for the review @JoshuaJB !

Also thanks to @fcxSanya (original author) who didn't have time for a technical review but still responded with a general encouragement / appreciation answer.

I'm not entirely sure that all these changes are needed, but they seem okay.

About the changes being needed:

  • There is a clang compiler warning about a glooxwrapper thing being unused is an actually occurring leak.
  • The two unused classes miss a destructor, deleting them altogether makes the code easier to maintain.
  • Using references means the caller doesnt have to worry anymore about whether or not the thing has to be deleted.

Coming from mostly writing C, it strikes me as particularly strange to modify something passed as a C++ reference. However, it doesn't seem that there's anything technically wrong with that.

I talked to @vladislavbelov on IRC about pointer vs reference briefly on http://irclogs.wildfiregames.com/2019-08/2019-08-14-QuakeNet-%230ad-dev.log

21:53 < Vladislav> It depends, I and some known CC recommends to use only const ref for read and pointers for read/write. Because it might be easier to understand that a function may change an argument: foo(arg) or foo(&arg).

I assume you've tested that hosting a game and STUN still work?

Of course. Not only hosting, but also joining, otherwise it's a bit inconclusive :-p

I also ran valgrind on it and neither xmpp nor gloox appears in the log.

source/lobby/XmppClient.cpp
1200 ↗(On Diff #9003)

ack