1029509 - Add debug only (fatal) assert for adding multiple pref caches against the same memory address

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[:Gijs (he/him)]

As bz noted aptly in bug 982428 "[that] is basically a memory leak".

We should just not allow this to happen. The easiest way is probably to make the Preferences::Add[Type]VarCache methods assert in debug builds if the variable against which we're caching was already passed before.

This could be done by either looping through gCacheData or by adding a debug only more efficient structure against which we can check...

Comment 1

[:Gijs (he/him)]

Attachment: assert for new caches with known addresses,

Let's see how this goes... https://tbpl.mozilla.org/?tree=Try&rev=f99f883ade25

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> Created attachment 8445183 [details] [diff] [review]
> assert for new caches with known addresses,
> 
> Let's see how this goes...
> https://tbpl.mozilla.org/?tree=Try&rev=f99f883ade25

Badly, clearly. At least some of this is the case being fixed in bug 982428. The gtest failures seem to point to gfxPrefs... not sure why yet. I'll pick this back up once bug 982428 has landed and stuck.

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Created attachment 8445183 [details] [diff] [review]
> > assert for new caches with known addresses,
> > 
> > Let's see how this goes...
> > https://tbpl.mozilla.org/?tree=Try&rev=f99f883ade25
> 
> Badly, clearly. At least some of this is the case being fixed in bug 982428.
> The gtest failures seem to point to gfxPrefs... not sure why yet. I'll pick
> this back up once bug 982428 has landed and stuck.

remote:   https://tbpl.mozilla.org/?tree=Try&rev=eb1ac2a3b782

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > (In reply to :Gijs Kruitbosch from comment #1)
> > > Created attachment 8445183 [details] [diff] [review]
> > > assert for new caches with known addresses,
> > > 
> > > Let's see how this goes...
> > > https://tbpl.mozilla.org/?tree=Try&rev=f99f883ade25
> > 
> > Badly, clearly. At least some of this is the case being fixed in bug 982428.
> > The gtest failures seem to point to gfxPrefs... not sure why yet. I'll pick
> > this back up once bug 982428 has landed and stuck.
> 
> remote:   https://tbpl.mozilla.org/?tree=Try&rev=eb1ac2a3b782

Well, that was dumb: https://tbpl.mozilla.org/?tree=Try&rev=c1a41a84c111

(I did try to check that stuff compiled, but I ran an opt build instead of a debug build... this time I'm more sure)

In any case, I looked into the gtest issue locally. It seems like the problem is that all of the AsyncPanZoomController gtests initialize and then destroy the gfxPrefs singleton (ditto for the gfxPrefs' own gtest). Unfortunately, there is no way to remove a pref cache, and so destroying the singleton leaves the pref cache intact... and then recreating gfxPrefs for the next test adds another cache for those bits. Markus said he understands why that happens at the same address (which I do not), so I'm CC'ing him. :-)

Comment 5

[:Gijs (he/him)]

Attachment: assert for new caches with known addresses,

This time I stole code from the non-local network access checks we use for tryserver in order to be able to have the warning include the pref as well as the pointer address that's at issue.

Comment 6

[:Gijs (he/him)]

Attachment: assert for new caches with known addresses,

Addressed a nit from bjacob (using size_t for the loop). I tried to use varargs for MOZ_CRASH but that doesn't compile because while the macro forwards the varargs, MOZ_ReportCrash doesn't deal with them well ( http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#151 ).

Comment 7

[:Gijs (he/him)]

Try push from inbound tip with bug 1030090 landed, plus the patch from bug 1030115 which is pending r?bas to see if there's anything else:

https://tbpl.mozilla.org/?tree=Try&rev=16dd888251dd

(AFAICT all the non-intermittent orange from the previous try run should be covered by either of these bugs, and there was none on b2g or android so I'm leaving those out of this push)

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> Try push from inbound tip with bug 1030090 landed, plus the patch from bug
> 1030115 which is pending r?bas to see if there's anything else:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=16dd888251dd
> 
> (AFAICT all the non-intermittent orange from the previous try run should be
> covered by either of these bugs, and there was none on b2g or android so I'm
> leaving those out of this push)

Egh. Messed up my qpushes - cancelled and repushed with the patch from bug 1030115 actually included: https://tbpl.mozilla.org/?tree=Try&rev=968d8a5bfa25

Comment 9

[Benjamin Smedberg]

Comment on attachment 8445973 [details] [diff] [review]
assert for new caches with known addresses,

>diff --git a/modules/libpref/src/Preferences.cpp b/modules/libpref/src/Preferences.cpp

>+#ifdef DEBUG
>+static bool
>+HaveExistingCacheFor(void* address) {

Style nit, opening brace of function should be in column 0.

Also asserting NS_IsMainThread would be good here.

> // static
> nsresult
> Preferences::AddBoolVarCache(bool* aCache,
>                              const char* aPref,
>                              bool aDefault)
> {
>   NS_ASSERTION(aCache, "aCache must not be NULL");
>+#ifdef DEBUG
>+  if (HaveExistingCacheFor(aCache)) {
>+    fprintf_stderr(stderr,
>+      "Attempt to add a bool pref cache for preference '%s' at address '%p'"
>+      "was made. However, a pref was already cached at this address.\n",
>+      aPref, aCache);
>+    MOZ_CRASH("Should not have an existing pref cache for this address");

This is a little confusing because MOZ_CRASH is normally for release-mode crashes, but it's in a DEBUG ifdef. Please use MOZ_ASSERT(false) to make it clear that this is still debug-only. Also are you sure you want to duplicate this printf three times, you could instead just have a function void AssertNotAlreadyCached(void*) which does the assert internally. I prefer that, but r=me either way.

Comment 10

[:Gijs (he/him)]

With all the nits addressed,

remote:   https://hg.mozilla.org/integration/fx-team/rev/4113d8711881

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4113d8711881