Closed Bug 1030115 Opened 6 years ago Closed 6 years ago
Manager re-adds pref cache every time constructor is called
It's using a static variable to cache a pref result, but it doesn't check whether there's already a pref cache for the variable. Which means every time an APZCTreeManager gets created, we add a new pref listener (and we never remove any). This is sadfaces and we shouldn't be doing it.
Can you be specific as to which variable you're talking about? Also there's only ever a single APZCTreeManager created.
Component: Graphics: Layers → Panning and Zooming
05:17:11 INFO - Attempt to add a bool pref cache for preference 'apz.printtree' at address '0x106c4bb11'was made. However, a pref was already cached at this address. https://tbpl.mozilla.org/php/getParsedLog.php?id=42439980&tree=Try&full=1#error0 from https://tbpl.mozilla.org/?tree=Try&rev=c1a41a84c111 which would point to this line being invoked more than once: http://hg.mozilla.org/mozilla-central/annotate/a19e0434ea52/gfx/layers/apz/src/APZCTreeManager.cpp#l49
(In reply to :Gijs Kruitbosch from comment #2) > which would point to this line being invoked more than once: > > http://hg.mozilla.org/mozilla-central/annotate/a19e0434ea52/gfx/layers/apz/ > src/APZCTreeManager.cpp#l49 ... or there being another cache for it, such as the one at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=c0e27620432b#619 This should be easily fixable by moving the pref cache into gfx/thebes/gfxPrefs.h and then referencing it from both the APZCTreeManager constructor and the nsDisplayList.cpp use site.
After discussion with Gijs on IRC, I filed bug 1030126 to investigate the possibility of multiple APZCTreeManager instances being created in the same process.
This seems to work. Try push: https://tbpl.mozilla.org/?tree=Try&rev=e542db1f1600
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8445870 [details] [diff] [review] fix apz.printtree caching to use gfxPrefs, Review of attachment 8445870 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I also tested locally to make sure the pref still works as intended. However I think Bas also needs to rubber-stamp the Logging.h change since he reviewed that code originally and owns the gfx/2d code.
Attachment #8445870 - Flags: review?(bas) → review+
Try push in comment #6 is green (apart from intermittents), so I'd land this, but the tree is closed. Setting checkin-needed instead.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.