[icedtea-web] RFC: Handle null NPP instances in plugin calls
Deepak Bhole
dbhole at redhat.com
Thu Mar 3 08:02:07 PST 2011
* Omair Majid <omajid at redhat.com> [2011-03-03 10:54]:
> 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()"
>
It is. I just added it to make it clear what was being initialized in
the function vs what was being initialized elsewhere.
> > 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.
>
Nope, it is not. goto cannot cross initialization, which is why it had
to be moved up.
> > 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?
>
Yep. Instance may become invalid at any point in time. It is best to
wait till last minute.
Thanks for reviewing! I will repost it later today (same content, just
broken down as Andrew suggested).
Cheers,
Deepak
> > #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
More information about the distro-pkg-dev
mailing list