1267868 - AddXXXVarCache uses partial string matching

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Matt Woodrow (:mattwoodrow)]

pref_DoCallback uses strncmp to decide which callbacks to fire, so modifying layout.display-list.dump-content also fires the callback we registered for layout-display-list.dump.

BoolVarChanged uses the modified pref name (.dump-content) in its call to GetBool(), so we set the cached value of .dump to the incoming value for .dump-content.

I think we either need an exact strcmp somewhere in the callback chain (though apparently pref_DoCallback relies on strncmp), or we need to store the pref name in our closure and use that for GetBool.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: pref-fix

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8745721 [details] [diff] [review]
pref-fix

Review of attachment 8745721 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for the delay in reviewing this.

I don't think this is going to fly; e.g. nsPresContext calls RegisterCallback with string prefixes, not fixed preferences:

http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresContext.cpp#848

and so I'm pretty sure it expects ValueObserver to pass things through to the callback and not do extra checking.

Just grepping for Preferences::RegisterCallback turns up a couple of other places that do this too.  I think it'd be feasible to do exact string matching for *only* the Add*Cache variants, but that would take some more plumbing.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: pref-fix v2

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8746915 [details] [diff] [review]
pref-fix v2

Review of attachment 8746915 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, very nice.  r=me with the change below.

::: modules/libpref/Preferences.h
@@ +252,5 @@
>     */
>    static nsresult RegisterCallback(PrefChangedFunc aCallback,
>                                     const char* aPref,
> +                                   void* aClosure = nullptr,
> +                                   bool aAllowPartialMatches = true);

Let's use an enum here:

enum MatchKind {
  PrefixMatch,
  ExactMatch,
};

defaulting to PrefixMatch, with corresponding changes elsewhere.  Update the documentation too, please.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84ce9439f0c6

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84ce9439f0c6