I spent a little time this weekend learning how to build firefox plugins,
and a little time spilled out into this week, but I think I'm at a point
where I can share what I've done, what I'm seeing, and start asking for
help to finalize this (if it is as meaningful as I hope it is).
First, the issue itself: It looks like we're actually leaking on both ends
- that is, not just in the JVM, but in the browser itself. When Dev Mode
transfers control back to the browser itself, it runs
BrowserChannelServer.reactToMessages(). This listens at the open socket
between the browser's plugin and itself, and waits for the next byte to
float over the wire, meaning that the browser wants something. This waiting
specifically happens in Message.readMessageType, where we block on
stream.readByte(). Once a tab/window has been closed, the thread that is
actively watching that connection stays stuck in readByte(), waiting for
that next message to float over the wire.
My first thought was "why can't we just ask if that socket is closed?" -
well, the socket *isn't* closed, which means that the browser is leaking
the socket itself, along with whatever supporting objects were set up to
manage that socket. Note that this suggests a workaround to the leak - quit
and relaunch firefox, and since the socket will be forcibly closed then,
readByte() will throw a EOF exception, and reactToMessages will trip off a
RemoteDeathError (not ideal, but at least it just logs it and moves on,
finishing the leak off).
Next I dug into how to make the plugin actually disconnect when the page
was closed. I started this by finding exactly where the socket was opened
(common/HostChannel.{h|cpp}), then what creates HostChannel objects in the
firefox plugin. This turns out to be achieved in this line in
xpcom/ExternalWrapper.cpp (plus instructive comments):
// TODO(jat): leaks HostChannel -- need to save it in a session object and
// return that so the host page can call a disconnect method on it at
unload
// time or when it gets GC'd.
HostChannel* channel = new HostChannel();
Okay, so at the time it was written, it was known that this leaks the
channel. This is where I started losing confidence, as it doesnt look like
it could be this easy... The next block actually opens the connection, and
passes it off to a FFSessionHandler instance, which is stored away in a
field:
Debug::log(Debug::Debugging) << "Connecting..." << Debug::flush;
if (!channel->connectToHost(hostPart.c_str(),
atoi(portPart.c_str()))) {
*_retval = false;
return NS_OK;
}
Debug::log(Debug::Debugging) << "...Connected" << Debug::flush;
sessionHandler.reset(new FFSessionHandler(channel/*, ctx*/));
All this code is part of ExternalWrapper::Connect (note, defined twice for
varying ff versions), and since Connect is never invoked internally, it was
my assumption that I could build a ExternalWrapper::Disconnect that simply
unwound this channel:
NS_IMETHODIMP ExternalWrapper::Disconnect() {
Debug::log(Debug::Info) << "Disconnecting..." << Debug::flush;
sessionHandler.get()->disconnect();
Debug::log(Debug::Info) << "...Disconnected" << Debug::flush;
return NS_OK;
}
That, combined with two other changes seems to expose this new function to
be called from in the browser and actually triggers the QUIT case in
reactToMessages - First, declare this new method in IOOPHM.idl, and second
tweak ExternalWrapper's CanCallMethod check to allow invoking disconnect
from within the browser. Actually building with the attached patch then
modifies several other files (more on that later).
Next, how to make disconnect() be invoked from within the browser - it is
already declared and invoked from within hosted.html and devmode.js, but it
seems to be in an invalid window callback:
window.onUnload = function() {
try {
// wrap in try/catch since plugins are not required to supply this
plugin.disconnect();
} catch (e) {
}
};
As far as I can tell, there is no such onUnload, but should be onunload
instead. Indeed, onunload is defined later in each file, but since it just
assigns an empty function, I suspect that one or the other is a typo:
window.onunload = function() {
};
To continue my exploration, I kept onUnload's name as is, and just called
it from the onunload declaration - this avoids changing the name of any
existing calls (in case there is actually a good reason for labelling it
so) and should prevent any possible issues from clobbering
com.google.gwt.user.client.impl.WindowImpl#initWindowCloseHandler by
re-overriding onunload after initWindowCloseHandler has already replaced it
(and kept a reference to the original):
window.onunload = function() {
window.onUnload && window.onUnload();
};
With that change to hosted.html (or a copy in my project's public/ dir) and
the attached patch, the leak seems to be resolved. Questions I have:
* How does one perform the complete plugin build process? How far back in
time do we rebuild and test?
* How many auto-generated files (mostly .h from the idl, but also the
.dll/.dylib/etc) do we commit when such a change is made?
* Apparently plugins don't need to provide a disconnect() method, but if
any do, are we *sure* they are actually being called as expected? Or is
onUnload a long-lived typo?
Looking forward to discussing this here, in ##gwt on freenode, or on the
call tomorrow,
Colin
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups "GWT
Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to google-web-toolkit-contributors+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
commit 6969765b5b60731f12ce529bbc6f730922b7e817
Author: Colin Alworth <niloc...@gmail.com>
Date: Tue Sep 24 18:29:13 2013 -0500
Initial work on supporting a disconnect message from the browser to close off the connection to the code server
diff --git a/xpcom/ExternalWrapper.cpp b/xpcom/ExternalWrapper.cpp
index ab0423e..f4d61cb 100644
--- a/xpcom/ExternalWrapper.cpp
+++ b/xpcom/ExternalWrapper.cpp
@@ -399,6 +399,13 @@ NS_IMETHODIMP ExternalWrapper::Connect(const nsACString& suppliedUrl,
return NS_OK;
}
+NS_IMETHODIMP ExternalWrapper::Disconnect() {
+ Debug::log(Debug::Info) << "Disconnecting..." << Debug::flush;
+ sessionHandler.get()->disconnect();
+ Debug::log(Debug::Info) << "...Disconnected" << Debug::flush;
+ return NS_OK;
+}
+
// nsISecurityCheckedComponent
static char* cloneAllAccess() {
static const char allAccess[] = "allAccess";
@@ -422,7 +429,7 @@ NS_IMETHODIMP ExternalWrapper::CanCreateWrapper(const nsIID * iid,
NS_IMETHODIMP ExternalWrapper::CanCallMethod(const nsIID * iid,
const PRUnichar *methodName, char **_retval) {
Debug::log(Debug::Spam) << "ExternalWrapper::CanCallMethod" << Debug::flush;
- if (strEquals(methodName, "connect") || strEquals(methodName, "init")) {
+ if (strEquals(methodName, "connect") || strEquals(methodName, "init") || strEquals(methodName, "disconnect")) {
*_retval = cloneAllAccess();
} else {
*_retval = nullptr;
diff --git a/xpcom/IOOPHM.idl b/xpcom/IOOPHM.idl
index 1aeea54..14e4d6f 100644
--- a/xpcom/IOOPHM.idl
+++ b/xpcom/IOOPHM.idl
@@ -8,4 +8,5 @@ interface IOOPHM : nsISupports
boolean init(in nsIDOMWindow window);
boolean connect(in ACString url, in ACString sessionKey, in ACString addr,
in ACString moduleName, in ACString hostedHtmlVersion);
+ void disconnect();
};
