Last Comment Bug 835011 - Restartless Restart 8 leaks chrome memory when windows are closed
: Restartless Restart 8 leaks chrome memory when windows are closed
Status: RESOLVED FIXED
[MemShrink]
:
Product: Tech Evangelism
Classification: Other
Component: Add-ons (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Erik Vold [:erikvold] (please needinfo? me)
:
Mentors:
https://github.com/voldsoftware/resta...
Depends on:
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2013-01-25 22:36 PST by Michael Kraft [:morac]
Modified: 2013-02-06 03:41 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Actually call the unload callback function when window unloads (1.08 KB, patch)
2013-01-28 20:27 PST, Michael Kraft [:morac]
no flags Details | Diff | Review

Description Michael Kraft [:morac] 2013-01-25 22:36:55 PST
With Restartless Restart 8 installed and enabled, objects added to chrome windows persist after the windows closed ending up in "top(none)/detached/window([system])".  No ghost windows are created, only the chrome window compartment leaks.

The reason for this is that Restartless Restart pushes a function from the window callback into a bootstrap.js global scope array variable.  The variable is NULLed when the add-on is disabled or uninstalled, but not when windows are closed.  As such the memory will continue to leak until Restartless Restart is disabled.

The fix for this is simple.  Clear out the window's array entry when it is closed by adding an "unload" event listener to the window in main(win) as such:

  function removePrefChgHandler(aEvt) { 
    prefChgHandlers[prefChgHandlerIndex] = null;
  }

  win.addEventListener("unload",  removePrefChgHandler, false);


then in the unload (disable/uninstall) function call, function simply add:

  win.removeEventListener("unload",  removePrefChgHandler);



With this added, chrome windows no longer leak.

I've tried going to the support site listed on the add-on page (https://addons.mozilla.org/en-us/firefox/addon/restartless-restart/), but the link is bad.  It appears the issues area has been removed.  I sent an email to the author, but don't know if he is working on this anymore.
Comment 1 Jorge Villalobos [:jorgev] 2013-01-28 07:21:46 PST
This could be related to bug 743414 and bug 743215.
Comment 2 Michael Kraft [:morac] 2013-01-28 07:57:16 PST
(In reply to Jorge Villalobos [:jorgev] from comment #1)
> This could be related to bug 743414 and bug 743215.

Except those are listed as fixed.

As I mentioned in comment #0, the problem in this case is being caused by storing a function (i.e object) created by a callback from a chrome window in the bootstrap global variable (prefChgHandlers), thereby causing a dependence from the Chrome window on that variable.  That keeps the Chrome window from being garbage collected until the prefChgHandlers variable is cleared out, which only happens when the add-on is disabled or uninstalled.
Comment 3 Erik Vold [:erikvold] (please needinfo? me) 2013-01-28 19:11:53 PST
(In reply to Michael Kraft [:morac] from comment #0)
> With Restartless Restart 8 installed and enabled, objects added to chrome
> windows persist after the windows closed ending up in
> "top(none)/detached/window([system])".  No ghost windows are created, only
> the chrome window compartment leaks.
> 
> The reason for this is that Restartless Restart pushes a function from the
> window callback into a bootstrap.js global scope array variable.  The
> variable is NULLed when the add-on is disabled or uninstalled, but not when
> windows are closed.  As such the memory will continue to leak until
> Restartless Restart is disabled.
> 
> The fix for this is simple.  Clear out the window's array entry when it is
> closed by adding an "unload" event listener to the window in main(win) as
> such:
> 
>   function removePrefChgHandler(aEvt) { 
>     prefChgHandlers[prefChgHandlerIndex] = null;
>   }
> 
>   win.addEventListener("unload",  removePrefChgHandler, false);
> 
> 
> then in the unload (disable/uninstall) function call, function simply add:
> 
>   win.removeEventListener("unload",  removePrefChgHandler);

I think this would still leak because you'd also have to remove the latter reference to `win` in the window's unload event.

It's weird that this line https://github.com/voldsoftware/restartless-restart-ffext/blob/master/src/bootstrap.js#L264 doesn't resolve the issue, at first glance, I'll have to investigate more soon..
Comment 4 Michael Kraft [:morac] 2013-01-28 19:27:21 PST
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #3)
> I think this would still leak because you'd also have to remove the latter
> reference to `win` in the window's unload event.

When I tested it by creating a leak test addon which basically allocates 50 MB of string space in a window, the string was garbage collected when I made the changes I mentioned.  

I believe the problem isn't that there are references to the bootstrap in the window, but that there are references to the window in the bootstrap (specifically prefChgHandlers).  Actually I'm not sure there are even references to bootstrap as I believe the main function (and hence the unload function) appear to execute somehow in the chrome window compartment.   It's possible moving prefChgHandlers inside the main function, might also solve this problem, though I didn't test that and I'm not sure how that would work with multiple chrome windows referencing the same object. 
> 
> It's weird that this line
> https://github.com/voldsoftware/restartless-restart-ffext/blob/master/src/
> bootstrap.js#L264 doesn't resolve the issue, at first glance, I'll have to
> investigate more soon..

That's because the unload function only runs when the addon is disabled or uninstalled.  It doesn't run when the window unloads since there is no event listener for window unloads.   That line is the reason that disabling or uninstalling the addon immediately results in all the leaking windows garbage collecting.
Comment 5 Erik Vold [:erikvold] (please needinfo? me) 2013-01-28 19:33:18 PST
(In reply to Michael Kraft [:morac] from comment #4)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #3)
> > I think this would still leak because you'd also have to remove the latter
> > reference to `win` in the window's unload event.
> 
> When I tested it by creating a leak test addon which basically allocates 50
> MB of string space in a window, the string was garbage collected when I made
> the changes I mentioned.  
> 
> I believe the problem isn't that there are references to the bootstrap in
> the window, but that there are references to the window in the bootstrap
> (specifically prefChgHandlers).  Actually I'm not sure there are even
> references to bootstrap as I believe the main function (and hence the unload
> function) appear to execute somehow in the chrome window compartment.   It's
> possible moving prefChgHandlers inside the main function, might also solve
> this problem, though I didn't test that and I'm not sure how that would work
> with multiple chrome windows referencing the same object. 

Could you provide the patch?  I don't see how the example in comment 1 would work.

> > It's weird that this line
> > https://github.com/voldsoftware/restartless-restart-ffext/blob/master/src/
> > bootstrap.js#L264 doesn't resolve the issue, at first glance, I'll have to
> > investigate more soon..
> 
> That's because the unload function only runs when the addon is disabled or
> uninstalled.  

No that isn't right, the unload function runs on window unload if a window is passed as the 2nd argument.

> It doesn't run when the window unloads since there is no event
> listener for window unloads.   That line is the reason that disabling or
> uninstalling the addon immediately results in all the leaking windows
> garbage collecting.

The window unload listener is here: https://github.com/voldsoftware/restartless-restart-ffext/blob/master/src/packages/unload.js#L70
Comment 6 Michael Kraft [:morac] 2013-01-28 20:17:27 PST
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> > That's because the unload function only runs when the addon is disabled or
> > uninstalled.  
> 
> No that isn't right, the unload function runs on window unload if a window
> is passed as the 2nd argument.
> 

The unload event runs, but it doesn't call the unload callback.

I just double checked the code in unload.js and it's not actually calling the unloader() function.  Line 70 adds a callback to removeUnloader, not to the unloader or callback function.  So when the window unloads, the unloader callback function is removed from unloaders, but not executed.

See https://github.com/voldsoftware/restartless-restart-ffext/blob/master/src/packages/unload.js#L90

The reason the unload() function called on the disable/uninstall works, because the unload() function actually calls all the callback functions.

See https://github.com/voldsoftware/restartless-restart-ffext/blob/master/src/packages/unload.js#L62


So an actually better fix, would be to change the unload.js file to actually call the callback function when the window unloads instead of simply removing it.  I'll see if I can come up with something really quick, unless you want to.
Comment 7 Erik Vold [:erikvold] (please needinfo? me) 2013-01-28 20:27:33 PST
(In reply to Michael Kraft [:morac] from comment #6)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> > > That's because the unload function only runs when the addon is disabled or
> > > uninstalled.  
> > 
> > No that isn't right, the unload function runs on window unload if a window
> > is passed as the 2nd argument.
> > 
> 
> The unload event runs, but it doesn't call the unload callback.
> 
> I just double checked the code in unload.js and it's not actually calling
> the unloader() function.  Line 70 adds a callback to removeUnloader, not to
> the unloader or callback function.  So when the window unloads, the unloader
> callback function is removed from unloaders, but not executed.
> 
> See
> https://github.com/voldsoftware/restartless-restart-ffext/blob/master/src/
> packages/unload.js#L90
> 
> The reason the unload() function called on the disable/uninstall works,
> because the unload() function actually calls all the callback functions.
> 
> See
> https://github.com/voldsoftware/restartless-restart-ffext/blob/master/src/
> packages/unload.js#L62
> 
> 
> So an actually better fix, would be to change the unload.js file to actually
> call the callback function when the window unloads instead of simply
> removing it.  I'll see if I can come up with something really quick, unless
> you want to.

Ah that makes sense!

I'd love to give you credit if you can put a pull request together, I know what to do now though so I'll leave that up to you.
Comment 8 Michael Kraft [:morac] 2013-01-28 20:27:57 PST
Created attachment 707425 [details] [diff] [review]
Actually call the unload callback function when window unloads

I wrote up a quick patch which appears to work.
Comment 9 Michael Kraft [:morac] 2013-01-28 20:29:24 PST
Missed it by 25 seconds. :)

I'm not sure how exactly to do a pull request (not familiar with git), but I put together a "diff" patch (see comment #8) will that work?
Comment 10 Erik Vold [:erikvold] (please needinfo? me) 2013-01-28 20:32:03 PST
yep thanks!  I'll upload a new version tonight.
Comment 11 Michael Kraft [:morac] 2013-01-28 20:36:38 PST
I think I figured out how to do a pull request so if you want to use that you can.  Username is "Morac2".
Comment 12 Erik Vold [:erikvold] (please needinfo? me) 2013-01-28 20:52:52 PST
Cool a pull request would be easier on me.
Comment 13 Michael Kraft [:morac] 2013-01-28 21:35:22 PST
I'll mark this fixed since I see you posted version 9 to AMO. 

https://addons.mozilla.org/en-us/firefox/addon/restartless-restart/versions/9
Comment 14 Szabolcs Hubai (:xabolcs) 2013-01-29 03:25:12 PST
Sorry for bugspam, but wanted to note two things.

This was came up more than a year ago. See issue #47! [1]
It was anno closed as WONTFIX.


Secondly.
Regarding to the reasons [2], and to :Mardak's explanation below:
"The unload module provides a way to clean up modifications to the browser when the add-on is being disabled or uninstalled. It wasn't made to run code when windows are being closed. The container was added as a convenience to free up references to windows that have been closed."

RR should use listen instead (or other functions) instead abusing unloader,
shouldn't it?

What about filing a follow-up bug and revisit the listen and unload usages of utils.js ... starting with mozilla/prospector [3]?



[1]: https://github.com/voldsoftware/restartless-restart-ffext/issues/47
[2]: https://github.com/voldsoftware/restartless-restart-ffext/issues/47#issuecomment-1951346
[3]: https://github.com/mozilla/prospector
Comment 15 Michael Kraft [:morac] 2013-01-29 07:46:17 PST
Well on one hand, having the unloader function called on a container unload, guarantees that the code using the unload.js won't accidentally leak objects.  It puts the emphasis on cleanup on the unload.js library instead of the user of the library.

On the other hand, it can slightly decrease performance by cleaning up objects that don't really need to be cleaned up since the garbage collection (GC) routine would already clean them up.  Though it would make it easier for the GC.

So I'm kind of split. Considering though that RR had this leak in it for 9 months and no one really noticed despite having 37,000 plus users, I'd say I'd lean toward "safe" code over performance.

Though if there is a bootstrap addon that registers a lot of containers with unload, this could slow things down.   I'd recommend filing a bug over at https://github.com/voldsoftware/restartless-restart-ffext/issues if you think it should be changed.

Note You need to log in before you can comment on or make changes to this bug.