Open Bug 577953 Opened 13 years ago Updated 4 months ago

Minefield appears to be holding on to preference observers

Categories

(Core :: General, defect)

x86
macOS
defect

Tracking

()

Tracking Status
blocking2.0 --- -

People

(Reporter: sayrer, Unassigned)

References

Details

(Keywords: memory-leak)

Attachments

(2 files)

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.
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]
I do have a sane number of DocShells and DOMWindows, though.
logging the membership of this array right now. I can see some problems by eyeballing it.
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$
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.
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).
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).
Blocks: 578392
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: ? → -
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.