SettingsListener.observe() leaks copypaste.enabled observers

RESOLVED FIXED in FxOS-S4 (07Aug)

Status

Firefox OS
Gaia::System::Status bar, Utility tray, Notification
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gerard, Assigned: jeremychen)

Tracking

({perf, regression})

unspecified
FxOS-S4 (07Aug)
ARM
Gonk (Firefox OS)
perf, regression
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:+)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
+++ 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.
Summary: SettingsListener.obverse() leaks copypaste.enabled observers → SettingsListener.observe() leaks copypaste.enabled observers
QA Whiteboard: [top-dogfood]
Ken, your guys seem to own copy/paste, can one of them take this?
Flags: needinfo?(kchang)

Comment 2

3 years ago
Jeremy, please look into this bug. Thanks.
Flags: needinfo?(kchang) → needinfo?(jeremychen)
Got it. I'll add SettingsListener.unobserve copypaste.enabled as bug 1189004 comment 9 said first, and see if this solves the leak problem.
Flags: needinfo?(jeremychen)
(Reporter)

Comment 4

3 years ago
(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
Flags: needinfo?(jeremychen)
(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?
Flags: needinfo?(jeremychen)
(Reporter)

Comment 6

3 years ago
(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 ?
Flags: needinfo?(jeremychen)
(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.
Flags: needinfo?(jeremychen)
Created attachment 8642355 [details] [review]
[gaia] chenpighead:1189078 > mozilla-b2g:master
Created attachment 8642356 [details]
[before apply patch] memory-reports
Created attachment 8642358 [details]
[after apply patch] memory-reports
(Reporter)

Comment 11

3 years ago
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 ?
Flags: needinfo?(jeremychen)
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.
Flags: needinfo?(jeremychen)
Attachment #8642355 - Flags: feedback?(mtseng)
(Reporter)

Comment 13

3 years ago
Thanks!
Attachment #8642355 - Flags: feedback?(mtseng) → feedback+

Updated

3 years ago
tracking-b2g: --- → +
Attachment #8642355 - Flags: review?(timdream)
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.
Attachment #8642355 - Flags: review?(timdream)
(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
Flags: needinfo?(timdream)
Comment on attachment 8642355 [details] [review]
[gaia] chenpighead:1189078 > mozilla-b2g:master

Nice! I think the new patch is entirely correct.
Flags: needinfo?(timdream)
Attachment #8642355 - Flags: review+
Assignee: lissyx+mozillians → jeremychen
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/6bccb1e1d4713b4e7e45779a759a8c9a70ffaeb9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S4 (07Aug)
(Reporter)

Updated

3 years ago
Blocks: 1193469
You need to log in before you can comment on or make changes to this bug.