Re: [gwt-contrib] Possible firefox leak fix
Wow, colin you're on fire recently with your contributions. ;-)
On Tue, Sep 24, 2013 at 6:48 PM, Colin Alworth niloc...@gmail.com wrote:
Done, change is at https://gwt-review.googlesource.com/4680.
Concern about breaking WindowImpl#initWindowCloseHandler was my guess as
well - after connect() is called and the app starts up, the app has already
wired up its own close handler, so it is too late when connect is
successful to replace unload.
Both the standard WindowImpl and the IE specific one replace onunload and
call the old version, so provided those stay consistent with
hosted.html/devmode.js, this approach should be okay...
I'd love to get any independent verification on this as even being a
viable fix - I've only tried it in FF24/mac so far.
-Colin
On Tuesday, September 24, 2013 8:02:54 PM UTC-5, Brian Slesinsky wrote:
+cromwellian since he did unload support.
Oops, I see now that you attached it. Could you upload it to Gerrit?
I looked pretty hard for a reason why it's window.onUnload and not
window.onload and I can't find any. It dates back to 2010 at least and was
copied from hosted.html, whether deliberately I don't know.
One issue is that it's possible the app itself might want an onunload
handler. See
user/src/com/google/gwt/user/**client/impl/WindowImpl.java line 32 where
it can install an onunload handler. It does preserve the old one but it
still seems kind of fragile to install one in the devmode.js. I wonder if
there's some way to detect the unload event in C++?
- Brian
On Tue, Sep 24, 2013 at 5:25 PM, Colin Alworth nilo...@gmail.com wrote:
The patch I provided only tweaks the normal source, and leaves the
generated sources and binaries alone - I think that is already what you are
after.
Note that this does *not* stop the leak by itself - we need to decide
what to do about hosted.html/devmode.js. One (ugly) option is to try to
have the ff plugin add an event handler to the window's close to call back
to its own disconnect? I think there are other ways we could think about
doing this as well, but changing hosted.html keeps it consistent across the
board.
On Tuesday, September 24, 2013 6:50:07 PM UTC-5, Brian Slesinsky wrote:
Hi,
Wow, thanks for tracking this down! Could you send a patch that just
modifies the source (not worrying about the autogenerated files)? I will
rebuild them when I do the next release.
Usually I just leave the existing dll's alone and move forward; it's
not worth fixing older plugins. However, I believe Firefox 24 will be an
ESR release so I think it's worth rebuilding that version.
On Tue, Sep 24, 2013 at 4:31 PM, Colin Alworth nilo...@gmail.comwrote:
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.**reactToMe**ssages(). 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...
[gwt-contrib] Possible firefox leak fix
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
Re: [gwt-contrib] Possible firefox leak fix
The patch I provided only tweaks the normal source, and leaves the
generated sources and binaries alone - I think that is already what you are
after.
Note that this does *not* stop the leak by itself - we need to decide what
to do about hosted.html/devmode.js. One (ugly) option is to try to have the
ff plugin add an event handler to the window's close to call back to its
own disconnect? I think there are other ways we could think about doing
this as well, but changing hosted.html keeps it consistent across the board.
On Tuesday, September 24, 2013 6:50:07 PM UTC-5, Brian Slesinsky wrote:
Hi,
Wow, thanks for tracking this down! Could you send a patch that just
modifies the source (not worrying about the autogenerated files)? I will
rebuild them when I do the next release.
Usually I just leave the existing dll's alone and move forward; it's not
worth fixing older plugins. However, I believe Firefox 24 will be an ESR
release so I think it's worth rebuilding that version.
On Tue, Sep 24, 2013 at 4:31 PM, Colin Alworth nilo...@gmail.comjavascript:
wrote:
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() {
Re: [gwt-contrib] Possible firefox leak fix
+cromwellian since he did unload support.
Oops, I see now that you attached it. Could you upload it to Gerrit?
I looked pretty hard for a reason why it's window.onUnload and not
window.onload and I can't find any. It dates back to 2010 at least and was
copied from hosted.html, whether deliberately I don't know.
One issue is that it's possible the app itself might want an onunload
handler. See
user/src/com/google/gwt/user/client/impl/WindowImpl.java line 32 where it
can install an onunload handler. It does preserve the old one but it still
seems kind of fragile to install one in the devmode.js. I wonder if there's
some way to detect the unload event in C++?
- Brian
On Tue, Sep 24, 2013 at 5:25 PM, Colin Alworth niloc...@gmail.com wrote:
The patch I provided only tweaks the normal source, and leaves the
generated sources and binaries alone - I think that is already what you are
after.
Note that this does *not* stop the leak by itself - we need to decide what
to do about hosted.html/devmode.js. One (ugly) option is to try to have the
ff plugin add an event handler to the window's close to call back to its
own disconnect? I think there are other ways we could think about doing
this as well, but changing hosted.html keeps it consistent across the board.
On Tuesday, September 24, 2013 6:50:07 PM UTC-5, Brian Slesinsky wrote:
Hi,
Wow, thanks for tracking this down! Could you send a patch that just
modifies the source (not worrying about the autogenerated files)? I will
rebuild them when I do the next release.
Usually I just leave the existing dll's alone and move forward; it's not
worth fixing older plugins. However, I believe Firefox 24 will be an ESR
release so I think it's worth rebuilding that version.
On Tue, Sep 24, 2013 at 4:31 PM, Colin Alworth nilo...@gmail.com wrote:
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)
Re: [gwt-contrib] Possible firefox leak fix
Done, change is at https://gwt-review.googlesource.com/4680.
Concern about breaking WindowImpl#initWindowCloseHandler was my guess as
well - after connect() is called and the app starts up, the app has already
wired up its own close handler, so it is too late when connect is
successful to replace unload.
Both the standard WindowImpl and the IE specific one replace onunload and
call the old version, so provided those stay consistent with
hosted.html/devmode.js, this approach should be okay...
I'd love to get any independent verification on this as even being a viable
fix - I've only tried it in FF24/mac so far.
-Colin
On Tuesday, September 24, 2013 8:02:54 PM UTC-5, Brian Slesinsky wrote:
+cromwellian since he did unload support.
Oops, I see now that you attached it. Could you upload it to Gerrit?
I looked pretty hard for a reason why it's window.onUnload and not
window.onload and I can't find any. It dates back to 2010 at least and was
copied from hosted.html, whether deliberately I don't know.
One issue is that it's possible the app itself might want an onunload
handler. See
user/src/com/google/gwt/user/client/impl/WindowImpl.java line 32 where it
can install an onunload handler. It does preserve the old one but it still
seems kind of fragile to install one in the devmode.js. I wonder if there's
some way to detect the unload event in C++?
- Brian
On Tue, Sep 24, 2013 at 5:25 PM, Colin Alworth nilo...@gmail.comjavascript:
wrote:
The patch I provided only tweaks the normal source, and leaves the
generated sources and binaries alone - I think that is already what you are
after.
Note that this does *not* stop the leak by itself - we need to decide
what to do about hosted.html/devmode.js. One (ugly) option is to try to
have the ff plugin add an event handler to the window's close to call back
to its own disconnect? I think there are other ways we could think about
doing this as well, but changing hosted.html keeps it consistent across the
board.
On Tuesday, September 24, 2013 6:50:07 PM UTC-5, Brian Slesinsky wrote:
Hi,
Wow, thanks for tracking this down! Could you send a patch that just
modifies the source (not worrying about the autogenerated files)? I will
rebuild them when I do the next release.
Usually I just leave the existing dll's alone and move forward; it's not
worth fixing older plugins. However, I believe Firefox 24 will be an ESR
release so I think it's worth rebuilding that version.
On Tue, Sep 24, 2013 at 4:31 PM, Colin Alworth nilo...@gmail.comwrote:
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
