1202592 - Activity chooser leaks settings observer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P2)

Description

[:gerard-majax]

Reproduced on my Z3c with regular master updates. Device has 9 days of uptime, and there is a few leaked settings observers.

> 21 (100.0%) -- settings-observers-suspect
> └──21 (100.0%) ── referent(topic=activity.default.viewurl)

I think any settings referenced in shared/js/default_activity_helper.js is a potential subject.

I already spotted some leaks reported on this one code path, with the photo pick activity.

Comment 1

[:gerard-majax]

Etienne, Guillaume or Fernando, this is code you landed/reviewed :)

Comment 2

[Etienne Segonzac (:etienne)]

Can you be a little bit more specific about what's causing the leak/how to fix it?
The DefaultActivityHelper only uses the simple SettingsHelper.set/get.

If out helper is causing a leak when doing a basic set/get we probably need to fix it there.

Comment 3

[:gerard-majax]

(In reply to Etienne Segonzac (:etienne) from comment #2)
> Can you be a little bit more specific about what's causing the leak/how to
> fix it?
> The DefaultActivityHelper only uses the simple SettingsHelper.set/get.
> 
> If out helper is causing a leak when doing a basic set/get we probably need
> to fix it there.

Each SettingsHelper() instanciation will trigger adding an observer. So we should make sure either the SettingsHelper is created only once ever, or the .uninit() method is called to remove the observer.

Comment 4

[Fernando Campo (:fcampo)]

If it's of any help for the leak, as bug 1039386 is stuck for a while now, and won't be implemented soon, we could backout bug 1073495 (as there's no way of changing the decisions taken unless reset or new app installed)

Comment 5

[:gerard-majax]

(In reply to Fernando Campo (:fcampo) from comment #4)
> If it's of any help for the leak, as bug 1039386 is stuck for a while now,
> and won't be implemented soon, we could backout bug 1073495 (as there's no
> way of changing the decisions taken unless reset or new app installed)

I don't understand your point: every leak like this got a simple fix of not instanciating a new object everytime.

Comment 6

[Guillaume Marty [:gmarty]]

Cleaning the ni as I haven't worked on this code. Ping me again if there's something I can do.

Comment 7

[Fernando Campo (:fcampo)]

(In reply to Alexandre LISSY :gerard-majax from comment #5)
> (In reply to Fernando Campo (:fcampo) from comment #4)
> > If it's of any help for the leak, as bug 1039386 is stuck for a while now,
> > and won't be implemented soon, we could backout bug 1073495 (as there's no
> > way of changing the decisions taken unless reset or new app installed)
> 
> I don't understand your point: every leak like this got a simple fix of not
> instanciating a new object everytime.
My point being to present another approach, that would be also useful as the rest of the bugs related to the original are not being worked on and it's an unfinished process that might lead to future bugs if not finished.
But as I understand that can be a longer process with more people/decisions involved, quick fix on the way instead.

Comment 8

[Guillaume Marty [:gmarty]]

It seems to me that there is a leak in SettingsListener too, responsible for the crash described in https://bugzilla.mozilla.org/show_bug.cgi?id=1196809#c2
Alex, have you noticed anything suspect in SettingsListener too?

Comment 9

[:gerard-majax]

(In reply to Fernando Campo (:fcampo) from comment #7)
> (In reply to Alexandre LISSY :gerard-majax from comment #5)
> > (In reply to Fernando Campo (:fcampo) from comment #4)
> > > If it's of any help for the leak, as bug 1039386 is stuck for a while now,
> > > and won't be implemented soon, we could backout bug 1073495 (as there's no
> > > way of changing the decisions taken unless reset or new app installed)
> > 
> > I don't understand your point: every leak like this got a simple fix of not
> > instanciating a new object everytime.
> My point being to present another approach, that would be also useful as the
> rest of the bugs related to the original are not being worked on and it's an
> unfinished process that might lead to future bugs if not finished.
> But as I understand that can be a longer process with more people/decisions
> involved, quick fix on the way instead.

Any news ?

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] fcampo:leak-observer-1202592 > mozilla-b2g:master

Comment 11

[Fernando Campo (:fcampo)]

(In reply to Alexandre LISSY :gerard-majax from comment #9)
> (In reply to Fernando Campo (:fcampo) from comment #7)
> > (In reply to Alexandre LISSY :gerard-majax from comment #5)
> > > (In reply to Fernando Campo (:fcampo) from comment #4)
> > > > If it's of any help for the leak, as bug 1039386 is stuck for a while now,
> > > > and won't be implemented soon, we could backout bug 1073495 (as there's no
> > > > way of changing the decisions taken unless reset or new app installed)
> > > 
> > > I don't understand your point: every leak like this got a simple fix of not
> > > instanciating a new object everytime.
> > My point being to present another approach, that would be also useful as the
> > rest of the bugs related to the original are not being worked on and it's an
> > unfinished process that might lead to future bugs if not finished.
> > But as I understand that can be a longer process with more people/decisions
> > involved, quick fix on the way instead.
> 
> Any news ?

Yeah, news are that I'm stupid and forgot to upload the patch before PTO :S

Comment 12

[Fernando Campo (:fcampo)]

Comment on attachment 8665911 [details] [review]
[gaia] fcampo:leak-observer-1202592 > mozilla-b2g:master

just checking that there's no SettingsHelper alive before creating a new one.


:gerard-majax, how are u exactly testing the leaks? I've never done it before

Comment 13

[:gerard-majax]

(In reply to Fernando Campo (:fcampo) from comment #12)
> Comment on attachment 8665911 [details] [review]
> [gaia] fcampo:leak-observer-1202592 > mozilla-b2g:master
> 
> just checking that there's no SettingsHelper alive before creating a new one.
> 
> 
> :gerard-majax, how are u exactly testing the leaks? I've never done it before

I'm just using the device long enough and collect the about:memory report :)

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 8665911 [details] [review]
[gaia] fcampo:leak-observer-1202592 > mozilla-b2g:master

r=me with nits

Comment 15

[Fernando Campo (:fcampo)]

nits done, waiting for treeherder. cheers

Comment 16

[Fernando Campo (:fcampo)]

merged - https://github.com/mozilla-b2g/gaia/commit/01ffe82cf088ca8fda9fe6783dc5cad2c3dde01c