/hg/icedtea-web: 2 new changesets
adomurad at icedtea.classpath.org
adomurad at icedtea.classpath.org
Fri Jun 21 08:55:51 PDT 2013
changeset 8c77698ab575 in /hg/icedtea-web
details: http://icedtea.classpath.org/hg/icedtea-web?cmd=changeset;node=8c77698ab575
author: Adam Domurad <adomurad at redhat.com>
date: Fri Jun 21 11:39:00 2013 -0400
More unit tests for scriptable object creation, destruction
changeset cfdb17c00603 in /hg/icedtea-web
details: http://icedtea.classpath.org/hg/icedtea-web?cmd=changeset;node=cfdb17c00603
author: Adam Domurad <adomurad at redhat.com>
date: Fri Jun 21 11:41:31 2013 -0400
Fix memory leak due to allocated NPClass
diffstat:
ChangeLog | 22 +++
plugin/icedteanp/IcedTeaPluginUtils.cc | 20 +++
plugin/icedteanp/IcedTeaPluginUtils.h | 10 +-
plugin/icedteanp/IcedTeaScriptablePluginObject.cc | 96 +++++++-------
tests/cpp-unit-tests/IcedTeaPluginUtilsTest.cc | 3 -
tests/cpp-unit-tests/IcedTeaScriptablePluginObjectTest.cc | 52 ++++++-
tests/cpp-unit-tests/MemoryLeakDetector.h | 85 +++++++++++++
tests/cpp-unit-tests/browser_mock.cc | 15 ++-
tests/cpp-unit-tests/main.cc | 10 +-
9 files changed, 247 insertions(+), 66 deletions(-)
diffs (477 lines):
diff -r a236aa5f729b -r cfdb17c00603 ChangeLog
--- a/ChangeLog Fri Jun 21 12:15:03 2013 +0200
+++ b/ChangeLog Fri Jun 21 11:41:31 2013 -0400
@@ -1,3 +1,25 @@
+2013-06-21 Adam Domurad <adomurad at redhat.com>
+
+ * plugin/icedteanp/IcedTeaScriptablePluginObject.cc
+ (IcedTeaScriptablePluginObject::get_scriptable_java_package_object): Fix
+ memory leak due to allocated NPClass.
+ (IcedTeaScriptableJavaPackageObject::get_scriptable_java_object):
+ Same.
+
+2013-06-21 Adam Domurad <adomurad at redhat.com>
+
+ * plugin/icedteanp/IcedTeaPluginUtils.cc: Add global state clearing
+ utility functions.
+ * plugin/icedteanp/IcedTeaPluginUtils.h: Same.
+ * tests/cpp-unit-tests/IcedTeaScriptablePluginObjectTest.cc: Test
+ scriptable object creation and destruction.
+ * tests/cpp-unit-tests/browser_mock.cc
+ (mock_createobject): New, mocks NPAPI 'createobject'.
+ * tests/cpp-unit-tests/MemoryLeakDetector.h: New, memory leak detection
+ utility class.
+ * tests/cpp-unit-tests/main.cc
+ (ReportTestFinish): Print which tests resulted in memory leaks.
+
2013-06-21 Jiri Vanek <jvanek at redhat.com>
Adam Domurad <adomurad at redhat.com>
Omair Majid <omajid at redhat.com>
diff -r a236aa5f729b -r cfdb17c00603 plugin/icedteanp/IcedTeaPluginUtils.cc
--- a/plugin/icedteanp/IcedTeaPluginUtils.cc Fri Jun 21 12:15:03 2013 +0200
+++ b/plugin/icedteanp/IcedTeaPluginUtils.cc Fri Jun 21 11:41:31 2013 -0400
@@ -498,6 +498,14 @@
instance_map->erase(member_ptr);
}
+/* Clear instance_map. Useful for tests. */
+void
+IcedTeaPluginUtilities::clearInstanceIDs()
+{
+ delete instance_map;
+ instance_map = new std::map<void*, NPP>();
+}
+
/**
* Removes all mappings to a given instance, and all associated objects
*/
@@ -603,6 +611,18 @@
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*>();
+}
+
/*
* Similar to printStringVector, but takes a vector of string pointers instead
*
diff -r a236aa5f729b -r cfdb17c00603 plugin/icedteanp/IcedTeaPluginUtils.h
--- a/plugin/icedteanp/IcedTeaPluginUtils.h Fri Jun 21 12:15:03 2013 +0200
+++ b/plugin/icedteanp/IcedTeaPluginUtils.h Fri Jun 21 11:41:31 2013 -0400
@@ -252,9 +252,12 @@
static void storeInstanceID(void* member_ptr, NPP instance);
- static void removeInstanceID(void* member_ptr);
+ static void removeInstanceID(void* member_ptr);
- static NPP getInstanceFromMemberPtr(void* member_ptr);
+ /* Clear object_map. Useful for tests. */
+ static void clearInstanceIDs();
+
+ static NPP getInstanceFromMemberPtr(void* member_ptr);
static NPObject* getNPObjectFromJavaKey(std::string key);
@@ -262,6 +265,9 @@
static void removeObjectMapping(std::string key);
+ /* Clear object_map. Useful for tests. */
+ static void clearObjectMapping();
+
static void invalidateInstance(NPP instance);
static bool isObjectJSArray(NPP instance, NPObject* object);
diff -r a236aa5f729b -r cfdb17c00603 plugin/icedteanp/IcedTeaScriptablePluginObject.cc
--- a/plugin/icedteanp/IcedTeaScriptablePluginObject.cc Fri Jun 21 12:15:03 2013 +0200
+++ b/plugin/icedteanp/IcedTeaScriptablePluginObject.cc Fri Jun 21 11:41:31 2013 -0400
@@ -139,35 +139,39 @@
return new IcedTeaScriptableJavaPackageObject(npp);
}
+static NPClass
+scriptable_plugin_object_class() {
+ NPClass np_class;
+ np_class.structVersion = NP_CLASS_STRUCT_VERSION;
+ np_class.allocate = allocate_scriptable_jp_object;
+ np_class.deallocate = IcedTeaScriptableJavaPackageObject::deAllocate;
+ np_class.invalidate = IcedTeaScriptableJavaPackageObject::invalidate;
+ np_class.hasMethod = IcedTeaScriptableJavaPackageObject::hasMethod;
+ np_class.invoke = IcedTeaScriptableJavaPackageObject::invoke;
+ np_class.invokeDefault = IcedTeaScriptableJavaPackageObject::invokeDefault;
+ np_class.hasProperty = IcedTeaScriptableJavaPackageObject::hasProperty;
+ np_class.getProperty = IcedTeaScriptableJavaPackageObject::getProperty;
+ np_class.setProperty = IcedTeaScriptableJavaPackageObject::setProperty;
+ np_class.removeProperty = IcedTeaScriptableJavaPackageObject::removeProperty;
+ np_class.enumerate = IcedTeaScriptableJavaPackageObject::enumerate;
+ np_class.construct = IcedTeaScriptableJavaPackageObject::construct;
+ return np_class;
+}
+
NPObject*
IcedTeaScriptablePluginObject::get_scriptable_java_package_object(NPP instance, const NPUTF8* name)
{
+ /* Shared NPClass instance for IcedTeaScriptablePluginObject */
+ static NPClass np_class = scriptable_plugin_object_class();
- NPObject* scriptable_object;
-
- NPClass* np_class = new NPClass();
- np_class->structVersion = NP_CLASS_STRUCT_VERSION;
- np_class->allocate = allocate_scriptable_jp_object;
- np_class->deallocate = IcedTeaScriptableJavaPackageObject::deAllocate;
- np_class->invalidate = IcedTeaScriptableJavaPackageObject::invalidate;
- np_class->hasMethod = IcedTeaScriptableJavaPackageObject::hasMethod;
- np_class->invoke = IcedTeaScriptableJavaPackageObject::invoke;
- np_class->invokeDefault = IcedTeaScriptableJavaPackageObject::invokeDefault;
- np_class->hasProperty = IcedTeaScriptableJavaPackageObject::hasProperty;
- np_class->getProperty = IcedTeaScriptableJavaPackageObject::getProperty;
- np_class->setProperty = IcedTeaScriptableJavaPackageObject::setProperty;
- np_class->removeProperty = IcedTeaScriptableJavaPackageObject::removeProperty;
- np_class->enumerate = IcedTeaScriptableJavaPackageObject::enumerate;
- np_class->construct = IcedTeaScriptableJavaPackageObject::construct;
-
- scriptable_object = browser_functions.createobject(instance, np_class);
- PLUGIN_DEBUG("Returning new scriptable package class: %p from instance %p with name %s\n", scriptable_object, instance, name);
+ NPObject* scriptable_object = browser_functions.createobject(instance, &np_class);
+ PLUGIN_DEBUG("Returning new scriptable package class: %p from instance %p with name %s\n", scriptable_object, instance, name);
((IcedTeaScriptableJavaPackageObject*) scriptable_object)->setPackageName(name);
IcedTeaPluginUtilities::storeInstanceID(scriptable_object, instance);
- return scriptable_object;
+ return scriptable_object;
}
IcedTeaScriptableJavaPackageObject::IcedTeaScriptableJavaPackageObject(NPP instance)
@@ -357,21 +361,39 @@
return new IcedTeaScriptableJavaObject(npp);
}
+
+static NPClass
+scriptable_java_package_object_class() {
+ NPClass np_class;
+ np_class.structVersion = NP_CLASS_STRUCT_VERSION;
+ np_class.allocate = allocate_scriptable_java_object;
+ np_class.deallocate = IcedTeaScriptableJavaObject::deAllocate;
+ np_class.invalidate = IcedTeaScriptableJavaObject::invalidate;
+ np_class.hasMethod = IcedTeaScriptableJavaObject::hasMethod;
+ np_class.invoke = IcedTeaScriptableJavaObject::invoke;
+ np_class.invokeDefault = IcedTeaScriptableJavaObject::invokeDefault;
+ np_class.hasProperty = IcedTeaScriptableJavaObject::hasProperty;
+ np_class.getProperty = IcedTeaScriptableJavaObject::getProperty;
+ np_class.setProperty = IcedTeaScriptableJavaObject::setProperty;
+ np_class.removeProperty = IcedTeaScriptableJavaObject::removeProperty;
+ np_class.enumerate = IcedTeaScriptableJavaObject::enumerate;
+ np_class.construct = IcedTeaScriptableJavaObject::construct;
+ return np_class;
+}
+
NPObject*
IcedTeaScriptableJavaPackageObject::get_scriptable_java_object(NPP instance,
std::string class_id,
std::string instance_id,
bool isArray)
{
- NPObject* scriptable_object;
+ /* Shared NPClass instance for IcedTeaScriptablePluginObject */
+ static NPClass np_class = scriptable_java_package_object_class();
- std::string obj_key = std::string();
- obj_key += class_id;
- obj_key += ":";
- obj_key += instance_id;
+ std::string obj_key = class_id + ":" + instance_id;
PLUGIN_DEBUG("get_scriptable_java_object searching for %s...\n", obj_key.c_str());
- scriptable_object = IcedTeaPluginUtilities::getNPObjectFromJavaKey(obj_key);
+ NPObject* scriptable_object = IcedTeaPluginUtilities::getNPObjectFromJavaKey(obj_key);
if (scriptable_object != NULL)
{
@@ -380,24 +402,8 @@
return scriptable_object;
}
-
- NPClass* np_class = new NPClass();
- np_class->structVersion = NP_CLASS_STRUCT_VERSION;
- np_class->allocate = allocate_scriptable_java_object;
- np_class->deallocate = IcedTeaScriptableJavaObject::deAllocate;
- np_class->invalidate = IcedTeaScriptableJavaObject::invalidate;
- np_class->hasMethod = IcedTeaScriptableJavaObject::hasMethod;
- np_class->invoke = IcedTeaScriptableJavaObject::invoke;
- np_class->invokeDefault = IcedTeaScriptableJavaObject::invokeDefault;
- np_class->hasProperty = IcedTeaScriptableJavaObject::hasProperty;
- np_class->getProperty = IcedTeaScriptableJavaObject::getProperty;
- np_class->setProperty = IcedTeaScriptableJavaObject::setProperty;
- np_class->removeProperty = IcedTeaScriptableJavaObject::removeProperty;
- np_class->enumerate = IcedTeaScriptableJavaObject::enumerate;
- np_class->construct = IcedTeaScriptableJavaObject::construct;
-
- // try to create normally
- scriptable_object = browser_functions.createobject(instance, np_class);
+ // try to create normally
+ scriptable_object = browser_functions.createobject(instance, &np_class);
// didn't work? try creating asynch
if (!scriptable_object)
@@ -408,7 +414,7 @@
thread_data.result = std::string();
thread_data.parameters.push_back(instance);
- thread_data.parameters.push_back(np_class);
+ thread_data.parameters.push_back(&np_class);
thread_data.parameters.push_back(&scriptable_object);
IcedTeaPluginUtilities::callAndWaitForResult(instance, &_createAndRetainJavaObject, &thread_data);
diff -r a236aa5f729b -r cfdb17c00603 tests/cpp-unit-tests/IcedTeaPluginUtilsTest.cc
--- a/tests/cpp-unit-tests/IcedTeaPluginUtilsTest.cc Fri Jun 21 12:15:03 2013 +0200
+++ b/tests/cpp-unit-tests/IcedTeaPluginUtilsTest.cc Fri Jun 21 11:41:31 2013 -0400
@@ -44,9 +44,6 @@
#include "IcedTeaNPPlugin.h"
#include <fstream>
-extern void trim(std::string& str);
-extern bool file_exists(std::string filename);
-
TEST(NPVariantAsString) {
NPVariant var;
STRINGZ_TO_NPVARIANT("test", var);
diff -r a236aa5f729b -r cfdb17c00603 tests/cpp-unit-tests/IcedTeaScriptablePluginObjectTest.cc
--- a/tests/cpp-unit-tests/IcedTeaScriptablePluginObjectTest.cc Fri Jun 21 12:15:03 2013 +0200
+++ b/tests/cpp-unit-tests/IcedTeaScriptablePluginObjectTest.cc Fri Jun 21 11:41:31 2013 -0400
@@ -39,28 +39,60 @@
#include <npapi.h>
#include "browser_mock.h"
-#include "checked_allocations.h"
+#include "MemoryLeakDetector.h"
#include "IcedTeaScriptablePluginObject.h"
+#include "IcedTeaPluginUtils.h"
+
+static NPP_t dummy_npp = {0,0};
+
+SUITE(IcedTeaScriptablePluginObject) {
+ TEST(destructor) {
+ MemoryLeakDetector leak_detector;
+ IcedTeaScriptablePluginObject* obj = new IcedTeaScriptablePluginObject(&dummy_npp);
+ delete obj;
+ CHECK(leak_detector.memory_leaks() == 0);
+ }
+
+ TEST(get_scriptable_java_object) {
+ MemoryLeakDetector leak_detector;
+ NPObject* obj = IcedTeaScriptablePluginObject::get_scriptable_java_package_object(&dummy_npp, "DummyPackage");
+ browser_functions.releaseobject(obj);
+ CHECK(leak_detector.memory_leaks() == 0);
+ }
+}
SUITE(IcedTeaScriptableJavaObject) {
TEST(deallocate) {
- int pre_allocations = cpp_unfreed_allocations();
- IcedTeaScriptableJavaObject* obj = new IcedTeaScriptableJavaObject(NULL);
+ MemoryLeakDetector leak_detector;
+ IcedTeaScriptableJavaObject* obj = new IcedTeaScriptableJavaObject(&dummy_npp);
IcedTeaScriptableJavaObject::deAllocate(obj);
- int post_allocations = cpp_unfreed_allocations();
-
- CHECK(pre_allocations == post_allocations);
+ CHECK(leak_detector.memory_leaks() == 0);
}
}
SUITE(IcedTeaScriptableJavaPackageObject) {
TEST(deallocate) {
- int pre_allocations = cpp_unfreed_allocations();
- IcedTeaScriptableJavaPackageObject* obj = new IcedTeaScriptableJavaPackageObject(NULL);
+ MemoryLeakDetector leak_detector;
+ IcedTeaScriptableJavaPackageObject* obj = new IcedTeaScriptableJavaPackageObject(&dummy_npp);
IcedTeaScriptableJavaPackageObject::deAllocate(obj);
- int post_allocations = cpp_unfreed_allocations();
+ CHECK(leak_detector.memory_leaks() == 0);
+ }
- CHECK(pre_allocations == post_allocations);
+ TEST(get_scriptable_java_object) {
+ MemoryLeakDetector leak_detector;
+
+ NPObject* first_obj = IcedTeaScriptableJavaPackageObject::get_scriptable_java_object(&dummy_npp, "DummyClass", "DummyInstance", false);
+ browser_functions.releaseobject(first_obj);
+
+ /* After the first call, the object should be cached in the object map */
+ NPObject* second_obj = IcedTeaScriptableJavaPackageObject::get_scriptable_java_object(&dummy_npp, "DummyClass", "DummyInstance", false);
+
+ /* Objects should be the same, because of caching */
+ CHECK(first_obj == second_obj);
+
+ browser_functions.releaseobject(second_obj);
+
+ CHECK(leak_detector.memory_leaks() == 0);
}
}
diff -r a236aa5f729b -r cfdb17c00603 tests/cpp-unit-tests/MemoryLeakDetector.h
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/tests/cpp-unit-tests/MemoryLeakDetector.h Fri Jun 21 11:41:31 2013 -0400
@@ -0,0 +1,85 @@
+/* Copyright (C) 2013 Red Hat
+
+ This file is part of IcedTea.
+
+ IcedTea is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2, or (at your option)
+ any later version.
+
+ IcedTea is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with IcedTea; see the file COPYING. If not, write to the
+ Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ 02110-1301 USA.
+
+ Linking this library statically or dynamically with other modules is
+ making a combined work based on this library. Thus, the terms and
+ conditions of the GNU General Public License cover the whole
+ combination.
+
+ As a special exception, the copyright holders of this library give you
+ permission to link this library with independent modules to produce an
+ executable, regardless of the license terms of these independent
+ modules, and to copy and distribute the resulting executable under
+ terms of your choice, provided that you also meet, for each linked
+ independent module, the terms and conditions of the license of that
+ module. An independent module is a module which is not derived from
+ or based on this library. If you modify this library, you may extend
+ this exception to your version of the library, but you are not
+ obligated to do so. If you do not wish to do so, delete this
+ exception statement from your version. */
+
+// Memory leak detection helper class.
+// This utilizes checked_allocations.h & browser_mock.h to query how many unfreed allocations exist.
+// As well, it clears global state that is problematic for accurate measure of memory leaks.
+
+#ifndef MEMORYLEAKDETECTOR_H_
+#define MEMORYLEAKDETECTOR_H_
+
+#include <cstdio>
+#include "checked_allocations.h"
+#include "IcedTeaPluginUtils.h"
+
+class MemoryLeakDetector {
+public:
+ MemoryLeakDetector() {
+ reset();
+ }
+
+ /* Reset allocation counts and certain global state touched by the tests.
+ * This is necessary to ensure accurate leak reporting for some functions. */
+ void reset() {
+ reset_global_state();
+ initial_cpp_allocations = cpp_unfreed_allocations();
+ initial_npapi_allocations = browsermock_unfreed_allocations();
+ }
+
+ /* Return allocation counts, after clearing global state that can conflict with the
+ * leak detection. */
+ int memory_leaks() {
+ reset_global_state();
+ int cpp_leaks = cpp_unfreed_allocations() - initial_cpp_allocations;
+ int npapi_leaks = browsermock_unfreed_allocations() - initial_npapi_allocations;
+
+ return cpp_leaks + npapi_leaks;
+ }
+
+private:
+ static void reset_global_state() {
+ /* Clears allocations caused by storeInstanceID */
+ IcedTeaPluginUtilities::clearInstanceIDs();
+ /* Clears allocations caused by storeObjectMapping */
+ IcedTeaPluginUtilities::clearObjectMapping();
+ }
+
+ int initial_cpp_allocations;
+ int initial_npapi_allocations;
+};
+
+
+#endif /* MEMORYLEAKDETECTOR_H_ */
diff -r a236aa5f729b -r cfdb17c00603 tests/cpp-unit-tests/browser_mock.cc
--- a/tests/cpp-unit-tests/browser_mock.cc Fri Jun 21 12:15:03 2013 +0200
+++ b/tests/cpp-unit-tests/browser_mock.cc Fri Jun 21 11:41:31 2013 -0400
@@ -73,17 +73,30 @@
if (obj->_class->deallocate) {
obj->_class->deallocate(obj);
} else {
- free(obj);
+ mock_memfree(obj);
}
}
}
+static NPObject* mock_createobject(NPP instance, NPClass* np_class) {
+ NPObject* obj;
+ if (np_class->allocate) {
+ obj = np_class->allocate(instance, np_class);
+ } else {
+ obj = (NPObject*) mock_memalloc(sizeof(NPObject));
+ }
+ obj->referenceCount = 1;
+ obj->_class = np_class;
+ return obj;
+}
+
void browsermock_setup_functions() {
memset(&browser_functions, 0, sizeof(NPNetscapeFuncs));
browser_functions.memalloc = &mock_memalloc;
browser_functions.memfree = &mock_memfree;
+ browser_functions.createobject = &mock_createobject;
browser_functions.retainobject = &mock_retainobject;
browser_functions.releaseobject= &mock_releaseobject;
}
diff -r a236aa5f729b -r cfdb17c00603 tests/cpp-unit-tests/main.cc
--- a/tests/cpp-unit-tests/main.cc Fri Jun 21 12:15:03 2013 +0200
+++ b/tests/cpp-unit-tests/main.cc Fri Jun 21 11:41:31 2013 -0400
@@ -85,18 +85,18 @@
float secondsElapsed) {
int posttest_allocs = cpp_unfreed_allocations();
+ std::string testname = full_testname(details);
if (browsermock_unfreed_allocations() > 0) {
- printf("*** WARNING: Memory leak! %d more NPAPI allocations than frees!\n",
- browsermock_unfreed_allocations());
+ printf("*** WARNING: %s has a memory leak! %d more NPAPI allocations than frees!\n",
+ testname.c_str(), browsermock_unfreed_allocations());
}
if (posttest_allocs > pretest_allocs) {
- printf("*** WARNING: Memory leak! %d more operator 'new' allocations than 'delete's!\n",
- posttest_allocs - pretest_allocs);
+ printf("*** WARNING: %s has a memory leak! %d more operator 'new' allocations than 'delete's!\n",
+ testname.c_str(), posttest_allocs - pretest_allocs);
}
if (did_finish_correctly) {
- std::string testname = full_testname(details);
printf("Passed: %s\n", testname.c_str());
}
}
More information about the distro-pkg-dev
mailing list