After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 776170 - GWeatherLocation.timezone_cache never initialized
GWeatherLocation.timezone_cache never initialized
Status: RESOLVED FIXED
Product: libgweather
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: future
Assigned To: libgweather-maint
libgweather-maint
Depends on:
Blocks:
 
 
Reported: 2016-12-16 14:36 UTC by Will Thompson
Modified: 2016-12-22 15:53 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
location: initialize timezone_cache (1.01 KB, patch)
2016-12-16 14:38 UTC, Will Thompson 		committed Details | Review
parser: fix g_hash_table_unref(NULL) (1019 bytes, patch)
2016-12-16 14:38 UTC, Will Thompson 		none Details | Review
parser: don't leak timezone_cache (827 bytes, patch)
2016-12-16 14:38 UTC, Will Thompson 		none Details | Review
parser: fix g_hash_table_unref(NULL) (1003 bytes, patch)
2016-12-16 15:00 UTC, Will Thompson 		committed Details | Review
parser: don't leak timezone_cache (821 bytes, patch)
2016-12-16 15:00 UTC, Will Thompson 		committed Details | Review

Description Will Thompson 2016-12-16 14:36:39 UTC 
I'm pretty sure it's meant to be a reference to the timezone_cache in GWeatherParser, probably in location_new_from_xml(), but this never happens. As a result, any call to gweather_timezone_get_by_tzid() will inevitably cause a CRITICAL message and return NULL.
Comment 1 Will Thompson 2016-12-16 14:38:44 UTC 
Created attachment 342061 [details] [review]
location: initialize timezone_cache
Comment 2 Will Thompson 2016-12-16 14:38:49 UTC 
Created attachment 342062 [details] [review]
parser: fix g_hash_table_unref(NULL)

If _gweather_parser_new fails (because the Location.xml cannot be found,
say), then _gweather_parser_free() is called on a GWeatherParser that
has not been fully initialized.
Comment 3 Will Thompson 2016-12-16 14:38:54 UTC 
Created attachment 342063 [details] [review]
parser: don't leak timezone_cache
Comment 4 Bastien Nocera 2016-12-16 14:48:57 UTC 
Review of attachment 342062 [details] [review]:

::: libgweather/gweather-parser.c
@@ +201,3 @@
 	xmlFreeTextReader (parser->xml);
+    if (parser->metar_code_cache)
+	g_hash_table_unref (parser->metar_code_cache);

g_clear_pointer()?
Comment 5 Bastien Nocera 2016-12-16 14:49:17 UTC 
Review of attachment 342063 [details] [review]:

::: libgweather/gweather-parser.c
@@ +203,3 @@
 	g_hash_table_unref (parser->metar_code_cache);
+    if (parser->timezone_cache)
+	g_hash_table_unref (parser->timezone_cache);

Ditto.
Comment 6 Will Thompson 2016-12-16 15:00:08 UTC 
Created attachment 342065 [details] [review]
parser: fix g_hash_table_unref(NULL)

If _gweather_parser_new fails (because the Location.xml cannot be found,
say), then _gweather_parser_free() is called on a GWeatherParser that
has not been fully initialized.
Comment 7 Will Thompson 2016-12-16 15:00:14 UTC 
Created attachment 342066 [details] [review]
parser: don't leak timezone_cache
Comment 8 Giovanni Campagna 2016-12-22 13:29:07 UTC 
Review of attachment 342065 [details] [review]:

Looks good.
Comment 9 Giovanni Campagna 2016-12-22 13:30:33 UTC 
Review of attachment 342066 [details] [review]:

Yeah
Comment 10 Giovanni Campagna 2016-12-22 13:32:23 UTC 
Review of attachment 342061 [details] [review]:

Not sure at what point this feature was added, or if it ever worked, but it seems correct.
Comment 11 Giovanni Campagna 2016-12-22 15:53:19 UTC 
I ended up pushing these myself because I was working in the same spot
so I could deal with the rebase conflicts.

Thanks for the report and the patches!

Attachment 342061 [details] pushed as 07c8179 - location: initialize timezone_cache
Attachment 342066 [details] pushed as 5831e58 - parser: don't leak timezone_cache