[icedtea-web] RFC: Handle null NPP instances in plugin calls

[Omair Majid]

On 03/02/2011 04:45 PM, Deepak Bhole wrote:
> This patch fixes problems that may arise if a call is requested by an
> already destroyed applet (race condition).
>
> Additionally, it also does some very minor cleanup by removing an unused
> thread count variable.
>
> ChangeLog:
>
> 2011-03-02  Deepak Bhole<dbhole at redhat.com>
>
>      * plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
>      (PluginRequestProcessor): Remove initialization of tc_mutex
>      (~PluginRequestProcessor): Remove destruction of tc_mutex
>      (eval): Proceed with _eval only if instance is valid.
>      (call): Proceed with _call only if instance is valid. Moved initialization
>      of result_variant_jniid and result_variant to the top.
>      (sendString): Proceed with _getString only if instance is valid. Remove
>      thread count incrementer.
>      (setMember): Proceed with _setMember only if instance is valid. Remove
>      thread count incrementer.
>      (sendMember): Proceed with _getMember only if instance is valid. Remove
>      thread count incrementer.
>      * plugin/icedteanp/IcedTeaPluginRequestProcessor.h: Removed tc_mutex and
>      thread_count variables.
>
> 1.0 is affected too, and I think the patch should be applied there too.
>

Overall, the patch looks fine. Additional comments inline below.

>
> plugin-instance-check-cleanup.patch
>
>
> diff -r 5dbf0f9de599 plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
> --- a/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc	Wed Mar 02 11:50:30 2011 -0500
> +++ b/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc	Wed Mar 02 16:12:14 2011 -0500
> @@ -66,7 +66,6 @@
>
>       pthread_mutex_init(&message_queue_mutex, NULL);
>       pthread_mutex_init(&syn_write_mutex, NULL);
> -    pthread_mutex_init(&tc_mutex, NULL);
>
>       pthread_cond_init(&cond_message_available, NULL);
>   }
> @@ -86,8 +85,6 @@
>
>       pthread_mutex_destroy(&message_queue_mutex);
>       pthread_mutex_destroy(&syn_write_mutex);
> -    pthread_mutex_destroy(&tc_mutex);
> -
>       pthread_cond_destroy(&cond_message_available);
>   }
>
> @@ -236,6 +233,10 @@
>       thread_data.parameters.push_back(NPVARIANT_TO_OBJECT(*window_ptr));
>       thread_data.parameters.push_back(&script);
>
> +    // If instance no longer exists, bail
> +    if (!instance)
> +        return;
> +

Any reason this isn't being done as soon as instance is computed?

>   #ifdef CHROMIUM_WORKAROUND
>       // Workaround for chromium
>       _eval(&thread_data);
> @@ -282,6 +283,8 @@
>       std::string response = std::string();
>       JavaRequestProcessor java_request = JavaRequestProcessor();
>       JavaResultData* java_result;
> +    std::string result_variant_jniid = std::string();
> +    NPVariant* result_variant;
>

My c++ is quite rusty these days, but "std::string result_variant_jniid" 
is sufficient; you dont need to do "= std::string()"

>       reference = atoi(message_parts->at(3)->c_str());
>
> @@ -331,6 +334,10 @@
>       thread_data.parameters.push_back(&arg_count);
>       thread_data.parameters.push_back(args_array);
>
> +    // If instance no longer exists, bail
> +    if (!instance)
> +        goto cleanup;
> +
>   #ifdef CHROMIUM_WORKAROUND
>       // Workaround for chromium
>       _call(&thread_data);
> @@ -346,8 +353,7 @@
>       }
>   #endif
>
> -    NPVariant* result_variant = (NPVariant*) IcedTeaPluginUtilities::stringToJSID(thread_data.result);
> -    std::string result_variant_jniid = std::string();
> +    result_variant = (NPVariant*) IcedTeaPluginUtilities::stringToJSID(thread_data.result);
>

This seems completely unrelated to the rest of this patch.

>       if (result_variant)
>       {
> @@ -395,6 +401,10 @@
>       thread_data.parameters.push_back(instance);
>       thread_data.parameters.push_back(variant);
>
> +    // If instance no longer exists, bail
> +    if (!instance)
> +        return;
> +
>   #ifdef CHROMIUM_WORKAROUND
>       // Workaround for chromium
>       _getString(&thread_data);
> @@ -415,12 +425,6 @@
>       response += thread_data.result;
>
>       plugin_to_java_bus->post(response.c_str());
> -
> -    cleanup:
> -
> -    pthread_mutex_lock(&tc_mutex);
> -    thread_count--;
> -    pthread_mutex_unlock(&tc_mutex);
>   }
>
>   /**
> @@ -490,6 +494,10 @@
>       thread_data.parameters.push_back(&property_identifier);
>       thread_data.parameters.push_back(&value);
>
> +    // If instance no longer exists, bail
> +    if (!instance)
> +        return;
> +

Any reason not to add this after the line initializing instance?

>   #ifdef CHROMIUM_WORKAROUND
>       // Workaround for chromium
>       _setMember(&thread_data);
> @@ -508,13 +516,6 @@
>       IcedTeaPluginUtilities::constructMessagePrefix(0, reference,&response);
>       response.append(" JavaScriptSetMember ");
>       plugin_to_java_bus->post(response.c_str());
> -
> -    cleanup:
> -
> -    // property_name, type and value are deleted by _setMember
> -    pthread_mutex_lock(&tc_mutex);
> -    thread_count--;
> -    pthread_mutex_unlock(&tc_mutex);
>   }
>
>   /**
> @@ -588,6 +589,10 @@
>       thread_data.parameters.push_back(NPVARIANT_TO_OBJECT(*parent_ptr));
>       thread_data.parameters.push_back(&member_identifier);
>
> +    // If instance no longer exists, bail
> +    if (!instance)
> +        return;
> +
>   #ifdef CHROMIUM_WORKAROUND
>       // Workaround for chromium
>       _getMember(&thread_data);
> @@ -660,14 +665,6 @@
>       }
>       response.append(java_result->return_string->c_str());
>       plugin_to_java_bus->post(response.c_str());
> -
> -
> -    // Now be a good citizen and help keep the heap free of garbage
> -    cleanup:
> -
> -    pthread_mutex_lock(&tc_mutex);
> -    thread_count--;
> -    pthread_mutex_unlock(&tc_mutex);
>   }
>
>   /**
> diff -r 5dbf0f9de599 plugin/icedteanp/IcedTeaPluginRequestProcessor.h
> --- a/plugin/icedteanp/IcedTeaPluginRequestProcessor.h	Wed Mar 02 11:50:30 2011 -0500
> +++ b/plugin/icedteanp/IcedTeaPluginRequestProcessor.h	Wed Mar 02 16:12:14 2011 -0500
> @@ -84,9 +84,6 @@
>   void _eval(void* data);
>   void _getString(void* data);
>
> -static pthread_mutex_t tc_mutex = PTHREAD_MUTEX_INITIALIZER;
> -static int thread_count = 0;
> -
>   void* queue_processor(void* data);
>
>   /* Mutex to ensure that the request queue is accessed synchronously */

Cheers,
Omair