Open
Bug 577953
Opened 13 years ago
Updated 4 months ago
Minefield appears to be holding on to preference observers
Categories
(Core :: General, defect)
Tracking
()
NEW
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: sayrer, Unassigned)
References
Details
(Keywords: memory-leak)
Attachments
(2 files)
1.16 KB,
patch
|
Details | Diff | Splinter Review | |
622 bytes,
text/plain
|
Details |
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•13 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•13 years ago
|
||
I do have a sane number of DocShells and DOMWindows, though.
Reporter | ||
Comment 3•13 years ago
|
||
logging the membership of this array right now. I can see some problems by eyeballing it.
Reporter | ||
Comment 4•13 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•13 years ago
|
||
I don't think security.csp should be a preference anymore. Don't know what the other two leakers do.
![]() |
||
Comment 6•13 years ago
|
||
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: --- → ?
Comment 7•13 years ago
|
||
Filed bug 578182 for the CSP leak.
Reporter | ||
Comment 8•13 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
Comment 9•13 years ago
|
||
Adding Neil Deakin regarding accessibility.browsewithcaret observer in nsFocusManager (comment 6).
Comment 10•13 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.
![]() |
||
Comment 11•13 years ago
|
||
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•13 years ago
|
||
Reporter | ||
Comment 13•13 years ago
|
||
Comment 14•13 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: ? → -
Updated•4 months ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•