Closed Bug 1202592 Opened 6 years ago Closed 6 years ago

Activity chooser leaks settings observer

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+)

RESOLVED FIXED
FxOS-S8 (02Oct)
blocking-b2g 2.5+

People

(Reporter: gerard-majax, Assigned: fcampo)

References

Details

(Keywords: perf, regression, Whiteboard: [systemsfe])

Attachments

(1 file)

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.
Etienne, Guillaume or Fernando, this is code you landed/reviewed :)
Blocks: 1073495
Flags: needinfo?(gmarty)
Flags: needinfo?(fernando.campo)
Flags: needinfo?(etienne)
Keywords: regression
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.
Flags: needinfo?(etienne) → needinfo?(lissyx+mozillians)
(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.
Flags: needinfo?(lissyx+mozillians)
Whiteboard: [systemsfe]
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)
Flags: needinfo?(fernando.campo)
(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.
Flags: needinfo?(fernando.campo)
Cleaning the ni as I haven't worked on this code. Ping me again if there's something I can do.
Flags: needinfo?(gmarty)
(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.
Assignee: nobody → fernando.campo
Flags: needinfo?(fernando.campo)
blocking-b2g: --- → 2.5+
Priority: -- → P2
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?
Flags: needinfo?(lissyx+mozillians)
Flags: needinfo?(lissyx+mozillians)
(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 ?
Flags: needinfo?(fernando.campo)
(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
Flags: needinfo?(fernando.campo)
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
Attachment #8665911 - Flags: review?(etienne)
(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 on attachment 8665911 [details] [review]
[gaia] fcampo:leak-observer-1202592 > mozilla-b2g:master

r=me with nits
Attachment #8665911 - Flags: review?(etienne) → review+
nits done, waiting for treeherder. cheers
Keywords: checkin-needed
merged - https://github.com/mozilla-b2g/gaia/commit/01ffe82cf088ca8fda9fe6783dc5cad2c3dde01c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → FxOS-S8 (02Oct)
You need to log in before you can comment on or make changes to this bug.