A few memory leaks were left unaddressed in rP19703. This patch either fixes them or removes the unused code they were a part of.
Details
Details
Check that one can host a game through the lobby with STUN
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 8433 Build 13777: Vulcan Build Jenkins
Event Timeline
Comment Actions
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/118/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues:
Executing section Source...
source/lobby/glooxwrapper/glooxwrapper.h
| 1| /*·Copyright·(C)·2018·Wildfire·Games.
| | [NORMAL] LicenseYearBear:
| | License should have "2019" year instead of "2018"
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"
source/lobby/glooxwrapper/glooxwrapper.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/121/display/redirect
Comment Actions
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.
Executing section JS...
Executing section cli...Link to build: https://jenkins.wildfiregames.com/job/docker-differential/122/display/redirect
Comment Actions
Correct the handling of sessionInitiate based off discussions with elexis on IRC yesterday.
Comment Actions
Correctness:
Accepting this since we agreed on IRC to have every author commit the things they came up with, and since every change herein is correct.
Completeness:
There are some more leaks in unused code, that can be identified by crunching the entire file multiple times, see inline comments in rP19703#35141.
The optimistic interpretation is that this patch fixes all(?) leaks in the binary, and the pessimistic interpretation is that the patch missed to find a bunch of memory leaks and that the classes they're in are unused.
Sorry for not having comprehended the glooxwrapper architecture properly. I have gained (1) the necessary experience with memory leaks (as Vladislav ensured that I don't mess these up in some reviews), (2) the necessary knowledge of the gloox codebase (https://bugs.camaya.net/ticket/?id=267 and https://bugs.camaya.net/ticket/?id=280) and (3) learnt from IRC logs why this file exists,
which enabled me to understand fcxSanyas glooxwrapper patch (that I committed here on the a22 feature freeze morning) more, than one I could one year ago (and that's about the time I was in GDPR land or on strike).
As I simultaneously worked on the fix in D2094 as I stopped working on the fixes to other complaints, it seems a bit complaint-driven development, but doesn't change the fact that this
was on my concern list and that I scheduled to go through these before committing new features (that's why we have the concern feature to begin with, it avoids trac tickets for issues with commits).
I accept the patch as it is correct and for the rest of the issues I could fathom in https://code.wildfiregames.com/rP19703#35141 D2094 can be rebased.
I'll add @fcxSanya as a reviewer as he authored the patched patch and might or might not want to contribute to its revision, also @echotangoecho as he pointed out some of the leaks.
| source/lobby/XmppClient.cpp | ||
|---|---|---|
| 165 | Check, this was the most obvious one, and the pointer cannot be avoided since the instantiation depends on other things instantiated in the XmppClient constructor (got the same in D2099?id=8985) | |
| source/lobby/glooxwrapper/glooxwrapper.cpp | ||
| 289 | Check, got the same too in D2099?id=8985 (except for passing the Session and Jingle by reference too) | |
| 816 | Check, it's unused by 0ad/pyrogenesis and I don't mind reducing the footpring of this file to reduce confusion rather than fixing unused code, got the same in D2099?id=8985. | |
| 827 | ||
| 848 | I got the same code but a comment for the ICEUDP plugin and a different comment for the Content in D2094.
to me the comment should be a bit more explicit and tell why the author thinks that gloox deletes that, since it's not in the documentation and since it's not trivial. // Content will delete the new ICEUDP plugin in the inherited ~Plugin()
gloox::Jingle::PluginList pluginList;
pluginList.push_back(new gloox::Jingle::ICEUDP(/*local_pwd*/"", /*local_ufrag*/"", candidateList));
// sessionInitiate deletes the new Content
return m_Wrapped->sessionInitiate(new gloox::Jingle::Content(std::string("game-data"), pluginList));The erasure of the ICEUDP plugin was not obvious to us at all and we needed multiple runs to figure it out, so it would be better commented on. But I'm okay with committing this as is, I can extend the comment in D2094, unless you prefer something else. (Also I had to lookup C++ specs again to ensure that the Plugin destructor is called, as Content has a custom empty one, but it's fine since all destructors are called without calling them explicitly) | |