1030115 - APZCTreeManager re-adds pref cache every time constructor is called

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[:Gijs (he/him)]

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.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Can you be specific as to which variable you're talking about? Also there's only ever a single APZCTreeManager created.

Comment 2

[:Gijs (he/him)]

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

Comment 3

[:Gijs (he/him)]

This was added in bug 958596.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

After discussion with Gijs on IRC, I filed bug 1030126 to investigate the possibility of multiple APZCTreeManager instances being created in the same process.

Comment 6

[:Gijs (he/him)]

Attachment: fix apz.printtree caching to use gfxPrefs,

This seems to work. Try push: https://tbpl.mozilla.org/?tree=Try&rev=e542db1f1600

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 8

[:Gijs (he/him)]

Try push in comment #6 is green (apart from intermittents), so I'd land this, but the tree is closed. Setting checkin-needed instead.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c445a59395d7

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c445a59395d7