Hello cprince,
I'd like you to do a code review. Please execute
g4 diff -c 8151949
or point your web browser to
http://mondrian/8151949
to review the following code:
Change 8151949 by [EMAIL PROTECTED] on 2008/09/02 18:55:16 *pending*
Plug some memory leaks, on IE. Specifically, there are two related
fixes.
Leak count on tester/gui.html before this change:
Google Gears is leaking memory. Known leaks include 958 objects:
29 DocumentJsRunner
172 JavaScriptWorkerInfo
108 JsEventMonitor
94 ModuleEnvironment
239 ModuleImplBaseClass
239 ModuleWrapper
77 PoolThreadsManager
One is simply the IE analog of 7871060 "Plug leaking DocumentJsRunner
instances, on Firefox" with the 8040339 fix "Make FF's DJR self-
deletion occur later". This ensures that the IE DocumentJsRunner is
deleted (which it previously wasn't) at the later of (1) page unload
and (2) its ModuleEnvironment being deleted. Unlike Firefox, a
DocumentJsRunner on
Leak count on tester/gui.html after part one:
Google Gears is leaking memory. Known leaks include 931 objects:
2 DocumentJsRunner
172 JavaScriptWorkerInfo
108 JsEventMonitor
94 ModuleEnvironment
239 ModuleImplBaseClass
239 ModuleWrapper
77 PoolThreadsManager
In other words, this plugs a leak of 27 DocumentJsRunner instances.
Unlike the Firefox Gears port, deleting a DocumentJsRunner doesn't
trigger further clean-up (such as the js_runner_ff_marshaling.cc
stuff for Firefox), and so the leak count isn't cut as dramatically
as it was when we plugged the Firefox DJR leak.
Two is another leak that was not occuring on the Firefox Gears port.
This was a leak of global objects (i.e. properties of the global
JavaScript object) in worker javascript. Global objects in the main
(i.e. non-worker) thread were being correctly cleaned up. Scoped
objects (i.e. those within curly braces) in worker threads were
being correctly cleaned up. But globals (including the mandatory
GearsFactory and GearsWorkerPool instances) were not being cleaned
up. This is fixed by the new CleanUpJsGlobalVariables function in
js_runner_ie.cc. See the comment in that function for an example
of what we're plugging. The proposed solution to the leak is a bit
of a hacky workaround, so if you have better ideas, please share.
Leak count on tester/gui.html after both parts one and two:
Google Gears is leaking memory. Known leaks include 8 objects:
2 DocumentJsRunner
2 ModuleEnvironment
2 ModuleImplBaseClass
2 ModuleWrapper
In other words, this plugs a leak of 923 instances over the course
of the Gears test suite.
The state of play, after this two-part CL, is that there is only
two leaked objects remaining, on tester/gui.html - a GearsFactory
and a GearsHttpRequest object, and in particular, their ModuleWrapper
instances (the ModuleWrapper holds a reference to the
ModuleImplBaseClass, which holds a reference to its
ModuleEnvironment, which prevents the DocumentJsRunner from self-
deleting).
These leaked objects aren't in the test/testcases (e.g.
database_tests.js), they're in the test harness code itself, which
is a little difficult to debug since it involves things like
re-used IFRAMEs and "kung fu grip" global variables. Nonetheless,
tackling that is next on my list.
R=cprince
[EMAIL PROTECTED]
DELTA=139 (100 added, 12 deleted, 27 changed)
OCL=8151949
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_runner_ie.cc#16
edit
... //depot/googleclient/gears/opensource/gears/base/common/js_types.cc#44 edit
... //depot/googleclient/gears/opensource/gears/base/common/js_types.h#30 edit
139 delta lines: 100 added, 12 deleted, 27 changed
Also consider running:
g4 lint -c 8151949
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 8151949 by [EMAIL PROTECTED] on 2008/09/02 18:55:16 *pending*
Plug some memory leaks, on IE. Specifically, there are two related
fixes.
Leak count on tester/gui.html before this change:
Google Gears is leaking memory. Known leaks include 958 objects:
29 DocumentJsRunner
172 JavaScriptWorkerInfo
108 JsEventMonitor
94 ModuleEnvironment
239 ModuleImplBaseClass
239 ModuleWrapper
77 PoolThreadsManager
One is simply the IE analog of 7871060 "Plug leaking DocumentJsRunner
instances, on Firefox" with the 8040339 fix "Make FF's DJR self-
deletion occur later". This ensures that the IE DocumentJsRunner is
deleted (which it previously wasn't) at the later of (1) page unload
and (2) its ModuleEnvironment being deleted. Unlike Firefox, a
DocumentJsRunner on
Leak count on tester/gui.html after part one:
Google Gears is leaking memory. Known leaks include 931 objects:
2 DocumentJsRunner
172 JavaScriptWorkerInfo
108 JsEventMonitor
94 ModuleEnvironment
239 ModuleImplBaseClass
239 ModuleWrapper
77 PoolThreadsManager
In other words, this plugs a leak of 27 DocumentJsRunner instances.
Unlike the Firefox Gears port, deleting a DocumentJsRunner doesn't
trigger further clean-up (such as the js_runner_ff_marshaling.cc
stuff for Firefox), and so the leak count isn't cut as dramatically
as it was when we plugged the Firefox DJR leak.
Two is another leak that was not occuring on the Firefox Gears port.
This was a leak of global objects (i.e. properties of the global
JavaScript object) in worker javascript. Global objects in the main
(i.e. non-worker) thread were being correctly cleaned up. Scoped
objects (i.e. those within curly braces) in worker threads were
being correctly cleaned up. But globals (including the mandatory
GearsFactory and GearsWorkerPool instances) were not being cleaned
up. This is fixed by the new CleanUpJsGlobalVariables function in
js_runner_ie.cc. See the comment in that function for an example
of what we're plugging. The proposed solution to the leak is a bit
of a hacky workaround, so if you have better ideas, please share.
Leak count on tester/gui.html after both parts one and two:
Google Gears is leaking memory. Known leaks include 8 objects:
2 DocumentJsRunner
2 ModuleEnvironment
2 ModuleImplBaseClass
2 ModuleWrapper
In other words, this plugs a leak of 923 instances over the course
of the Gears test suite.
The state of play, after this two-part CL, is that there is only
two leaked objects remaining, on tester/gui.html - a GearsFactory
and a GearsHttpRequest object, and in particular, their ModuleWrapper
instances (the ModuleWrapper holds a reference to the
ModuleImplBaseClass, which holds a reference to its
ModuleEnvironment, which prevents the DocumentJsRunner from self-
deleting).
These leaked objects aren't in the test/testcases (e.g.
database_tests.js), they're in the test harness code itself, which
is a little difficult to debug since it involves things like
re-used IFRAMEs and "kung fu grip" global variables. Nonetheless,
tackling that is next on my list.
OCL=8151949
Affected files ...
... //depot/googleclient/gears/opensource/gears/base/common/js_runner_ie.cc#16
edit
... //depot/googleclient/gears/opensource/gears/base/common/js_types.cc#44 edit
... //depot/googleclient/gears/opensource/gears/base/common/js_types.h#30 edit
==== //depot/googleclient/gears/opensource/gears/base/common/js_runner_ie.cc#16
-
c:\devel\srcwingears2/googleclient/gears/opensource/gears/base/common/js_runner_ie.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_runner_ie.cc
2008-09-02 12:17:28.000000000 +1000
+++ googleclient/gears/opensource/gears/base/common/js_runner_ie.cc
2008-09-02 18:57:11.000000000 +1000
@@ -67,12 +67,10 @@
public:
JsRunnerBase() {}
- void OnModuleEnvironmentAttach() {
- // TODO(nigeltao): implement on IE, i.e. plug the DocumentJsRunner leak.
- }
-
- void OnModuleEnvironmentDetach() {
- // TODO(nigeltao): implement on IE, i.e. plug the DocumentJsRunner leak.
+ virtual ~JsRunnerBase() {
+ for (int i = 0; i < MAX_JSEVENTS; i++) {
+ assert(0 == event_handlers_[i].size());
+ }
}
JsContextPtr GetContext() {
@@ -380,6 +378,8 @@
error_handler_ = error_handler;
}
+ bool CleanUpJsGlobalVariables();
+
// IActiveScriptSiteImpl overrides
STDMETHOD(LookupNamedItem)(const OLECHAR *name, IUnknown **item);
STDMETHOD(HandleScriptError)(EXCEPINFO *ei, ULONG line, LONG pos, BSTR src);
@@ -547,10 +547,52 @@
return true;
}
+bool JsRunnerImpl::CleanUpJsGlobalVariables() {
+ // NOTE(nigeltao): Yes, this is as hack. Without this method, Gears is
+ // leaking all global JavaScript objects in worker ActiveScript instances.
+ // For example, this worker code:
+ // var foo = google.gears.factory.create('beta.timer');
+ // google.gears.workerPool.onmessage = function(a, b, msg) {
+ // var bar = google.gears.factory.create('beta.timer');
+ // google.gears.workerPool.sendMessage(msg.body, msg.sender);
+ // }
+ // will leak three objects: foo, and the two implicit global objects, viz.
+ // the GearsFactory and the GearsWorkerPool, inserted with the global names
+ // kWorkerInsertedFactoryName and kWorkerInsertedWorkerPoolName. Note that
+ // the object bound to the local variable bar is not leaked.
+ //
+ // To avoid leaking these objects, this code paragraph here first finds the
+ // names of all global variables, and then sets them to (JavaScript) null,
+ // reducing their COM ref-count and hence allowing them to be garbage
+ // collected.
+ //
+ // This solution is less than ideal - the fundamental problem is probably
+ // deeper, such as leaking the ActiveScript instance itself, but after some
+ // effort, I (nigeltao) was insufficiently awesome to pinpoint where our
+ // fundamental leak is, and as a hacky workaround, we instead clean up our
+ // Gears modules (i.e. instances of ModuleImplBaseClass). We may still be
+ // leaking memory upon worker exit, but at least we're not leaking anything
+ // that we're explicitly tracking (via Gears' LEAK_COUNTER_XXX macros).
+ IDispatch *dispatch = GetGlobalObject();
+ std::vector<std::string16> names;
+ HRESULT hr = ActiveXUtils::GetDispatchPropertyNames(dispatch, &names);
+ if (FAILED(hr)) { return false; }
+ std::string16 clean_up_script;
+
+ std::vector<std::string16>::const_iterator it;
+ for (it = names.begin(); it != names.end(); ++it) {
+ clean_up_script += STRING16(L"var ");
+ clean_up_script += *it;
+ clean_up_script += STRING16(L"=null;");
+ }
+ return Eval(clean_up_script);
+}
+
bool JsRunnerImpl::Stop() {
// Check pointer because dtor calls Stop() regardless of whether Start()
// happened.
if (javascript_engine_) {
+ if (!CleanUpJsGlobalVariables()) { return false; }
javascript_engine_->Close();
}
return S_OK;
@@ -633,12 +675,14 @@
class JsRunner : public JsRunnerBase {
public:
JsRunner() {
+ LEAK_COUNTER_INCREMENT(JsRunner);
HRESULT hr = CComObject<JsRunnerImpl>::CreateInstance(&com_obj_);
if (com_obj_) {
com_obj_->AddRef(); // MSDN says call AddRef after CreateInstance
}
}
virtual ~JsRunner() {
+ LEAK_COUNTER_DECREMENT(JsRunner);
// Alert modules that the engine is unloading.
SendEvent(JSEVENT_UNLOAD);
@@ -646,6 +690,14 @@
com_obj_->Stop();
com_obj_->Release();
}
+ }
+
+ 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.
}
IDispatch *GetGlobalObject(bool dump_on_error = false) {
@@ -686,9 +738,25 @@
// common functionality to both workers and the main thread.
class DocumentJsRunner : public JsRunnerBase {
public:
- DocumentJsRunner(IUnknown *site) : site_(site) {}
+ DocumentJsRunner(IUnknown *site)
+ : site_(site),
+ is_module_environment_detached_(true),
+ is_unloaded_(true) {
+ LEAK_COUNTER_INCREMENT(DocumentJsRunner);
+ }
virtual ~DocumentJsRunner() {
+ LEAK_COUNTER_DECREMENT(DocumentJsRunner);
+ assert(is_module_environment_detached_ && is_unloaded_);
+ }
+
+ void OnModuleEnvironmentAttach() {
+ is_module_environment_detached_ = false;
+ }
+
+ void OnModuleEnvironmentDetach() {
+ is_module_environment_detached_ = true;
+ DeleteIfNoLongerRelevant();
}
IDispatch *GetGlobalObject(bool dump_on_error = false) {
@@ -789,27 +857,22 @@
assert(false); // Should not be called on the DocumentJsRunner.
}
- bool AddEventHandler(JsEventType event_type,
- JsEventHandlerInterface *handler) {
- if (event_type == JSEVENT_UNLOAD) {
+ bool ListenForUnloadEvent() {
#ifdef WINCE
- // WinCE does not use HtmlEventMonitor to monitor page unloading.
+ // WinCE does not use HtmlEventMonitor to monitor page unloading.
#else
- // Create an HTML event monitor to send the unload event when the page
- // goes away.
- if (unload_monitor_ == NULL) {
- unload_monitor_.reset(new HtmlEventMonitor(kEventUnload,
- HandleEventUnload, this));
- CComPtr<IHTMLWindow3> event_source;
- if (FAILED(ActiveXUtils::GetHtmlWindow3(site_, &event_source))) {
- return false;
- }
- unload_monitor_->Start(event_source);
- }
+ // Create an HTML event monitor to send the unload event when the page
+ // goes away.
+ unload_monitor_.reset(new HtmlEventMonitor(kEventUnload,
+ HandleEventUnload, this));
+ CComPtr<IHTMLWindow3> event_source;
+ if (FAILED(ActiveXUtils::GetHtmlWindow3(site_, &event_source))) {
+ return false;
+ }
+ unload_monitor_->Start(event_source);
+ is_unloaded_ = false;
#endif
- }
-
- return JsRunnerBase::AddEventHandler(event_type, handler);
+ return true;
}
bool InvokeCallback(const JsRootedCallback *callback,
@@ -844,11 +907,17 @@
private:
static void HandleEventUnload(void *user_param) {
- // Callback for 'onunload'
- static_cast<DocumentJsRunner*>(user_param)->SendEvent(JSEVENT_UNLOAD);
- // TODO(nigeltao): Are we leaking the DocumentJsRunner? This might be a
- // good time to call
- // delete static_cast<DocumentJsRunner*>(user_param);
+ DocumentJsRunner *document_js_runner =
+ static_cast<DocumentJsRunner*>(user_param);
+ document_js_runner->SendEvent(JSEVENT_UNLOAD);
+ document_js_runner->is_unloaded_ = true;
+ document_js_runner->DeleteIfNoLongerRelevant();
+ }
+
+ void DeleteIfNoLongerRelevant() {
+ if (is_module_environment_detached_ && is_unloaded_) {
+ delete this;
+ }
}
#ifdef WINCE
@@ -857,6 +926,8 @@
scoped_ptr<HtmlEventMonitor> unload_monitor_; // For 'onunload'
notifications
#endif
CComPtr<IUnknown> site_;
+ bool is_module_environment_detached_;
+ bool is_unloaded_;
DISALLOW_EVIL_CONSTRUCTORS(DocumentJsRunner);
};
@@ -867,5 +938,12 @@
}
JsRunnerInterface* NewDocumentJsRunner(IUnknown *base, JsContextPtr) {
- return static_cast<JsRunnerInterface*>(new DocumentJsRunner(base));
-}
+ scoped_ptr<DocumentJsRunner> document_js_runner(
+ new DocumentJsRunner(base));
+ if (!document_js_runner->ListenForUnloadEvent()) {
+ return NULL;
+ }
+ // A DocumentJsRunner who has had ListenForUnloadEvent called successfully
+ // is self-deleting, so we can release it from our scoped_ptr.
+ return document_js_runner.release();
+}
==== //depot/googleclient/gears/opensource/gears/base/common/js_types.cc#44 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/base/common/js_types.cc
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_types.cc 2008-09-02
12:17:28.000000000 +1000
+++ googleclient/gears/opensource/gears/base/common/js_types.cc 2008-09-01
12:22:46.000000000 +1000
@@ -2045,6 +2045,13 @@
return true;
}
+JsCallContext::JsCallContext(DISPPARAMS FAR *disp_params, VARIANT FAR *retval,
+ EXCEPINFO FAR *excep_info)
+ : disp_params_(disp_params), retval_(retval), exception_info_(excep_info),
+ is_exception_set_(false), is_return_value_set_(false) {
+ LEAK_COUNTER_INCREMENT(JsCallContext);
+}
+
void JsCallContext::SetReturnValue(JsParamType type, const void *value_ptr) {
// There is only a valid retval_ if the javascript caller is expecting a
// return value.
@@ -2158,6 +2165,14 @@
assert(false);
}
return true;
+}
+
+JsCallContext::JsCallContext(JsContextPtr js_context, NPObject *object,
+ int argc, const JsToken *argv, JsToken *retval)
+ : js_context_(js_context), is_exception_set_(false),
+ is_return_value_set_(false), object_(object),
+ argc_(argc), argv_(argv), retval_(retval) {
+ LEAK_COUNTER_INCREMENT(JsCallContext);
}
void JsCallContext::SetReturnValue(JsParamType type, const void *value_ptr) {
==== //depot/googleclient/gears/opensource/gears/base/common/js_types.h#30 -
c:\devel\srcwingears2/googleclient/gears/opensource/gears/base/common/js_types.h
====
# action=edit type=text
--- googleclient/gears/opensource/gears/base/common/js_types.h 2008-09-02
12:17:28.000000000 +1000
+++ googleclient/gears/opensource/gears/base/common/js_types.h 2008-09-01
12:22:58.000000000 +1000
@@ -516,15 +516,10 @@
// Only browser-specific wrapper code should need to instantiate this object.
#if BROWSER_NPAPI
JsCallContext(JsContextPtr js_context, NPObject *object,
- int argc, const JsToken *argv, JsToken *retval)
- : js_context_(js_context), is_exception_set_(false),
- is_return_value_set_(false), object_(object),
- argc_(argc), argv_(argv), retval_(retval) {}
+ int argc, const JsToken *argv, JsToken *retval);
#elif BROWSER_IE
JsCallContext(DISPPARAMS FAR *disp_params, VARIANT FAR *retval,
- EXCEPINFO FAR *excep_info)
- : disp_params_(disp_params), retval_(retval),
exception_info_(excep_info),
- is_exception_set_(false), is_return_value_set_(false) {}
+ EXCEPINFO FAR *excep_info);
#elif BROWSER_FF
JsCallContext(JsContextPtr cx, JsRunnerInterface *js_runner,
int argc, JsToken *argv, JsToken *retval);
