Hello playmobil,
I'd like you to do a code review. Please execute
g4 diff -c 8200629
or point your web browser to
http://mondrian/8200629
to review the following code:
Change 8200629 by [EMAIL PROTECTED] on 2008/09/08 14:05:50 *pending*
Plug DocumentJsRunner leak on NPAPI. This is the NPAPI analog of
FF's 7871060 and IE's 8151949.
PRESUBMIT=passed
R=playmobil
[EMAIL PROTECTED]
DELTA=66 (48 added, 11 deleted, 7 changed)
OCL=8200629
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_runner_np.cc#24
edit
... //depot/googleclient/gears/opensource/gears/base/common/leak_counter.cc#9
edit
... //depot/googleclient/gears/opensource/gears/base/common/leak_counter.h#8
edit
... //depot/googleclient/gears/opensource/gears/base/npapi/npp_bindings.cc#7
edit
66 delta lines: 48 added, 11 deleted, 7 changed
Also consider running:
g4 lint -c 8200629
which verifies that the changelist doesn't introduce new style violations.
If you can't do the review, please let me know as soon as possible. During
your review, please ensure that all new code has corresponding unit tests and
that existing unit tests are updated appropriately. Visit
http://www/eng/code_review.html for more information.
This is a semiautomated message from "g4 mail". Complaints or suggestions?
Mail [EMAIL PROTECTED]
Change 8200629 by [EMAIL PROTECTED] on 2008/09/08 14:05:50 *pending*
Plug DocumentJsRunner leak on NPAPI. This is the NPAPI analog of
FF's 7871060 and IE's 8151949.
OCL=8200629
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_runner_np.cc#24
edit
... //depot/googleclient/gears/opensource/gears/base/common/leak_counter.cc#9
edit
... //depot/googleclient/gears/opensource/gears/base/common/leak_counter.h#8
edit
... //depot/googleclient/gears/opensource/gears/base/npapi/npp_bindings.cc#7
edit
==== //depot/googleclient/gears/opensource/gears/base/common/js_runner_np.cc#24
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/base/common/js_runner_np.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_runner_np.cc
2008-09-08 14:03:07.000000000 +1000
+++ googleclient/gears/opensource/gears/base/common/js_runner_np.cc
2008-09-08 14:04:05.000000000 +1000
@@ -43,6 +43,7 @@
#include "gears/base/common/js_runner.h"
#include "gears/base/common/js_runner_utils.h" // For ThrowGlobalErrorImpl()
#include "gears/base/common/js_standalone_engine.h"
+#include "gears/base/common/leak_counter.h"
#include "gears/base/common/mutex.h"
#include "gears/base/common/string_utils.h"
#include "gears/base/common/thread_locals.h"
@@ -73,9 +74,6 @@
if (!g_document_js_runners)
return;
- // TODO(nigeltao): Are we leaking the DocumentJsRunner (since the map's
- // value type is just a raw pointer, not a linked_ptr)? If so, this would
- // be a good time to delete it.
g_document_js_runners->erase(instance);
if (g_document_js_runners->empty()) {
delete g_document_js_runners;
@@ -95,16 +93,11 @@
virtual ~JsRunnerBase() {
// Should have called Cleanup() to kill this before getting here();
assert(evaluator_.get() == NULL);
- }
-
- void OnModuleEnvironmentAttach() {
- // TODO(nigeltao): implement on NPAPI, i.e. plug the DocumentJsRunner leak.
- }
-
- void OnModuleEnvironmentDetach() {
- // TODO(nigeltao): implement on NPAPI, i.e. plug the DocumentJsRunner leak.
- }
-
+ for (int i = 0; i < MAX_JSEVENTS; i++) {
+ assert(0 == event_handlers_[i].size());
+ }
+ }
+
virtual NPObject *GetGlobalObject() = 0;
// Because JsRunner destroys the JSEngine in it's destructor, we add this
@@ -434,7 +427,8 @@
#endif
static bool initialized = false;
static Mutex browser_callbacks_mutex;
-
+ LEAK_COUNTER_INCREMENT(JsRunner);
+
if (!initialized) {
MutexLock lock(&browser_callbacks_mutex);
JSStandaloneEngine::GetNPNEntryPoints(
@@ -448,6 +442,7 @@
}
virtual ~JsRunner() {
+ LEAK_COUNTER_DECREMENT(JsRunner);
// Alert modules that the engine is unloading.
SendEvent(JSEVENT_UNLOAD);
@@ -456,6 +451,14 @@
NPN_ReleaseObject(global_object_);
JSStandaloneEngine::TerminateEngine();
+ }
+
+ void OnModuleEnvironmentAttach() {
+ // No-op. A JsRunner is not self-deleting, unlike a DocumentJsRunner.
+ }
+
+ void OnModuleEnvironmentDetach() {
+ // No-op. A JsRunner is not self-deleting, unlike a DocumentJsRunner.
}
JsContextPtr GetContext() {
@@ -524,19 +527,31 @@
class DocumentJsRunner : public JsRunnerBase {
public:
DocumentJsRunner(NPP instance)
- : np_instance_(instance) {
+ : np_instance_(instance),
+ is_module_environment_detached_(true),
+ is_unloaded_(false) {
+ LEAK_COUNTER_INCREMENT(DocumentJsRunner);
NPN_GetValue(np_instance_, NPNVWindowNPObject, &global_object_);
RegisterDocumentJsRunner(np_instance_, this);
}
virtual ~DocumentJsRunner() {
- // TODO(mpcomplete): This never gets called. When should we delete the
- // DocumentJsRunner?
+ LEAK_COUNTER_DECREMENT(DocumentJsRunner);
+ assert(is_module_environment_detached_ && is_unloaded_);
Cleanup();
UnregisterDocumentJsRunner(np_instance_);
NPN_ReleaseObject(global_object_);
}
+ void OnModuleEnvironmentAttach() {
+ is_module_environment_detached_ = false;
+ }
+
+ void OnModuleEnvironmentDetach() {
+ is_module_environment_detached_ = true;
+ DeleteIfNoLongerRelevant();
+ }
+
NPObject *GetGlobalObject() {
return global_object_;
}
@@ -572,6 +587,8 @@
void HandleNPInstanceDestroyed() {
SendEvent(JSEVENT_UNLOAD);
UnregisterDocumentJsRunner(np_instance_);
+ is_unloaded_ = true;
+ DeleteIfNoLongerRelevant();
}
// TODO(mpcomplete): We only support JSEVENT_UNLOAD. We should rework this
@@ -583,8 +600,16 @@
}
private:
+ void DeleteIfNoLongerRelevant() {
+ if (is_module_environment_detached_ && is_unloaded_) {
+ delete this;
+ }
+ }
+
NPP np_instance_;
NPObject *global_object_;
+ bool is_module_environment_detached_;
+ bool is_unloaded_;
DISALLOW_EVIL_CONSTRUCTORS(DocumentJsRunner);
};
==== //depot/googleclient/gears/opensource/gears/base/common/leak_counter.cc#9
-
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/base/common/leak_counter.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/leak_counter.cc
2008-09-08 14:03:07.000000000 +1000
+++ googleclient/gears/opensource/gears/base/common/leak_counter.cc
2008-09-08 13:40:17.000000000 +1000
@@ -93,8 +93,7 @@
}
#ifdef WIN32
::MessageBox(0, s.c_str(), PRODUCT_FRIENDLY_NAME, MB_OK);
-#endif
-#ifdef LINUX
+#else
std::string s_as_utf8;
String16ToUTF8(s, &s_as_utf8);
printf("%s", s_as_utf8.c_str());
==== //depot/googleclient/gears/opensource/gears/base/common/leak_counter.h#8 -
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/base/common/leak_counter.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/leak_counter.h
2008-09-08 14:03:07.000000000 +1000
+++ googleclient/gears/opensource/gears/base/common/leak_counter.h
2008-09-08 11:47:40.000000000 +1000
@@ -27,12 +27,10 @@
#define GEARS_BASE_COMMON_LEAK_COUNTER_H__
#ifdef DEBUG
-#if BROWSER_FF || BROWSER_IE
#ifdef WINCE
// TODO(nigeltao): figure out some sort of UI for showing leaks on WinCE.
#else
#define ENABLE_LEAK_COUNTING 1
-#endif
#endif
#endif
==== //depot/googleclient/gears/opensource/gears/base/npapi/npp_bindings.cc#7 -
/Users/nigeltao/devel/srcmacgears1/googleclient/gears/opensource/gears/base/npapi/npp_bindings.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/npapi/npp_bindings.cc
2008-09-08 14:03:07.000000000 +1000
+++ googleclient/gears/opensource/gears/base/npapi/npp_bindings.cc
2008-09-08 13:53:42.000000000 +1000
@@ -41,12 +41,27 @@
// particular plugin.
//
#include "gears/base/common/string_utils.h"
+#include "gears/base/common/leak_counter.h"
#include "gears/base/npapi/module.h"
#include "gears/base/npapi/plugin.h"
#include "gears/factory/factory_np.h"
#include "genfiles/product_constants.h"
std::string16 g_user_agent; // Access externally via BrowserUtils class.
+
+#if ENABLE_LEAK_COUNTING
+// This class (and its sole instance) is solely to provide appropriate times
+// to set up and tear down the leak counting code.
+class NpapiLeakCounter {
+ public:
+ NpapiLeakCounter() {
+ LEAK_COUNTER_INITIALIZE();
+ }
+ ~NpapiLeakCounter() {
+ LEAK_COUNTER_DUMP_COUNTS();
+ }
+} npapi_leak_counter_global_instance;
+#endif
#ifdef OS_ANDROID
const ThreadLocals::Slot kNPPInstance = ThreadLocals::Alloc();
