[icedtea-web] RFC: Rewrite of plugin threading model
Deepak Bhole
dbhole at redhat.com
Tue Jun 7 13:35:54 PDT 2011
Hi,
I've been holding back 1.1 for this patch.
This patch adresses PR721:
http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=721
The patch in fact fixes far more than just that issue (which have
varying symptoms, all ultimately resulting in a browser crash).
The patch rewrites the threading model of the plug-in such that it
executes events on the correct thread. Furthermore, it does away with
all "chromium" workarounds. With this patch, the plugin works
significantly better with chromium. Furthermore, it no longer crashes
FF4.
Given the frequency of the newer FF4 crashes, I strongly think this
patch should go into 1.1. I have kept things to the absolute minimum and
tested it will all known applets and run the live connect tests. I found
no regressions vs the current 1.1. The only difference was that there
were no crashes, so it is much better with the patch.
Okay for 1.1 and HEAD?
ChangeLog:
2011-06-07 Deepak Bhole <dbhole at redhat.com>
PR721: IcedTeaPlugin.so cannot run g_main_context_iteration on a different
thread unless a different GMainContext *context is used
* plugin/icedteanp/IcedTeaJavaRequestProcessor.cc
(postAndWaitForResponse): Added logic for tracking when the processor is
running from a plugin main thread, and logic to process main thread
specific messages queued thereafter until function exit.
* plugin/icedteanp/IcedTeaNPPlugin.cc:
(itnp_plugin_thread_id): New variable. Tracks plugin main thread ID.
(pluginAsyncCallMutex): New variable. Mutex to lock async call queue.
(NP_Initialize): Initialize the itnp_plugin_thread_id variable and make
the make pluginAsyncCallMutex recursive.
* plugin/icedteanp/IcedTeaNPPlugin.h:
(CHROMIUM_WORKAROUND): Remove macro.
(itnp_plugin_thread_id): New variable. Tracks plugin main thread ID.
(pluginAsyncCallMutex): New variable. Mutex to lock async call queue.
* plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
(eval): Remove chromium workaround.
(call): Same.
(sendString): Same.
(setMember): Same.
(sendMember): Same.
(loadURL): Same.
* plugin/icedteanp/IcedTeaPluginRequestProcessor.h: Moved
async_call_thread_data to IcedTeaPluginUtils.h.
* plugin/icedteanp/IcedTeaPluginUtils.cc
(pendingPluginThreadRequests): New variable. Queue to track events waiting
for async execution on plug-in thread.
(callAndWaitForResult): New function. Calls a method on plug-in thread and
waits for the execution to complete.
(postPluginThreadAsyncCall): New function. Posts a method call to the
async execution queue and calls NPN_PluginThreadAsynCall.
(processAsyncCallQueue): New function. Called from the plug-in thread,
this function empties the event queue of functions waiting for plug-in
thread execution.
* plugin/icedteanp/IcedTeaPluginUtils.h
(plugin_thread_call): New struct to hold async call data.
(async_call_thread_data): Struct moved from IcedTeaPluginRequestProcessor.
(processAsyncCallQueue): New function.
(postPluginThreadAsyncCall): Same.
(callAndWaitForResult): Same.
* plugin/icedteanp/IcedTeaScriptablePluginObject.cc
(get_scriptable_java_object): Use
IcedTeaPluginUtilities::callAndWaitForResult to post async callback for
_createAndRetainJavaObject.
Cheers,
Deepak
-------------- next part --------------
diff -r 015b544f05d8 ChangeLog
--- a/ChangeLog Fri May 27 18:01:27 2011 -0400
+++ b/ChangeLog Tue Jun 07 16:34:43 2011 -0400
@@ -1,3 +1,49 @@
+2011-06-07 Deepak Bhole <dbhole at redhat.com>
+
+ PR721: IcedTeaPlugin.so cannot run g_main_context_iteration on a different
+ thread unless a different GMainContext *context is used
+ * plugin/icedteanp/IcedTeaJavaRequestProcessor.cc
+ (postAndWaitForResponse): Added logic for tracking when the processor is
+ running from a plugin main thread, and logic to process main thread
+ specific messages queued thereafter until function exit.
+ * plugin/icedteanp/IcedTeaNPPlugin.cc:
+ (itnp_plugin_thread_id): New variable. Tracks plugin main thread ID.
+ (pluginAsyncCallMutex): New variable. Mutex to lock async call queue.
+ (NP_Initialize): Initialize the itnp_plugin_thread_id variable and make
+ the make pluginAsyncCallMutex recursive.
+ * plugin/icedteanp/IcedTeaNPPlugin.h:
+ (CHROMIUM_WORKAROUND): Remove macro.
+ (itnp_plugin_thread_id): New variable. Tracks plugin main thread ID.
+ (pluginAsyncCallMutex): New variable. Mutex to lock async call queue.
+ * plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
+ (eval): Remove chromium workaround.
+ (call): Same.
+ (sendString): Same.
+ (setMember): Same. Return 0 if member was not found.
+ (sendMember): Remove chromium workaround.
+ * plugin/icedteanp/IcedTeaPluginRequestProcessor.h: Moved
+ async_call_thread_data to IcedTeaPluginUtils.h.
+ * plugin/icedteanp/IcedTeaPluginUtils.cc
+ (pendingPluginThreadRequests): New variable. Queue to track events waiting
+ for async execution on plug-in thread.
+ (callAndWaitForResult): New function. Calls a method on plug-in thread and
+ waits for the execution to complete.
+ (postPluginThreadAsyncCall): New function. Posts a method call to the
+ async execution queue and calls NPN_PluginThreadAsynCall.
+ (processAsyncCallQueue): New function. Called from the plug-in thread,
+ this function empties the event queue of functions waiting for plug-in
+ thread execution.
+ * plugin/icedteanp/IcedTeaPluginUtils.h
+ (plugin_thread_call): New struct to hold async call data.
+ (async_call_thread_data): Struct moved from IcedTeaPluginRequestProcessor.
+ (processAsyncCallQueue): New function.
+ (postPluginThreadAsyncCall): Same.
+ (callAndWaitForResult): Same.
+ * plugin/icedteanp/IcedTeaScriptablePluginObject.cc
+ (get_scriptable_java_object): Use
+ IcedTeaPluginUtilities::callAndWaitForResult to post async callback for
+ _createAndRetainJavaObject.
+
2011-05-27 Deepak Bhole <dbhole at redhat.com>
PR723: AccessControlException while downloading resource
diff -r 015b544f05d8 plugin/icedteanp/IcedTeaJavaRequestProcessor.cc
--- a/plugin/icedteanp/IcedTeaJavaRequestProcessor.cc Fri May 27 18:01:27 2011 -0400
+++ b/plugin/icedteanp/IcedTeaJavaRequestProcessor.cc Tue Jun 07 16:34:43 2011 -0400
@@ -247,22 +247,40 @@
plugin_to_java_bus->post(message.c_str());
// Wait for result to be filled in.
- struct timespec curr_t;
+ struct timespec curr_t;
+
+ bool isPluginThread = false;
+
+ if (pthread_self() == itnp_plugin_thread_id)
+ {
+ isPluginThread = true;
+ PLUGIN_DEBUG("JRP is in plug-in thread...\n");
+ }
do
{
- clock_gettime(CLOCK_REALTIME, &curr_t);
+ clock_gettime(CLOCK_REALTIME, &curr_t);
- if (!result_ready && (curr_t.tv_sec < t.tv_sec))
- {
- if (g_main_context_pending(NULL))
- g_main_context_iteration(NULL, false);
- else
- usleep(200);
- }
- else
- break;
+ if (!result_ready && (curr_t.tv_sec < t.tv_sec))
+ {
+ if (isPluginThread)
+ {
+ processAsyncCallQueue(NULL);
+ // Let the browser run its pending events too
+ if (g_main_context_pending(NULL))
+ {
+ g_main_context_iteration(NULL, false);
+ }
+ } else
+ {
+ usleep(1000); // 1ms
+ }
+ }
+ else
+ {
+ break;
+ }
} while (1);
if (curr_t.tv_sec >= t.tv_sec)
diff -r 015b544f05d8 plugin/icedteanp/IcedTeaNPPlugin.cc
--- a/plugin/icedteanp/IcedTeaNPPlugin.cc Fri May 27 18:01:27 2011 -0400
+++ b/plugin/icedteanp/IcedTeaNPPlugin.cc Tue Jun 07 16:34:43 2011 -0400
@@ -160,6 +160,12 @@
// Applet viewer output watch source.
gint out_watch_source;
+// Thread ID of plug-in thread
+pthread_t itnp_plugin_thread_id;
+
+// Mutex to lock async call queue
+pthread_mutex_t pluginAsyncCallMutex;
+
// Applet viewer output channel.
GIOChannel* out_to_appletviewer;
@@ -1159,6 +1165,7 @@
NPP instance = (NPP) g_hash_table_lookup(id_to_instance_map,
GINT_TO_POINTER(instance_id));
+ PLUGIN_DEBUG (" PIPE: plugin read (1): %s\n", message);
if (instance_id > 0 && !instance)
{
PLUGIN_DEBUG("Instance %d is not active. Refusing to consume message \"%s\"\n", instance_id, message);
@@ -2204,8 +2211,6 @@
PLUGIN_DEBUG ("NP_Initialize: using %s\n", appletviewer_executable);
- PLUGIN_DEBUG ("NP_Initialize return\n");
-
plugin_req_proc = new PluginRequestProcessor();
java_req_proc = new JavaMessageSender();
@@ -2219,6 +2224,16 @@
pthread_create (&plugin_request_processor_thread2, NULL, &queue_processor, (void*) plugin_req_proc);
pthread_create (&plugin_request_processor_thread3, NULL, &queue_processor, (void*) plugin_req_proc);
+ itnp_plugin_thread_id = pthread_self();
+
+ pthread_mutexattr_t attribute;
+ pthread_mutexattr_init(&attribute);
+ pthread_mutexattr_settype(&attribute, PTHREAD_MUTEX_RECURSIVE);
+ pthread_mutex_init(&pluginAsyncCallMutex, &attribute);
+ pthread_mutexattr_destroy(&attribute);
+
+ PLUGIN_DEBUG ("NP_Initialize return\n");
+
return NPERR_NO_ERROR;
cleanup_appletviewer_executable:
@@ -2363,6 +2378,9 @@
g_free (in_pipe_name);
in_pipe_name = NULL;
+ // Destroy the call queue mutex
+ pthread_mutex_destroy(&pluginAsyncCallMutex);
+
initialized = false;
pthread_cancel(plugin_request_processor_thread1);
diff -r 015b544f05d8 plugin/icedteanp/IcedTeaNPPlugin.h
--- a/plugin/icedteanp/IcedTeaNPPlugin.h Fri May 27 18:01:27 2011 -0400
+++ b/plugin/icedteanp/IcedTeaNPPlugin.h Tue Jun 07 16:34:43 2011 -0400
@@ -57,9 +57,6 @@
#include "IcedTeaPluginUtils.h"
#include "IcedTeaPluginRequestProcessor.h"
-// Work around across some chromium issues
-#define CHROMIUM_WORKAROUND
-
// ITNPPluginData stores all the data associated with a single plugin
// instance. A separate plugin instance is created for each <APPLET>
// tag. For now, each plugin instance spawns its own applet viewer
@@ -97,6 +94,12 @@
// Condition on which the queue processor waits
extern pthread_cond_t cond_message_available;
+// ID of plug-in thread
+extern pthread_t itnp_plugin_thread_id;
+
+/* Mutex around plugin async call queue ops */
+extern pthread_mutex_t pluginAsyncCallMutex;
+
// debug switch
extern int plugin_debug;
diff -r 015b544f05d8 plugin/icedteanp/IcedTeaPluginRequestProcessor.cc
--- a/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc Fri May 27 18:01:27 2011 -0400
+++ b/plugin/icedteanp/IcedTeaPluginRequestProcessor.cc Tue Jun 07 16:34:43 2011 -0400
@@ -239,20 +239,7 @@
thread_data.parameters.push_back(NPVARIANT_TO_OBJECT(*window_ptr));
thread_data.parameters.push_back(&script);
-#ifdef CHROMIUM_WORKAROUND
- // Workaround for chromium
- _eval(&thread_data);
-
- if (!thread_data.call_successful)
- {
-#endif
- thread_data.result_ready = false;
- browser_functions.pluginthreadasynccall(instance, &_eval, &thread_data);
-
- while (!thread_data.result_ready) usleep(2000); // Wait till result is ready
-#ifdef CHROMIUM_WORKAROUND
- }
-#endif
+ IcedTeaPluginUtilities::callAndWaitForResult(instance, &_eval, &thread_data);
NPVariant* result_variant = (NPVariant*) IcedTeaPluginUtilities::stringToJSID(thread_data.result);
std::string result_variant_jniid = std::string();
@@ -341,20 +328,7 @@
thread_data.parameters.push_back(&arg_count);
thread_data.parameters.push_back(args_array);
-#ifdef CHROMIUM_WORKAROUND
- // Workaround for chromium
- _call(&thread_data);
-
- if (!thread_data.call_successful)
- {
-#endif
- thread_data.result_ready = false;
- browser_functions.pluginthreadasynccall(instance, &_call, &thread_data);
-
- while (!thread_data.result_ready) usleep(2000); // wait till ready
-#ifdef CHROMIUM_WORKAROUND
- }
-#endif
+ IcedTeaPluginUtilities::callAndWaitForResult(instance, &_call, &thread_data);
result_variant = (NPVariant*) IcedTeaPluginUtilities::stringToJSID(thread_data.result);
@@ -409,19 +383,7 @@
thread_data.parameters.push_back(instance);
thread_data.parameters.push_back(variant);
-#ifdef CHROMIUM_WORKAROUND
- // Workaround for chromium
- _getString(&thread_data);
-
- if (!thread_data.call_successful)
- {
-#endif
- thread_data.result_ready = false;
- browser_functions.pluginthreadasynccall(instance, &_getString, &thread_data);
- while (!thread_data.result_ready) usleep(2000); // wait till ready
-#ifdef CHROMIUM_WORKAROUND
- }
-#endif
+ IcedTeaPluginUtilities::callAndWaitForResult(instance, &_getString, &thread_data);
// We need the context 0 for backwards compatibility with the Java side
IcedTeaPluginUtilities::constructMessagePrefix(0, reference, &response);
@@ -502,20 +464,7 @@
thread_data.parameters.push_back(&property_identifier);
thread_data.parameters.push_back(&value);
-#ifdef CHROMIUM_WORKAROUND
- // Workaround for chromium
- _setMember(&thread_data);
-
- if (!thread_data.call_successful)
- {
-#endif
- thread_data.result_ready = false;
- browser_functions.pluginthreadasynccall(instance, &_setMember, &thread_data);
-
- while (!thread_data.result_ready) usleep(2000); // wait till ready
-#ifdef CHROMIUM_WORKAROUND
- }
-#endif
+ IcedTeaPluginUtilities::callAndWaitForResult(instance, &_setMember, &thread_data);
IcedTeaPluginUtilities::constructMessagePrefix(0, reference, &response);
response.append(" JavaScriptSetMember ");
@@ -598,21 +547,7 @@
thread_data.parameters.push_back(NPVARIANT_TO_OBJECT(*parent_ptr));
thread_data.parameters.push_back(&member_identifier);
-#ifdef CHROMIUM_WORKAROUND
- // Workaround for chromium
- _getMember(&thread_data);
-
- if (!thread_data.call_successful)
- {
-#endif
- thread_data.result_ready = false;
- browser_functions.pluginthreadasynccall(instance, &_getMember, &thread_data);
-
- while (!thread_data.result_ready) usleep(2000); // wait till ready
-
-#ifdef CHROMIUM_WORKAROUND
- }
-#endif
+ IcedTeaPluginUtilities::callAndWaitForResult(instance, &_getMember, &thread_data);
PLUGIN_DEBUG("Member PTR after internal request: %s\n", thread_data.result.c_str());
@@ -743,8 +678,7 @@
thread_data.parameters.push_back(message_parts->at(6)); // push target
thread_data.result_ready = false;
- browser_functions.pluginthreadasynccall(instance, &_loadURL, &thread_data);
- while (!thread_data.result_ready) usleep(2000); // wait till ready
+ IcedTeaPluginUtilities::callAndWaitForResult(instance, &_loadURL, &thread_data);
}
static void
diff -r 015b544f05d8 plugin/icedteanp/IcedTeaPluginRequestProcessor.h
--- a/plugin/icedteanp/IcedTeaPluginRequestProcessor.h Fri May 27 18:01:27 2011 -0400
+++ b/plugin/icedteanp/IcedTeaPluginRequestProcessor.h Tue Jun 07 16:34:43 2011 -0400
@@ -57,18 +57,6 @@
#include "IcedTeaPluginUtils.h"
#include "IcedTeaJavaRequestProcessor.h"
-/**
- * Data structure passed to functions called in a new thread.
- */
-
-typedef struct async_call_thread_data
-{
- std::vector<void*> parameters;
- std::string result;
- bool result_ready;
- bool call_successful;
-} AsyncCallThreadData;
-
/* Internal request reference counter */
static long internal_req_ref_counter;
@@ -109,10 +97,10 @@
/* Dispatch request processing to a new thread for asynch. processing */
void dispatch(void* func_ptr (void*), std::vector<std::string>* message, std::string* src);
- /* Send main window pointer to Java */
- void sendWindow(std::vector<std::string*>* message_parts);
+ /* Send main window pointer to Java */
+ void sendWindow(std::vector<std::string*>* message_parts);
- /* Stores the variant on java side */
+ /* Stores the variant on java side */
void storeVariantInJava(NPVariant variant, std::string* result);
public:
diff -r 015b544f05d8 plugin/icedteanp/IcedTeaPluginUtils.cc
--- a/plugin/icedteanp/IcedTeaPluginUtils.cc Fri May 27 18:01:27 2011 -0400
+++ b/plugin/icedteanp/IcedTeaPluginUtils.cc Tue Jun 07 16:34:43 2011 -0400
@@ -54,6 +54,9 @@
std::map<void*, NPP>* IcedTeaPluginUtilities::instance_map = new std::map<void*, NPP>();
std::map<std::string, NPObject*>* IcedTeaPluginUtilities::object_map = new std::map<std::string, NPObject*>();
+/* Plugin async call queue */
+static std::vector< PluginThreadCall* >* pendingPluginThreadRequests = new std::vector< PluginThreadCall* >();
+
/**
* Given a context number, constructs a message prefix to send to Java
*
@@ -910,6 +913,110 @@
PLUGIN_DEBUG("SENDING URL: %s\n", *decoded_url);
}
+
+/**
+ * Posts a function for execution on the plug-in thread and wait for result.
+ *
+ * @param instance The NPP instance
+ * @param func The function to post
+ * @param data Arguments to *func
+ */
+void
+IcedTeaPluginUtilities::callAndWaitForResult(NPP instance, void (*func) (void *), AsyncCallThreadData* data)
+{
+
+ struct timespec t;
+ struct timespec post_t;
+ struct timespec curr_t;
+ clock_gettime(CLOCK_REALTIME, &t);
+ t.tv_sec += REQUESTTIMEOUT; // timeout
+
+ // post request
+ clock_gettime(CLOCK_REALTIME, &post_t);
+ postPluginThreadAsyncCall(instance, func, data);
+
+ do
+ {
+ clock_gettime(CLOCK_REALTIME, &curr_t);
+ if (data != NULL && !data->result_ready && (curr_t.tv_sec < t.tv_sec))
+ {
+ usleep(2000);
+ } else
+ {
+ break;
+ }
+ } while (1);
+}
+
+
+/**
+ * Posts a request that needs to be handled in a plugin thread.
+ *
+ * @param instance The plugin instance
+ * @param func The function to execute
+ * @param userData The userData for the function to consume/write to
+ * @return if the call was posted successfully
+ */
+
+bool
+IcedTeaPluginUtilities::postPluginThreadAsyncCall(NPP instance, void (*func) (void *), void* data)
+{
+ if (instance)
+ {
+ PluginThreadCall* call = new PluginThreadCall();
+ call->instance = instance;
+ call->func = func;
+ call->userData = data;
+
+ pthread_mutex_lock(&pluginAsyncCallMutex);
+ pendingPluginThreadRequests->push_back(call);
+ pthread_mutex_unlock(&pluginAsyncCallMutex);
+
+ browser_functions.pluginthreadasynccall(instance, &processAsyncCallQueue, NULL); // Always returns immediately
+
+ PLUGIN_DEBUG("Pushed back call evt %p\n", call);
+
+ return true;
+ }
+
+ // Else
+ PLUGIN_DEBUG("Instance is not active. Call rejected.\n");
+ return false;
+}
+
+/**
+ * Runs through the async call wait queue and executes all calls
+ *
+ * @param param Ignored -- required to conform to NPN_PluginThreadAsynCall API
+ */
+void
+processAsyncCallQueue(void* param /* ignored */)
+{
+ do {
+ PluginThreadCall* call = NULL;
+
+ pthread_mutex_lock(&pluginAsyncCallMutex);
+ if (pendingPluginThreadRequests->size() > 0)
+ {
+ call = pendingPluginThreadRequests->front();
+ pendingPluginThreadRequests->erase(pendingPluginThreadRequests->begin());
+ }
+ pthread_mutex_unlock(&pluginAsyncCallMutex);
+
+ if (call)
+ {
+ PLUGIN_DEBUG("Processing call evt %p\n", call);
+ call->func(call->userData);
+ PLUGIN_DEBUG("Call evt %p processed\n", call);
+
+ delete call;
+ } else
+ {
+ break;
+ }
+ } while(1);
+}
+
/******************************************
* Begin JavaMessageSender implementation *
******************************************
diff -r 015b544f05d8 plugin/icedteanp/IcedTeaPluginUtils.h
--- a/plugin/icedteanp/IcedTeaPluginUtils.h Fri May 27 18:01:27 2011 -0400
+++ b/plugin/icedteanp/IcedTeaPluginUtils.h Tue Jun 07 16:34:43 2011 -0400
@@ -119,12 +119,43 @@
} JavaResultData;
+/**
+ * This struct holds data to do calls that need to be run in the plugin thread
+ */
+typedef struct plugin_thread_call
+{
+ // The plugin instance
+ NPP instance;
+
+ // The function to call
+ void (*func) (void *);
+
+ // The data to pass to the function
+ void *userData;
+
+} PluginThreadCall;
+
+/**
+ * Data structure passed to functions called in a new thread.
+ */
+
+typedef struct async_call_thread_data
+{
+ std::vector<void*> parameters;
+ std::string result;
+ bool result_ready;
+ bool call_successful;
+} AsyncCallThreadData;
+
/*
* Misc. utility functions
*
* This class is never instantiated and should contain static functions only
*/
+/* Function to process all pending async calls */
+void processAsyncCallQueue(void*);
+
class IcedTeaPluginUtilities
{
@@ -140,6 +171,9 @@
/* Map holding java-side-obj-key->NPObject relationship */
static std::map<std::string, NPObject*>* object_map;
+ /* Posts a call in the async call queue */
+ static bool postPluginThreadAsyncCall(NPP instance, void (*func) (void *), void* data);
+
public:
/* Constructs message prefix with given context */
@@ -227,6 +261,9 @@
static bool isObjectJSArray(NPP instance, NPObject* object);
static void decodeURL(const char* url, char** decoded_url);
+
+ /* Posts call in async queue and waits till execution completes */
+ static void callAndWaitForResult(NPP instance, void (*func) (void *), AsyncCallThreadData* data);
};
/*
diff -r 015b544f05d8 plugin/icedteanp/IcedTeaScriptablePluginObject.cc
--- a/plugin/icedteanp/IcedTeaScriptablePluginObject.cc Fri May 27 18:01:27 2011 -0400
+++ b/plugin/icedteanp/IcedTeaScriptablePluginObject.cc Tue Jun 07 16:34:43 2011 -0400
@@ -411,9 +411,7 @@
thread_data.parameters.push_back(np_class);
thread_data.parameters.push_back(&scriptable_object);
- browser_functions.pluginthreadasynccall(instance, &_createAndRetainJavaObject, &thread_data);
-
- while (!thread_data.result_ready) usleep(2000); // wait till ready
+ IcedTeaPluginUtilities::callAndWaitForResult(instance, &_createAndRetainJavaObject, &thread_data);
} else
{
// Else retain object and continue
More information about the distro-pkg-dev
mailing list