many sync observers build up during mochitest-browser-chrome runs

RESOLVED DUPLICATE of bug 1353571

Status

RESOLVED DUPLICATE of bug 1353571
5 years ago
10 months ago

People

(Reporter: froydnj, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa+])

(Reporter)

Description

5 years ago
While looking at the stats for recent mochitest-browser-chrome issues, I noticed that we were accumulating observers over time.  I modified the observer service to dump topics and their observers and got:

43:33.39 Observer list weave:service:setup-complete has 332 observers
43:33.46 Observer list weave:service:sync:start has 331 observers
43:33.32 Observer list weave:service:start-over has 331 observers
43:33.32 Observer list weave:service:logout:finish has 331 observers
43:33.45 Observer list weave:service:login:start has 330 observers
43:33.39 Observer list weave:ui:sync:error has 330 observers
43:33.39 Observer list weave:ui:clear-error has 330 observers
43:33.39 Observer list weave:service:sync:delayed has 330 observers
43:33.39 Observer list weave:service:login:finish has 330 observers
43:33.32 Observer list weave:ui:sync:finish has 330 observers
43:33.32 Observer list weave:service:quota:remaining has 330 observers
43:33.32 Observer list weave:notification:added has 330 observers
43:33.25 Observer list weave:ui:login:error has 330 observers
43:33.25 Observer list weave:eol has 330 observers

I hope you will agree this is a little excessive.  Is this the expected behavior?
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-syncui.js

That list implies that gSyncUI.initUI was called once each for 330 windows, and each window was unloaded before Weave.Status.ready became true (see line 48).

This code somewhat predates me, so I don't know exactly why it's doing that, but as some context: most of Sync is never loaded unless Sync is configured, and it mostly consists of singletons that last as long as the entire browser. Not bothering to unregister on *quit* is good practice; I don't know if that's being misapplied here.
(Reporter)

Comment 2

5 years ago
(In reply to Richard Newman [:rnewman] from comment #1)
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> syncui.js
> 
> That list implies that gSyncUI.initUI was called once each for 330 windows,
> and each window was unloaded before Weave.Status.ready became true (see line
> 48).

But why should that matter?  We should just remove all the observers, as the window is going away.

> Not bothering to unregister on *quit* is good practice; I don't
> know if that's being misapplied here.

I think it is; creating new windows and closing them will result in observers endlessly accumulating.  Those observers shouldn't outlast the window.
(In reply to Nathan Froyd (:froydnj) from comment #2)

> But why should that matter?  We should just remove all the observers, as the
> window is going away.

"so I don't know exactly why it's doing that".

There's a lot of stuff that Sync does that is necessary for some obtuse, undocumented reason.

There's also a lot of stuff that it does because it was hacked together as a Labs prototype, started life as an add-on, and has been mostly unmaintained for two years, with most of the original authors having moved on.

It's hard to tell which is which without manual testing and exploration, as well as this kind of "it should..." analysis.

For example, what if you close the last window half-way through Sync's initialization (approximately 10,050ms after launch)? Maybe nothing, maybe things b0rk.


> I think it is; creating new windows and closing them will result in
> observers endlessly accumulating.  Those observers shouldn't outlast the
> window.

I agree with you in principle, but let's make sure that any patch doesn't regress TPS or manual QA.
Whiteboard: [qa+]

Updated

10 months ago
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1353571
You need to log in before you can comment on or make changes to this bug.