1189078 - SettingsListener.observe() leaks copypaste.enabled observers

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Status bar, Utility tray, Notification, defect)

Description

[:gerard-majax]

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

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 2

[Ken Chang[:kenkai]]

Jeremy, please look into this bug. Thanks.

Comment 3

[Jeremy Chen ([e-mail])]

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

Comment 4

[:gerard-majax]

(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

Comment 5

[Jeremy Chen ([e-mail])]

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

Comment 6

[:gerard-majax]

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

Comment 7

[Jeremy Chen ([e-mail])]

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

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] chenpighead:1189078 > mozilla-b2g:master

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: [before apply patch] memory-reports

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: [after apply patch] memory-reports

Comment 11

[:gerard-majax]

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 ?

Comment 12

[Jeremy Chen ([e-mail])]

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.

Comment 13

[:gerard-majax]

Thanks!

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8642355 [details] [review]
[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.

Comment 15

[Jeremy Chen ([e-mail])]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #14)
> Comment on attachment 8642355 [details] [review]
> [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 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8642355 [details] [review]
[gaia] chenpighead:1189078 > mozilla-b2g:master

Nice! I think the new patch is entirely correct.

Comment 17

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/6bccb1e1d4713b4e7e45779a759a8c9a70ffaeb9