+++ This bug was initially created as a clone of Bug #1189004 +++ See the memory reports in bug 1189004 and also bug 1189004 comment 9: there is system app code that is leaking mozSettings observers with the setting copypaste.enabled.

Ken, your guys seem to own copy/paste, can one of them take this?

Jeremy, please look into this bug. Thanks.

Got it. I'll add SettingsListener.unobserve copypaste.enabled as bug 1189004 comment 9 said first, and see if this solves the leak problem.

(In reply to Jeremy Chen [:jeremychen] from comment #3) > Got it. I'll add SettingsListener.unobserve copypaste.enabled as bug 1189004 > comment 9 said first, and see if this solves the leak problem. Jeremy, it's easy to test: find the code path that calls this SettingsListener.observe(), and get about:memory from device. If it leaks, after some hit of adding the observer it should be visible in the "settings-observers-suspect" section of about:memory. The threshold was set to 20: https://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js#424

(In reply to Alexandre LISSY :gerard-majax from comment #4) > (In reply to Jeremy Chen [:jeremychen] from comment #3) > > Got it. I'll add SettingsListener.unobserve copypaste.enabled as bug 1189004 > > comment 9 said first, and see if this solves the leak problem. > > Jeremy, it's easy to test: find the code path that calls this > SettingsListener.observe(), and get about:memory from device. If it leaks, > after some hit of adding the observer it should be visible in the > "settings-observers-suspect" section of about:memory. > > The threshold was set to 20: > https://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager. > js#424 The number indeed decreases a bit. Should I just upload a patch?

(In reply to Jeremy Chen [:jeremychen] from comment #5) > (In reply to Alexandre LISSY :gerard-majax from comment #4) > > (In reply to Jeremy Chen [:jeremychen] from comment #3) > > > Got it. I'll add SettingsListener.unobserve copypaste.enabled as bug 1189004 > > > comment 9 said first, and see if this solves the leak problem. > > > > Jeremy, it's easy to test: find the code path that calls this > > SettingsListener.observe(), and get about:memory from device. If it leaks, > > after some hit of adding the observer it should be visible in the > > "settings-observers-suspect" section of about:memory. > > > > The threshold was set to 20: > > https://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager. > > js#424 > > The number indeed decreases a bit. Should I just upload a patch? Can you five figures ?

(In reply to Alexandre LISSY :gerard-majax from comment #6) > (In reply to Jeremy Chen [:jeremychen] from comment #5) > > (In reply to Alexandre LISSY :gerard-majax from comment #4) > > > (In reply to Jeremy Chen [:jeremychen] from comment #3) > > > > Got it. I'll add SettingsListener.unobserve copypaste.enabled as bug 1189004 > > > > comment 9 said first, and see if this solves the leak problem. > > > > > > Jeremy, it's easy to test: find the code path that calls this > > > SettingsListener.observe(), and get about:memory from device. If it leaks, > > > after some hit of adding the observer it should be visible in the > > > "settings-observers-suspect" section of about:memory. > > > > > > The threshold was set to 20: > > > https://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager. > > > js#424 > > > > The number indeed decreases a bit. Should I just upload a patch? > > Can you five figures ? Just found that I've done this wrong. I'll check again according to what I've discussed with :gerard-majax on IRC.

There is no "copypaste.enabled" in the second report. Are you sure it's the good one? Can you document your methodology to assert it is fixed ?

In original version, we only remove preference observer whenevenr copypaste.enable is changed to false. If an app is destroyed w/o changing copypaste.enable in its lifetime, the event handlers and preference observer will not be removed. In this patch, I add an unregister function which is called whenever an app is being destroyed. The unregister function will make sure we at least check once whether we already removed event handlers and preference observer or not. If not, remove them. I wrote the patch and verified locally. Both memory reports are recorded after I did following steps: 1. Open 22 apps 2. Long press to card view mode 3. Delete these 22 apps Looks like the leaks have been solved.

Thanks!

Comment on attachment 8642355 [details] [gaia] chenpighead:1189078 > mozilla-b2g:master I don't believe this patch fix the leak. We are doing |new AppTextSelectionDialog()| multiple times on multiple AppWindow. It is expected to get multiple callbacks installed. I assume the leak come from the fact that we don't remove the callback when AppWindows are destroyed. Unfortunately there is no destructor in JS so you would have to listen to AppWindow internal event for that.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #14) > Comment on attachment 8642355 [details] > [gaia] chenpighead:1189078 > mozilla-b2g:master > > I don't believe this patch fix the leak. > > We are doing |new AppTextSelectionDialog()| multiple times on multiple > AppWindow. It is expected to get multiple callbacks installed. I assume the > leak come from the fact that we don't remove the callback when AppWindows > are destroyed. > > Unfortunately there is no destructor in JS so you would have to listen to > AppWindow internal event for that. I suppose the callback you mentioned is the preference observer? Since AppTextSelectionDialog comes form BaseUI, I think we can use BaseUI.prototype.destroy in [1]. That's why I overwrite BaseUI.prototype._unregisterEvents to remove the installed callbacks in this patch. Could you take a look again? [1] http://mxr.mozilla.org/gaia/source/apps/system/js/base_ui.js#116

Comment on attachment 8642355 [details] [gaia] chenpighead:1189078 > mozilla-b2g:master Nice! I think the new patch is entirely correct.