Minefield appears to be holding on to preference observers

NEW
Unassigned

Status

()

8 years ago
4 years ago

People

(Reporter: sayrer, Unassigned)

Tracking

({memory-leak})

unspecified
x86
Mac OS X
memory-leak
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
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]
(Reporter)

Comment 2

8 years ago
I do have a sane number of DocShells and DOMWindows, though.
(Reporter)

Comment 3

8 years ago
logging the membership of this array right now. I can see some problems by eyeballing it.
(Reporter)

Comment 4

8 years ago
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$
(Reporter)

Comment 5

8 years ago
I don't think security.csp should be a preference anymore. Don't know what the other two leakers do.
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?
blocking2.0: --- → ?
Keywords: mlk
Depends on: 578182
Filed bug 578182 for the CSP leak.
(Reporter)

Comment 8

8 years ago
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
Adding Neil Deakin regarding accessibility.browsewithcaret observer in nsFocusManager (comment 6).

Comment 10

8 years ago
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.
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).
(Reporter)

Comment 12

8 years ago
Created attachment 457078 [details] [diff] [review]
print observer population to stdout
(Reporter)

Comment 13

8 years ago
Created attachment 457080 [details]
python script to compare two observer populations
(Reporter)

Updated

8 years ago
Blocks: 578392

Comment 14

8 years ago
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.
blocking2.0: ? → -
You need to log in before you can comment on or make changes to this bug.