csete / gpredict

Gpredict satellite tracking application
http://gpredict.oz9aec.net/
GNU General Public License v2.0
844 stars 247 forks source link

Stop map buffer leaking #224

Closed brookst closed 3 years ago

brookst commented 4 years ago

Taking a look at memory with valgrind, I found that the map pixel buffer was not deallocated. When configuring the module, the map gets re-created and another buffer leaks. I beefed up the destructor to clean up both the map and the objects associated with satellites. I had to add a reference to the sat_t in sat_map_obj_t as in #223 so that the ground track can be deleted.

A second (minor) issue that tripped me up while debugging is the timerid in GtkSatModule. GSource IDs are unsigned, but always greater than 0. See g_source_get_id. Setting it to -1 means it passes the >0 test in gtk_sat_module_destroy and the timer gets removed a second time.

brookst commented 4 years ago

Ok, it turns out taking a reference to a sat_t is a bad idea since they can mutate when updating TLEs, etc. Instead, take a catalogue number in the sat_map_obj_t and lookup in the satmap->sats hash table.

Incidentally, it seems like that hash table ought to use GINT_TO_POINTER to pack the catalogue numbers into the table, instead of allocating a gint for each one.

csete commented 3 years ago

Hi,

Sorry for the late reply. I just tried this but it doesn't build for me:

In file included from gtk-sat-data.h:29:0,
                 from gtk-sat-map.c:34:
gtk-sat-map.c: In function ‘free_sat_obj’:
gtk-sat-map.c:2019:59: error: ‘sat_map_obj_t {aka struct <anonymous>}’ has no member named ‘catnr’; did you mean ‘catnum’?
         sat = SAT(g_hash_table_lookup(satmap->sats, &obj->catnr));
                                                       ^
sgpsdp/sgp4sdp4.h:196:30: note: in definition of macro ‘SAT’
 #define SAT(sat)  ((sat_t *) sat)

Does it depend on PR #223?

brookst commented 3 years ago

Sorry about that, I think I messed up splitting some history from PR #223. I fixed that identifier and managed to catch another small leak. (I also double checked it still builds this time.) There's an outstanding leak I've tracked down to tooltip strings, but it seems to be within GooCanvas so I'd have to take it up there.

The commit still seems to merge cleanly onto master so I haven't rebased it but I can if you'd prefer that.

csete commented 3 years ago

Thanks for the update!

Github can rebase before merging, so no problem there.