Michael Kraft [:morac] Reporter
	Description • 11 years ago

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.

	Jorge Villalobos [:jorgev] (he/him)
	Comment 1 • 11 years ago

This could be related to bug 743414 and bug 743215.

	Michael Kraft [:morac] Reporter
	Comment 2 • 11 years ago

(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.

	Erik Vold [:erikvold] (please needinfo? me) Assignee
	Comment 3 • 11 years ago

(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..

	Michael Kraft [:morac] Reporter
	Comment 4 • 11 years ago

(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.

	Erik Vold [:erikvold] (please needinfo? me) Assignee
	Comment 5 • 11 years ago

(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

	Michael Kraft [:morac] Reporter
	Comment 6 • 11 years ago

(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.

	Erik Vold [:erikvold] (please needinfo? me) Assignee
	Comment 7 • 11 years ago

(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.

	Michael Kraft [:morac] Reporter
	Comment 8 • 11 years ago

I wrote up a quick patch which appears to work.

	Michael Kraft [:morac] Reporter
	Comment 9 • 11 years ago

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?

	Erik Vold [:erikvold] (please needinfo? me) Assignee
	Comment 10 • 11 years ago

yep thanks!  I'll upload a new version tonight.

	Michael Kraft [:morac] Reporter
	Comment 11 • 11 years ago

I think I figured out how to do a pull request so if you want to use that you can.  Username is "Morac2".

	Erik Vold [:erikvold] (please needinfo? me) Assignee
	Comment 12 • 11 years ago

Cool a pull request would be easier on me.

	Michael Kraft [:morac] Reporter
	Comment 13 • 11 years ago

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

	Szabolcs Hubai (:xabolcs)
	Comment 14 • 11 years ago

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

	Michael Kraft [:morac] Reporter
	Comment 15 • 11 years ago

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.