/hg/icedtea-web: Do not wait for applet initialization when bind...
Andrew Azores
aazores at redhat.com
Fri Oct 4 08:27:07 PDT 2013
On 08/28/13 14:50, adomurad at icedtea.classpath.org wrote:
> changeset 420d72e5cee7 in /hg/icedtea-web
> details: http://icedtea.classpath.org/hg/icedtea-web?cmd=changeset;node=420d72e5cee7
> author: Adam Domurad <adomurad at redhat.com>
> date: Tue Aug 27 16:53:30 2013 -0400
>
> Do not wait for applet initialization when binding Java applets for NPAPI.
>
>
> diffstat:
>
> ChangeLog | 15 +
> plugin/icedteanp/IcedTeaNPPlugin.cc | 36 +--
> plugin/icedteanp/IcedTeaPluginUtils.cc | 134 +++++--
> plugin/icedteanp/IcedTeaPluginUtils.h | 67 +++-
> plugin/icedteanp/IcedTeaScriptablePluginObject.cc | 220 +++++++++----
> plugin/icedteanp/IcedTeaScriptablePluginObject.h | 28 +-
> tests/cpp-unit-tests/IcedTeaScriptablePluginObjectTest.cc | 15 +-
> 7 files changed, 356 insertions(+), 159 deletions(-)
>
> diffs (truncated from 851 to 500 lines):
>
> diff -r 94ebabfba6ab -r 420d72e5cee7 ChangeLog
> --- a/ChangeLog Fri Aug 23 16:35:37 2013 -0400
> +++ b/ChangeLog Tue Aug 27 16:53:30 2013 -0400
> @@ -1,3 +1,18 @@
> +2013-08-27 Adam Domurad <adomurad at redhat.com>
> +
> + Do not wait for applet initialization when binding Java applets for NPAPI.
> + * plugin/icedteanp/IcedTeaNPPlugin.cc: Refactor to use
> + lazy-initialized javascript applet binding.
> + * plugin/icedteanp/IcedTeaPluginUtils.cc: Make use of new helper
> + class, introduce (stringPrintf), introduce NPObjectRef.
> + * plugin/icedteanp/IcedTeaPluginUtils.h: Same.
> + * plugin/icedteanp/IcedTeaScriptablePluginObject.cc: Allow
> + IcedTeaScriptableJavaObject to be lazy-initialized, introduce
> + lazy-initializing (get_scriptable_applet_object).
> + * plugin/icedteanp/IcedTeaScriptablePluginObject.h: Same.
> + * tests/cpp-unit-tests/IcedTeaScriptablePluginObjectTest.cc: Adapt
> + test to new helper class.
> +
> 2013-08-23 Adam Domurad <adomurad at redhat.com>
>
> Spawn Java side during C++ unit tests. Many new tests.
> diff -r 94ebabfba6ab -r 420d72e5cee7 plugin/icedteanp/IcedTeaNPPlugin.cc
> --- a/plugin/icedteanp/IcedTeaNPPlugin.cc Fri Aug 23 16:35:37 2013 -0400
> +++ b/plugin/icedteanp/IcedTeaNPPlugin.cc Tue Aug 27 16:53:30 2013 -0400
> @@ -2101,14 +2101,6 @@
>
> if (data->is_applet_instance) // dummy instance/package?
> {
> - JavaRequestProcessor java_request = JavaRequestProcessor();
> - JavaResultData* java_result;
> - std::string instance_id = std::string();
> - std::string applet_class_id = std::string();
> -
> - int id = get_id_from_instance(instance);
> - gchar* id_str = g_strdup_printf ("%d", id);
> -
> // Some browsers.. (e.g. chromium) don't call NPP_SetWindow
> // for 0x0 plugins and therefore require initialization with
> // a 0 handle
> @@ -2117,30 +2109,10 @@
> plugin_send_initialization_message(data->instance_id, 0, 0, 0, data->parameters_string);
> }
>
> - java_result = java_request.getAppletObjectInstance(id_str);
> -
> - g_free(id_str);
> -
> - if (java_result->error_occurred)
> - {
> - printf("Error: Unable to fetch applet instance id from Java side.\n");
> - return NULL;
> - }
> -
> - instance_id.append(*(java_result->return_string));
> -
> - java_result = java_request.getClassID(instance_id);
> -
> - if (java_result->error_occurred)
> - {
> - printf("Error: Unable to fetch applet instance id from Java side.\n");
> - return NULL;
> - }
> -
> - applet_class_id.append(*(java_result->return_string));
> -
> - obj = IcedTeaScriptableJavaObject::get_scriptable_java_object(instance, applet_class_id, instance_id, false);
> -
> + NPObjectRef applet_object = IcedTeaScriptableJavaObject::get_scriptable_applet_object(instance);
> + /* Retain because we are returning an NPObject* */
> + applet_object.raw_retain();
> + obj = applet_object.get();
> } else
> {
> obj = IcedTeaScriptableJavaPackageObject::get_scriptable_java_package_object(instance, "");
> diff -r 94ebabfba6ab -r 420d72e5cee7 plugin/icedteanp/IcedTeaPluginUtils.cc
> --- a/plugin/icedteanp/IcedTeaPluginUtils.cc Fri Aug 23 16:35:37 2013 -0400
> +++ b/plugin/icedteanp/IcedTeaPluginUtils.cc Tue Aug 27 16:53:30 2013 -0400
> @@ -53,7 +53,7 @@
> int IcedTeaPluginUtilities::reference = -1;
> pthread_mutex_t IcedTeaPluginUtilities::reference_mutex = PTHREAD_MUTEX_INITIALIZER;
> 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*>();
> +std::map<std::string, NPObjectRef> IcedTeaPluginUtilities::object_map;
>
> /* Plugin async call queue */
> static std::vector< PluginThreadCall* >* pendingPluginThreadRequests = new std::vector< PluginThreadCall* >();
> @@ -433,9 +433,9 @@
> IcedTeaPluginUtilities::printStringVector(const char* prefix, std::vector<std::string>* str_vector)
> {
>
> - // This is a CPU intensive function. Run only if debugging
> - if (!plugin_debug)
> - return;
> + // This is a CPU intensive function. Run only if debugging
> + if (!plugin_debug)
> + return;
>
> std::string* str = new std::string();
> *str += "{ ";
> @@ -561,27 +561,25 @@
> * @return The associated active NPObject, NULL otherwise
> */
>
> -NPObject*
> +NPObjectRef
> IcedTeaPluginUtilities::getNPObjectFromJavaKey(std::string key)
> {
> -
> - NPObject* object = NULL;
> PLUGIN_DEBUG("getNPObjectFromJavaKey looking for %s\n", key.c_str());
>
> - std::map<std::string, NPObject*>::iterator iterator = object_map->find(key);
> + std::map<std::string, NPObjectRef>::iterator iterator = object_map.find(key);
>
> - if (iterator != object_map->end())
> + if (iterator != object_map.end())
> {
> - NPObject* mapped_object = object_map->find(key)->second;
> + NPObjectRef object = object_map.find(key)->second;
>
> - if (getInstanceFromMemberPtr(mapped_object) != NULL)
> + if (getInstanceFromMemberPtr(object.get()) != NULL)
> {
> - object = mapped_object;
> - PLUGIN_DEBUG("getNPObjectFromJavaKey found %s. NPObject = %p\n", key.c_str(), object);
> + PLUGIN_DEBUG("getNPObjectFromJavaKey found %s. NPObject = %p\n", key.c_str(), object.get());
> + return object;
> }
> }
>
> - return object;
> + return NPObjectRef(NULL);
> }
>
> /**
> @@ -592,10 +590,10 @@
> */
>
> void
> -IcedTeaPluginUtilities::storeObjectMapping(std::string key, NPObject* object)
> +IcedTeaPluginUtilities::storeObjectMapping(std::string key, NPObjectRef object)
> {
> - PLUGIN_DEBUG("Storing object %p with key %s\n", object, key.c_str());
> - object_map->insert(std::make_pair(key, object));
> + PLUGIN_DEBUG("Storing object %p with key %s\n", object.get(), key.c_str());
> + object_map.insert(std::make_pair(key, object));
> }
>
> /**
> @@ -608,19 +606,14 @@
> IcedTeaPluginUtilities::removeObjectMapping(std::string key)
> {
> PLUGIN_DEBUG("Removing key %s from object map\n", key.c_str());
> - object_map->erase(key);
> + object_map.erase(key);
> }
>
> /* Clear object_map. Useful for tests. */
> void
> IcedTeaPluginUtilities::clearObjectMapping()
> {
> - std::map<std::string, NPObject*>::iterator iter = object_map->begin();
> - for (; iter != object_map->end(); ++iter) {
> - browser_functions.releaseobject(iter->second);
> - }
> - delete object_map;
> - object_map = new std::map<std::string, NPObject*>();
> + object_map = std::map<std::string, NPObjectRef>();
> }
>
> /*
> @@ -633,9 +626,9 @@
> void
> IcedTeaPluginUtilities::printStringPtrVector(const char* prefix, std::vector<std::string*>* str_ptr_vector)
> {
> - // This is a CPU intensive function. Run only if debugging
> - if (!plugin_debug)
> - return;
> + // This is a CPU intensive function. Run only if debugging
> + if (!plugin_debug)
> + return;
>
> std::string* str = new std::string();
> *str += "{ ";
> @@ -819,18 +812,13 @@
>
> std::string jclass_id = *jclass_result->return_string;
>
> - NPObject* obj;
> - if (jclass_id.at(0) == '[') // array
> - {
> - obj = IcedTeaScriptableJavaObject::get_scriptable_java_object(instance, jclass_id,
> - jobject_id, true);
> - } else
> - {
> - obj = IcedTeaScriptableJavaObject::get_scriptable_java_object(instance, jclass_id,
> - jobject_id, false);
> - }
> + bool is_array = (jclass_id.at(0) == '[');
> + NPObjectRef object = IcedTeaScriptableJavaObject::get_scriptable_java_object(instance, jclass_id,
> + jobject_id, is_array);
>
> - OBJECT_TO_NPVARIANT(obj, *variant);
> + OBJECT_TO_NPVARIANT(object.get(), *variant);
> + /* Retain because we are returning an NPObject* */
> + object.raw_retain();
>
> return true;
> }
> @@ -874,12 +862,12 @@
> }
>
> bool
> -IcedTeaPluginUtilities::isObjectJSArray(NPP instance, NPObject* object)
> +IcedTeaPluginUtilities::isObjectJSArray(NPP instance, NPObjectRef object)
> {
>
> NPVariant constructor_v = NPVariant();
> NPIdentifier constructor_id = browser_functions.getstringidentifier("constructor");
> - browser_functions.getproperty(instance, object, constructor_id, &constructor_v);
> + browser_functions.getproperty(instance, object.get(), constructor_id, &constructor_v);
> IcedTeaPluginUtilities::printNPVariant(constructor_v);
>
> // void constructor => not an array
> @@ -1087,6 +1075,34 @@
> str = str.substr(start, end - start + 1);
> }
>
> +std::string
> +IcedTeaPluginUtilities::stringPrintf(const char* fmt, /*variable arguments*/ ...)
> +{
> + /* Extract the variable arguments */
> + va_list args;
> + va_start(args, fmt);
> +
> + gchar* result = NULL;
> + size_t size;
> +
> + /* Format the arguments into a new buffer, held in 'result' */
> + size = g_vasprintf (&result, fmt, args);
> +
> + if (result == NULL)
> + {
> + // We are out of memory
> + throw std::bad_alloc();
> + }
> +
> + /* Wrap as string and free buffer */
> + std::string str(result, size);
> + g_free(result);
> +
> + va_end(args); /* Finish using variable arguments */
> +
> + return str;
> +}
> +
> std::string IcedTeaPluginUtilities::NPIdentifierAsString(NPIdentifier id) {
> NPUTF8* cstr = browser_functions.utf8fromidentifier(id);
> if (cstr == NULL) {
> @@ -1242,3 +1258,41 @@
> PLUGIN_DEBUG("%p unlocked...\n", &msg_queue_mutex);
> }
>
> +void
> +NPObjectRef::set(NPObject* new_object)
> +{
> + if (new_object != NULL) {
> + /* Increase the new object's reference count */
> + browser_functions.retainobject(new_object);
> + }
> + if (current_object != NULL) {
> + /* Decrease the old object's reference count */
> + browser_functions.releaseobject(current_object);
> + }
> +
> + this->current_object = new_object;
> +}
> +
> +NPObjectRef
> +NPObjectRef::create(NPP instance, NPClass* np_class)
> +{
> + NPObjectRef ref;
> + /* Don't use set(), otherwise we'd double-retain */
> + ref.current_object = browser_functions.createobject(instance, np_class);
> + return ref;
> +}
> +
> +/* Explicit reference counting operations, use only if needed for interoperating with NPObject* */
> +void
> +NPObjectRef::raw_retain() {
> + if (current_object != NULL) {
> + browser_functions.retainobject(current_object);
> + }
> +}
> +
> +void
> +NPObjectRef::raw_release() {
> + if (current_object != NULL) {
> + browser_functions.releaseobject(current_object);
> + }
> +}
> diff -r 94ebabfba6ab -r 420d72e5cee7 plugin/icedteanp/IcedTeaPluginUtils.h
> --- a/plugin/icedteanp/IcedTeaPluginUtils.h Fri Aug 23 16:35:37 2013 -0400
> +++ b/plugin/icedteanp/IcedTeaPluginUtils.h Tue Aug 27 16:53:30 2013 -0400
> @@ -153,6 +153,61 @@
> /* Function to process all pending async calls */
> void processAsyncCallQueue(void*);
>
> +/* Reference-counted 'smart pointer' to NPObject */
> +class NPObjectRef {
> +public:
> + /* Create with browser_functions.createobject.
> + * This ensures the object is not double-retained. */
> + static NPObjectRef create(NPP instance, NPClass* np_class);
> +
> + NPObjectRef(NPObject* obj = NULL) {
> + current_object = NULL;
> + set(obj);
> + }
> +
> + NPObjectRef(const NPObjectRef& ref) {
> + current_object = NULL;
> + set(ref.current_object);
> + }
> +
> + NPObjectRef& operator=(const NPObjectRef& ref) {
> + set(ref.current_object);
> + return *this;
> + }
> +
> + ~NPObjectRef() {
> + clear();
> + }
> +
> + void set(NPObject* new_object);
> +
> + /* Get's the object pointer */
> + NPObject* get() {
> + return current_object;
> + }
> +
> + /* Helper for getting object as different type.
> + * NOTE: This cast is unchecked. */
> + template <typename T>
> + T as() {
> + return (T)current_object;
> + }
> +
> + /* Explicit reference counting operations, use only if needed for interoperating with NPObject* */
> + void raw_retain();
> + void raw_release();
> +
> + bool empty() {
> + return (current_object == NULL);
> + }
> +
> + void clear() {
> + set(NULL);
> + }
> +private:
> + NPObject* current_object;
> +};
> +
> class IcedTeaPluginUtilities
> {
>
> @@ -165,8 +220,8 @@
> /* Map holding window pointer<->instance relationships */
> static std::map<void*, NPP>* instance_map;
>
> - /* Map holding java-side-obj-key->NPObject relationship */
> - static std::map<std::string, NPObject*>* object_map;
> + /* Map holding java-side-obj-key->NPObject relationship. */
> + static std::map<std::string, NPObjectRef> object_map;
>
> /* Posts a call in the async call queue */
> static bool postPluginThreadAsyncCall(NPP instance, void (*func) (void *), void* data);
> @@ -202,6 +257,8 @@
> /* Converts the given integer to a string */
> static void itoa(int i, std::string* result);
>
> + static std::string stringPrintf(const char* fmt, ...);
> +
> /* Copies a variant data type into a C++ string */
> static std::string NPVariantAsString(NPVariant variant);
>
> @@ -262,9 +319,9 @@
>
> static NPP getInstanceFromMemberPtr(void* member_ptr);
>
> - static NPObject* getNPObjectFromJavaKey(std::string key);
> + static NPObjectRef getNPObjectFromJavaKey(std::string key);
>
> - static void storeObjectMapping(std::string key, NPObject* object);
> + static void storeObjectMapping(std::string key, NPObjectRef object);
>
> static void removeObjectMapping(std::string key);
>
> @@ -273,7 +330,7 @@
>
> static void invalidateInstance(NPP instance);
>
> - static bool isObjectJSArray(NPP instance, NPObject* object);
> + static bool isObjectJSArray(NPP instance, NPObjectRef object);
>
> static void decodeURL(const char* url, char** decoded_url);
>
> diff -r 94ebabfba6ab -r 420d72e5cee7 plugin/icedteanp/IcedTeaScriptablePluginObject.cc
> --- a/plugin/icedteanp/IcedTeaScriptablePluginObject.cc Fri Aug 23 16:35:37 2013 -0400
> +++ b/plugin/icedteanp/IcedTeaScriptablePluginObject.cc Tue Aug 27 16:53:30 2013 -0400
> @@ -306,24 +306,26 @@
>
> //NPIdentifier property = browser_functions.getstringidentifier(property_name.c_str());
>
> - NPObject* obj;
> + NPObjectRef object;
>
> if (isPropertyClass)
> {
> PLUGIN_DEBUG("Returning package object\n");
> - obj = IcedTeaScriptableJavaPackageObject::get_scriptable_java_package_object(
> + object = IcedTeaScriptableJavaPackageObject::get_scriptable_java_package_object(
> IcedTeaPluginUtilities::getInstanceFromMemberPtr(npobj),
> property_name.c_str());
> }
> else
> {
> PLUGIN_DEBUG("Returning Java object\n");
> - obj = IcedTeaScriptableJavaObject::get_scriptable_java_object(
> + object = IcedTeaScriptableJavaObject::get_scriptable_java_object(
> IcedTeaPluginUtilities::getInstanceFromMemberPtr(npobj),
> *(java_result->return_string), "0", false);
> }
>
> - OBJECT_TO_NPVARIANT(obj, *result);
> + OBJECT_TO_NPVARIANT(object.get(), *result);
> + /* Retain because we are returning an NPObject* */
> + object.raw_retain();
>
> return true;
> }
> @@ -384,32 +386,33 @@
> return np_class;
> }
>
> -NPObject*
> -IcedTeaScriptableJavaObject::get_scriptable_java_object(NPP instance,
> - std::string class_id,
> - std::string instance_id,
> - bool isArray)
> +/* Creates a scriptable java object (intended to be called asynch.) */
> +static void
> +create_scriptable_java_object_async(void* data)
> +{
> + PLUGIN_DEBUG("Asynchronously creating object ...\n");
> +
> + std::vector<void*> parameters = ((AsyncCallThreadData*) data)->parameters;
> + NPP instance = (NPP) parameters.at(0);
> + NPClass* np_class = (NPClass*) parameters.at(1);
> + NPObjectRef* object_ref = (NPObjectRef*) parameters.at(2);
> +
> + *object_ref = browser_functions.createobject(instance, np_class);
> +
> + ((AsyncCallThreadData*) data)->result_ready = true;
> +}
> +
> +static NPObjectRef
> +create_scriptable_java_object(NPP instance)
> {
> /* Shared NPClass instance for IcedTeaScriptablePluginObject */
> static NPClass np_class = scriptable_java_package_object_class();
>
> - std::string obj_key = class_id + ":" + instance_id;
> -
> - PLUGIN_DEBUG("get_scriptable_java_object searching for %s...\n", obj_key.c_str());
> - IcedTeaScriptableJavaObject* scriptable_object = (IcedTeaScriptableJavaObject*) IcedTeaPluginUtilities::getNPObjectFromJavaKey(obj_key);
> -
> - if (scriptable_object != NULL)
> - {
> - PLUGIN_DEBUG("Returning existing object %p\n", scriptable_object);
> - browser_functions.retainobject(scriptable_object);
> - return scriptable_object;
> - }
> -
> // try to create normally
> - scriptable_object = (IcedTeaScriptableJavaObject*)browser_functions.createobject(instance, &np_class);
> + NPObjectRef np_object = NPObjectRef::create(instance, &np_class);
>
> // didn't work? try creating asynch
> - if (!scriptable_object)
> + if (np_object.empty())
> {
> AsyncCallThreadData thread_data = AsyncCallThreadData();
> thread_data.result_ready = false;
> @@ -418,45 +421,104 @@
>
> thread_data.parameters.push_back(instance);
> thread_data.parameters.push_back(&np_class);
> - thread_data.parameters.push_back(&scriptable_object);
This changeset causes nearly all of our JavaScript-related reproducer
tests to fail. The failure is very clearly illustrated using external
test [1] as well. You can verify the failure easily by running this test
with hg head vs 1.4.1 from official repos. I have attempted to find the
specific cause of failure within the changeset but I haven't been
successful. Some small parts of it are acceptable to apply (for example,
the object_map in IcedTeaPluginUtils being changed from
std::map<std::string, NPObject*>* to std::map<std::string, NPObject*>
seems to be harmless). However the patch contains two large changes -
one to incorporate a new 'smart pointer' for NPObject*, and one to
enable lazy-initialization. I tried to manually backout each of those
large changes separately, but with either one still applied, the
failures persisted.
As such I'd like to propose that we backout this changeset.
[1]
https://jdk6.java.net/nonav/plugin2/liveconnect/LiveConnectTests/
Thanks,
--
Andrew A
More information about the distro-pkg-dev
mailing list