/hg/icedtea-web: Fixed two memory leaks in C++ side of plugin

[Pavel Tisnovsky]

Hi Adam,

your fixes looks ok for me, thank you.

I just have two questions:

1) did you use valgrind or similar tool to found (possibly all) memory leaks? Or did you simply look at the code?

2) the second fix (msg/message): was there any reason to use a copy of original message string (more thread modifying
the message str. or something similar)?

Cheers,
Pavel

adomurad at icedtea.classpath.org wrote:
> changeset 24722d62f53c in /hg/icedtea-web
> details: http://icedtea.classpath.org/hg/icedtea-web?cmd=changeset;node=24722d62f53c
> author: Adam Domurad <adomurad at redhat.com>
> date: Fri Jun 15 15:27:12 2012 -0400
> 
> 	Fixed two memory leaks in C++ side of plugin
> 
> 
> diffstat:
> 
>  ChangeLog                              |   9 +++++++++
>  plugin/icedteanp/IcedTeaNPPlugin.cc    |   3 +++
>  plugin/icedteanp/IcedTeaPluginUtils.cc |  12 ++++--------
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diffs (69 lines):
> 
> diff -r 1819a9bbd922 -r 24722d62f53c ChangeLog
> --- a/ChangeLog	Thu Jun 14 11:11:01 2012 -0400
> +++ b/ChangeLog	Fri Jun 15 15:27:12 2012 -0400
> @@ -1,3 +1,12 @@
> +2012-06-15  Adam Domurad  <adomurad at redhat.com>
> +
> +	Fixed two memory leaks
> +	* plugin/icedteanp/IcedTeaNPPlugin.cc
> +	(consume_message): Call to g_strsplit matched with call to g_strfreev.
> +	* plugin/icedteanp/IcedTeaPluginUtils.cc
> +	(post): Removed copy of string, which assumed consumer freed string
> +	(which was not true and not always possible)
> +
>  2012-06-11  Danesh Dadachanji  <ddadacha at redhat.com>
>  
>  	PR855: AppletStub getDocumentBase() doesn't return full URL
> diff -r 1819a9bbd922 -r 24722d62f53c plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc	Thu Jun 14 11:11:01 2012 -0400
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc	Fri Jun 15 15:27:12 2012 -0400
> @@ -1303,11 +1303,14 @@
>          g_free(cookie_info);
>          cookie_info = NULL;
>        }
> +      g_strfreev (parts);
> +      parts = NULL;
>      }
>    else
>      {
>          g_print ("  Unable to handle message: %s\n", message);
>      }
> +
>  }
>  
>  void get_instance_from_id(int id, NPP& instance)
> diff -r 1819a9bbd922 -r 24722d62f53c plugin/icedteanp/IcedTeaPluginUtils.cc
> --- a/plugin/icedteanp/IcedTeaPluginUtils.cc	Thu Jun 14 11:11:01 2012 -0400
> +++ b/plugin/icedteanp/IcedTeaPluginUtils.cc	Fri Jun 15 15:27:12 2012 -0400
> @@ -1124,27 +1124,23 @@
>  void
>  MessageBus::post(const char* message)
>  {
> -	char* msg = (char*) malloc(sizeof(char)*strlen(message) + 1);
>  	bool message_consumed = false;
>  
> -	// consumer frees this memory
> -	strcpy(msg, message);
> -
>  	PLUGIN_DEBUG("Trying to lock %p...\n", &msg_queue_mutex);
>  	pthread_mutex_lock(&subscriber_mutex);
>  
> -    PLUGIN_DEBUG("Message %s received on bus. Notifying subscribers.\n", msg);
> +    PLUGIN_DEBUG("Message %s received on bus. Notifying subscribers.\n", message);
>  
>      std::list<BusSubscriber*>::const_iterator i;
>      for( i = subscribers.begin(); i != subscribers.end() && !message_consumed; ++i ) {
> -    	PLUGIN_DEBUG("Notifying subscriber %p of %s\n", *i, msg);
> -    	message_consumed = ((BusSubscriber*) *i)->newMessageOnBus(msg);
> +    	PLUGIN_DEBUG("Notifying subscriber %p of %s\n", *i, message);
> +    	message_consumed = ((BusSubscriber*) *i)->newMessageOnBus(message);
>      }
>  
>      pthread_mutex_unlock(&subscriber_mutex);
>  
>      if (!message_consumed)
> -    	PLUGIN_DEBUG("Warning: No consumer found for message %s\n", msg);
> +    	PLUGIN_DEBUG("Warning: No consumer found for message %s\n", message);
>  
>      PLUGIN_DEBUG("%p unlocked...\n", &msg_queue_mutex);
>  }