Closed Bug 1030115 Opened 6 years ago Closed 6 years ago

APZCTreeManager re-adds pref cache every time constructor is called

Categories

(Core :: Panning and Zooming, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: Gijs, Assigned: Gijs, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file)

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
This was added in bug 958596.
Blocks: 958596
(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.
Mentor: bugmail.mozilla
Whiteboard: [lang=c++]
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
Attachment #8445870 - Flags: review?(bugmail.mozilla)
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?(bugmail.mozilla)
Attachment #8445870 - Flags: review?(bas)
Attachment #8445870 - Flags: review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c445a59395d7
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.