577953 - Minefield appears to be holding on to preference observers

Status: NEW

Description

[Robert Sayre]

This looks like a pretty bad leak. I'm still debugging, but I have a printf in nsPrefBranch::RemoveObserver that prints the count of observers in there. Observers do get removed, but the baseline seems to keep rising.

Comment 1

[Robert Sayre]

After half an hour or so of browsing, I have about 900 of these hanging out.

--DOMWINDOW == 17 (0x12180fd38) [serial = 607] [outer = 0x123e6ba90] [url = http://www.mozilla.org/projects/minefield/]
--DOMWINDOW == 16 (0x123e6baf8) [serial = 603] [outer = 0x0] [url = http://www.mozilla.org/projects/minefield/]
Removing observers. Count is 968.
--DOMWINDOW == 15 (0x1077cdce8) [serial = 600] [outer = 0x1077ce3a0] [url = about:blank]
--DOMWINDOW == 14 (0x1077ce408) [serial = 599] [outer = 0x0] [url = chrome://browser/content/browser.xul]
Removing observers. Count is 967.
Removing observers. Count is 966.
Removing observers. Count is 965.
Removing observers. Count is 964.
Removing observers. Count is 963.
Removing observers. Count is 962.
Removing observers. Count is 961.
Removing observers. Count is 960.
Removing observers. Count is 959.
Removing observers. Count is 958.
Removing observers. Count is 957.
--DOMWINDOW == 13 (0x124ea5a38) [serial = 615] [outer = 0x124eec6d0] [url = about:blank]

Comment 2

[Robert Sayre]

I do have a sane number of DocShells and DOMWindows, though.

Comment 3

[Robert Sayre]

logging the membership of this array right now. I can see some problems by eyeballing it.

Comment 4

[Robert Sayre]

Looks like we are keeping observers for security.csp.enable, browser.zoom, and accessibility.browsewithcaret around.

Random sample with several windows open:
668 observers in list
Top 20:
58 : security.csp.enable
44 : javascript.options.
17 : gfx.font_rendering.
17 : browser.xul.error_pages.enabled
16 : layout.css.dpi
16 : browser.visited_color
16 : layout.css.devPixelsPerPx
16 : font.
16 : bidi.
16 : image.animation_mode
15 : dom.popup_allowed_events
15 : browser.underline_anchors
15 : ui.key.chromeAccess
15 : ui.click_hold_context_menus
15 : browser.active_color
15 : ui.key.contentAccess
15 : nglayout.events.dispatchLeftClickOnly
15 : browser.anchor_color
15 : browser.display.
15 : accessibility.accesskeycausesactivation


An hour later with no windows open, and very few DOMWindows (like only 5 or so, yay!):

1171 observers in list
Top 20:
723 : security.csp.enable
64 : browser.zoom.
64 : accessibility.browsewithcaret
33 : javascript.options.
3 : network.enableIDN
3 : browser.safebrowsing.enabled
2 : gfx.font_rendering.
2 : dom.popup_allowed_events
2 : ui.listVerticalInsidePadding
2 : browser.safebrowsing.malware.enabled
2 : browser.xul.error_pages.enabled
2 : intl.accept_languages
2 : font.
2 : intl.uidirection.
1 : network.IDN_testbed
1 : image.mem.decodeondraw
1 : image.http.accept
1 : dom.max_chrome_script_run_time
1 : browser.cache.disk_cache_ssl
1 : ui.listHorizontalInsidePadding
Robert-Sayres-MacBook-Pro:Desktop sayrer$

Comment 5

[Robert Sayre]

I don't think security.csp should be a preference anymore. Don't know what the other two leakers do.

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Sounds like someone is not removing observers when they should...

In particular, the CSP code in the nsDocument constructor is using AddBoolPrefVarCache, which is only meant to be used for global vars, and only meant to be used once per var.  That is NOT what the CSP code is doing.  We should probably file separate bugs blocking this one on the separate issues...

accessibility.browsewithcaret needs to be a pref.  Looks like the only pref observers for it are in nsFocusManager.cpp (added in nsFocusManager::Init and removed in ~nsFocusManager) and in nsTypeAheadFind.cpp (added in Init() and removed in the destructor).  The focus manager is a service.  Are we leaking typeaheadfind instances for the app lifetime or something?

browser.zoom also needs to be a pref.  The only observer seems to be in browser-fullZoom.js.  It removes in destroy().  Are we failing to call that?

Comment 7

[Brandon Sterne (:bsterne)]

Filed bug 578182 for the CSP leak.

Comment 8

[Robert Sayre]

The fix for the CSP pref definitely helped.

The state of affairs at DOMWindow #11, just after startup:

348 observers in list
Top 20:
30 : javascript.options.
6 : browser.xul.error_pages.enabled
4 : gfx.font_rendering.
4 : layout.css.devPixelsPerPx
4 : font.
3 : dom.popup_allowed_events
3 : network.enableIDN
3 : browser.underline_anchors
3 : ui.key.chromeAccess
3 : ui.click_hold_context_menus
3 : layout.css.dpi
3 : browser.active_color
3 : ui.key.contentAccess
3 : bidi.
3 : nglayout.events.dispatchLeftClickOnly
3 : browser.anchor_color
3 : browser.display.
3 : image.animation_mode
3 : accessibility.accesskeycausesactivation
3 : browser.visited_color

The state of affairs at DOMWindow #1000:

491 observers in list
Top 20:
86 : accessibility.browsewithcaret
85 : browser.zoom.
33 : javascript.options.
3 : network.enableIDN
3 : browser.safebrowsing.enabled
2 : gfx.font_rendering.
2 : dom.popup_allowed_events
2 : ui.listVerticalInsidePadding
2 : browser.safebrowsing.malware.enabled
2 : browser.xul.error_pages.enabled
2 : intl.accept_languages
2 : font.
2 : intl.uidirection.
1 : network.IDN_testbed
1 : image.mem.decodeondraw
1 : image.http.accept
1 : dom.max_chrome_script_run_time
1 : browser.cache.disk_cache_ssl
1 : ui.listHorizontalInsidePadding
1 : browser.display.show_image_placeholders

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Adding Neil Deakin regarding accessibility.browsewithcaret observer in nsFocusManager (comment 6).

Comment 10

[Neil Deakin]

The typeaheadfind component is created in tabbrowser.xml as well as within each <browser>. Not sure why both are needed. Is the test above for repeated opening of new tabs? That might suggest the latter not being deleted.

As an aside, I don't think the typeahead find needs to observe the accessibility.browsewithcaret preference. Retrieving it each time should be sufficient.

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

That should be a separate bug with a separate discussion, esp. because with e10s pref-getting is about to get a lot more expensive (needs an IPC call if it happens in the content process, for example).

Comment 12

[Robert Sayre]

Created attachment 457078 [details] [diff] [review]
print observer population to stdout

Comment 13

[Robert Sayre]

Created attachment 457080 [details]
python script to compare two observer populations

Comment 14

[Benjamin Smedberg]

Not going to block on the meta-bug: it looks like the top 3 in your list are not per-tab or per-JSContext, I might block on those specifically.