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<javascript:>
> > 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) << "...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<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+unsubscribe@**
>>>> googlegroups.com.
>>>> For more options, visit
>>>> https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/opt_out>
>>>> .
>>>>
>>>
>>> --
>> 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<javascript:>
>> .
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>
>
--
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.
