Closed Bug 1400865 Opened 2 years ago Closed 2 years ago

too many pref watchers for browser.tabs.remote.warmup.*

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 + fixed

People

(Reporter: froydnj, Assigned: mconley)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

[Tracking Requested - why for this release]: memory leak-like behavior in long-lived/large browser sessions.

I see in my about:memory for the parent process from today:

22,467 (100.0%) -- preference-service
└──22,467 (100.0%) -- referent
   ├──21,619 (96.23%) -- weak
   │  ├──21,306 (94.83%) ── dead
   │  └─────313 (01.39%) ── alive
   └─────848 (03.77%) ── strong

21,198 (100.0%) -- preference-service-suspect
├──10,599 (50.00%) ── referent(pref=browser.tabs.remote.warmup.enabled)
└──10,599 (50.00%) ── referent(pref=browser.tabs.remote.warmup.maxTabs)

These numbers have been steadily going up all weekend.

It looks like tabbrowser.xml registers pref watchers through these via XPCOMUtils.defineLazyPreferenceGetter, which registers the weak preference observers.  The observers themselves go away when the tabbed browser is destroyed, but the weak references to them sit around in the preference service, taking up memory and doing nothing.  They would get cleaned up if the watched preferences were changed, I believe, and the preference service noticed that it had a bunch of weak observers that needed to go away, but otherwise, they're just sitting there.

I see that tab warmup was disabled in bug 1394455, but these preference observers are still present even with the feature disabled.

tabbrowser.xml already unregisters a bunch of stuff when it gets destroyed; it would be nice if it could manually unregister these pref watchers too.
Thanks froydnj - looking.
Flags: needinfo?(mconley)
So, my fault here, we were being pretty silly before, and creating these lazy preference getters every time we instantiated an async tab switcher, which could happen (in the worst case) every time you switch tabs. I'm not entirely sure how the weak reference eviction stuff works for the preference observer service, but at least with this patch, we're guaranteed to only instantiate one lazy observer per pref per tabbrowser window.
Flags: needinfo?(mconley)
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Thank you!  So we're still "leaking" things, we just leak them at a much slower rate now?  I guess the right thing to do there is to adjust XPCOMUtils to enable deleting the pref watches when we don't care about them any more?
Flags: needinfo?(mconley)
(In reply to Nathan Froyd [:froydnj] from comment #4)
> Thank you!  So we're still "leaking" things, we just leak them at a much
> slower rate now?  I guess the right thing to do there is to adjust
> XPCOMUtils to enable deleting the pref watches when we don't care about them
> any more?

Well, maybe this is an opportunity to clear something up for me. How were we leaking in the first place? If I'm understanding this correctly, the observer is connected to the lifetime of the aObject that's passed into defineLazyPreferenceGetter. How come the destruction of the async tab switcher at the end of a successful tab switch wasn't causing those preference observers to be destroyed? The observers are weakly held... and if the only thing holding onto them is destroyed, shouldn't that take them out?
Flags: needinfo?(mconley) → needinfo?(nfroyd)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #5)
> Well, maybe this is an opportunity to clear something up for me. How were we
> leaking in the first place? If I'm understanding this correctly, the
> observer is connected to the lifetime of the aObject that's passed into
> defineLazyPreferenceGetter. How come the destruction of the async tab
> switcher at the end of a successful tab switch wasn't causing those
> preference observers to be destroyed? The observers are weakly held... and
> if the only thing holding onto them is destroyed, shouldn't that take them
> out?

That's a great question.  My understanding is that when we register observers with the pref service, we have:

  pref service -> observer

For defineLazyPreferenceGetter, however, we register the observer weakly, which means we have something between the pref service and the observer, a "weak reference":

  pref service -> weak reference -> observer

What does the weak reference do here?  Well, it ensures that the pref service doesn't have an owning reference to the observer, so the lifetime of the observer can be decoupled from whether it's observing prefs.

When the pref service sees a change to a pref for which such an observer is registered, it can ask the weak reference, "do you still have an observer?"  If the weak reference does, the observer will be notified of the change; if it doesn't, the weak reference can be removed, because the object it was referring to has gone away by other means.

So what was happening before was that we were registering lots of weakly-held observers for the async tab switching things we were creating.  Those tab switchers were GC'd, and through *magic*, the weak references were nulled out, so they wouldn't be holding on to GC'd memory.  But the pref service was still holding on to all those (now null) weak references, and those references were just piling up, and piling up, because there was no mechanism by which they can be GC'd.  And since we created a number of tab switches, we had a lot of useless weak references piling up.

We'll still have the same situation after your patch, but theoretically, we shouldn't be creating tabbed browsers at the same rate we created tab switchers--I guess we only create one tabbed browser per window, right?  If that's true, we'd have to open a lot of windows before we even got close to the numbers we had before.  And we already have one lazy preference getter, animationsEnabled, which I haven't seen problems with (yet ;), so we may just be OK on that front.

All clear, or clear as mud?
Flags: needinfo?(nfroyd)
Yep, totally clear. Thanks for a super excellent explanation!

For some reason I expected the reference itself to vanish once the thing it's referring to vanished, but yeah, what we're talking about is an array of nsISupportsWeakReference's, which themselves consume memory even if they're not pointing at anything.

I wonder if the pref service can do more to evict these things - perhaps after a CC/GC, it could go through and clear out weak references pointed at nothing. Hey njn, I know you've been stomping around in Preferences code lately... is there an action here to make the Preferences backend clear out null weak references more aggressively?

At any rate, I think at least reducing the number of preference observers we're creating for the warmer to be bounded by the number of browser windows is probably a sufficient step forward here.
ni? to njn for comment 7.

I'm a little hesistant to have the pref service (and the observer service, since the same principle operates there) be more aggressive about clearing out weak references, because it's extra work to do every GC/CC, and because noticing observers registered out of proportion to the number of windows or tabs is usually a bug that we should address.  It's more difficult to notice that if you're silently cleaning up after mistakes.
Flags: needinfo?(n.nethercote)
(In reply to Nathan Froyd [:froydnj] from comment #8)

> I'm a little hesistant to have the pref service (and the observer service,
> since the same principle operates there) be more aggressive about clearing
> out weak references, because it's extra work to do every GC/CC

Could we do it less frequently? Eg. each time a browser window is closed

(In reply to Mike Conley (:mconley) (:⚙️) from comment #7)
> Yep, totally clear. Thanks for a super excellent explanation!

+1 to that, thanks for comment 6!
Comment on attachment 8909402 [details]
Bug 1400865 - Put tab warming prefs on tabbrowser binding instead of async tab switcher object.

https://reviewboard.mozilla.org/r/180918/#review186136
Attachment #8909402 - Flags: review?(florian) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/65b5b0da8345
Put tab warming prefs on tabbrowser binding instead of async tab switcher object. r=florian
> Hey njn, I know you've been stomping around in Preferences code
> lately... is there an action here to make the Preferences backend clear out
> null weak references more aggressively?

It's more that I plan to stomp around in Preferences code in the near future. Right now I don't have enough Prefs knowledge to address this, but I have added this question to my list of things to investigate.
Flags: needinfo?(n.nethercote)
https://hg.mozilla.org/mozilla-central/rev/65b5b0da8345
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.