[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
LGTM http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
LGTM, but see comment below. http://gwt-code-reviews.appspot.com/1601803/diff/9/user/src/com/google/gwt/user/client/ui/ScrollImpl.java File user/src/com/google/gwt/user/client/ui/ScrollImpl.java (right): http://gwt-code-reviews.appspot.com/1601803/diff/9/user/src/com/google/gwt/user/client/ui/ScrollImpl.java#newcode73 user/src/com/google/gwt/user/client/ui/ScrollImpl.java:73: // initialize static, leak-safe scroll/resize handlers Correct me if I'm wrong but there's chance it could leak if the compiler inlines initHandlers here. Calling it from the constructor or static initializer we'd be assured it's not inlined (AFAICT) so that there won't be any element in-scope to trigger a leak. Static initializer would introduce a clinit so it's not a good idea IMO. So either a constructor, or an explicit init() call from ScrollImpl.get() below (with a no-op method in ScrollImpl for the non-IE case) http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
Correct me if I'm wrong but there's chance it could leak if the compiler inlines initHandlers here. Tricky! That seems like it could happen; I don't know for sure, but I think it makes more sense to init the handlers in the cstr anyway. Updated the patch accordingly. http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
On 2011/12/06 06:11:56, stephenh wrote: This patch uses Thomas's suggestion of static methods with srcElement to avoid holding on to a scrollableElem that comes from outside the event handler's closure. http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
Adding Joel as a reviewer, as IE leaks are his Moby Dick (as he says). http://gwt-code-reviews.appspot.com/1601803/diff/1/user/src/com/google/gwt/user/client/ui/ScrollImpl.java File user/src/com/google/gwt/user/client/ui/ScrollImpl.java (right): http://gwt-code-reviews.appspot.com/1601803/diff/1/user/src/com/google/gwt/user/client/ui/ScrollImpl.java#newcode56 user/src/com/google/gwt/user/client/ui/ScrollImpl.java:56: scrollable.__scrollHandler = scrollHandler; On 2011/11/27 01:06:33, stephenh wrote: On 2011/11/26 10:36:07, tbroyer wrote: How about creating static functions (have a look into com.google.gwt.user.client.impl.DOMImplTrident) I don't see any static methods in DOMImplTrident? I meant the callDispatchEvent et al. that use window.event.srcElement to get a reference to the scrollableElem? That way it wouldn't leak and wouldn't need to be uninstalled. That seems cool. For the container.onresize, one solution could be to set the scrollableElem as an expando; e.g. I think Dirk's other leak (issue 1601804 in reitveld) was caused by a parent DOM element having an expando property to one of its children. (AFAIK, would appreciate your comments on that review if I'm wrong.) It seems like you're right: http://msdn.microsoft.com/en-us/library/bb250448(v=vs.85).aspx In this case scrollableElem is the parent, so perhaps a child (container) having an expando to its parent would be okay? I'm afraid it'd leak too (according to the MSDN). A quick and dirty fix would be to set an expando on the 'container' just saying hey, I'm the 'container', the 'scrollable' is my parent, and then using: if (scrollableElem.__container) scrollableElem = scrollableElem.parent; Otherwise, I'm afraid an onAttach/onDetach is necessary; but using the technique I proposed and just setting the expando back to 'null' in onDetach would then be more readable I guess. http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
http://gwt-code-reviews.appspot.com/1601803/diff/1/user/src/com/google/gwt/user/client/ui/ScrollImpl.java File user/src/com/google/gwt/user/client/ui/ScrollImpl.java (right): http://gwt-code-reviews.appspot.com/1601803/diff/1/user/src/com/google/gwt/user/client/ui/ScrollImpl.java#newcode56 user/src/com/google/gwt/user/client/ui/ScrollImpl.java:56: scrollable.__scrollHandler = scrollHandler; How about creating static functions (have a look into com.google.gwt.user.client.impl.DOMImplTrident) that use window.event.srcElement to get a reference to the scrollableElem? That way it wouldn't leak and wouldn't need to be uninstalled. For the container.onresize, one solution could be to set the scrollableElem as an expando; e.g. container.__scrollable = scrollable; And thus use the following from within the resize handler: var scrollableElem = window.event.srcElement; scrollableElem = scrollableElem.__scrollable || scrollableElem; (the event's target can be either the scrollable or the container; the __scrollable expando references the scrollable from the container) http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Fix for ScrollImplTrident leak (issue1601803)
My local commit message didn't make it into the issue description, but this patch should include in the commit message attribution to Dirk Scheffler. If also fixes issue 7015: http://code.google.com/p/google-web-toolkit/issues/detail?id=7015 http://gwt-code-reviews.appspot.com/1601803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
