Closed Bug 1353936 Opened 7 years ago Closed 7 years ago

Preferences::Add*VarCache should ensure they will update var caches before any other observers or callbacks

Categories

(Core :: Preferences: Backend, enhancement)

Product:
Core
Component:
Preferences: Backend
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(3 files)

Bug 1353936 - Test to ensure var caches are updated before any other ExactMatch callbacks.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1353936 - Implement PREF_RegisterPriorityCallback.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1353936 - Use PREF_RegisterPriorityCallback from Add*VarCache.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
Moreover, they will miss pref changes by profile change.

I had to write a gross hack to workaround the problem. (See bug 1353493)
Summary: Preferences::Add*VarCache should ensure they will update car caches before any other observers or callbacks → Preferences::Add*VarCache should ensure they will update var caches before any other observers or callbacks
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment on attachment 8856118 [details]
Bug 1353936 - Implement PREF_RegisterPriorityCallback.

https://reviewboard.mozilla.org/r/128062/#review130958

Clearing review for now, because I think this can be done more clearly.

::: modules/libpref/Preferences.h:452
(Diff revision 1)
> -    PrefixMatch,
> -    ExactMatch,
> +    PrefixMatch   = 0,
> +    ExactMatch    = 1 << 0,
> +    PriorityMatch = 1 << 1,

The `PriorityMatch` enum needs some kind of documentation.  But see also comments below.

::: modules/libpref/Preferences.cpp:199
(Diff revision 1)
> -    mClosures.RemoveElement(aClosure);
> +    auto i = mClosures.IndexOf(aClosure);
> +    if (i == mClosures.NoIndex) {
> +      return;
> +    }
> +    mCallbacks.RemoveElementAt(i);
> +    mClosures.RemoveElementAt(i);

Why are you keying on the closure data only and not the callback and the closure data?  That's what the original code did, but I think doing that now means that we could register multiple ValueObservers with identical closure data, but then remove the wrong one at some future point, which seems like a hard bug to track down.

::: modules/libpref/Preferences.cpp:1731
(Diff revision 1)
>  Preferences::RegisterCallback(PrefChangedFunc aCallback,
>                                const char* aPref,
>                                void* aClosure,
>                                MatchKind aMatchKind)

I think things would be a little clearer if this function took a `CallbackKind` enum, `Observer` or `VarCache`, rather than trying to stuff that information into `MatchKind` where it does not belong.  Doing that change makes this function a little unwieldly due to the large number of parameters, but I think it makes it clearer that cache observers take priority in all cases.  The `HashKey` code in particular is helped by this change, I think.
Attachment #8856118 - Flags: review?(nfroyd)
Comment on attachment 8856117 [details]
Bug 1353936 - Test to ensure var caches are updated before any other ExactMatch callbacks.

https://reviewboard.mozilla.org/r/128060/#review130972

Thanks for writing a test!

::: modules/libpref/test/gtest/CallbackAndVarCacheOrder.cpp:125
(Diff revision 1)
> +  ASSERT_TRUE(NS_SUCCEEDED(rv));
> +
> +  ASSERT_FALSE(closure1.called);
> +  SetFunc(prefName1, value2);
> +  ASSERT_EQ(var1, value2);
> +  ASSERT_TRUE(closure1.called);

I can't believe this didn't work prior to your patch.
Attachment #8856117 - Flags: review?(nfroyd) → review+
Comment on attachment 8856118 [details]
Bug 1353936 - Implement PREF_RegisterPriorityCallback.

https://reviewboard.mozilla.org/r/128062/#review131438

::: modules/libpref/Preferences.h:452
(Diff revision 1)
> -    PrefixMatch,
> -    ExactMatch,
> +    PrefixMatch   = 0,
> +    ExactMatch    = 1 << 0,
> +    PriorityMatch = 1 << 1,

MatchKind is no longer touched.

::: modules/libpref/Preferences.cpp:199
(Diff revision 1)
> -    mClosures.RemoveElement(aClosure);
> +    auto i = mClosures.IndexOf(aClosure);
> +    if (i == mClosures.NoIndex) {
> +      return;
> +    }
> +    mCallbacks.RemoveElementAt(i);
> +    mClosures.RemoveElementAt(i);

Add*VarCache uses dedicated callbacks. If I key the callbacks as the original code did, var cache will use different PREF_RegisterCallback callbacks from other observers. But PREF_RegisterCallback does not guarantee the order of callbacks. So var cache might be notified after other callbacks or observers.

That said, I stopped touching ValueObserverHashKey and added a new function to give the desired guarantee.

::: modules/libpref/Preferences.cpp:1731
(Diff revision 1)
>  Preferences::RegisterCallback(PrefChangedFunc aCallback,
>                                const char* aPref,
>                                void* aClosure,
>                                MatchKind aMatchKind)

I added a new function instead.
Comment on attachment 8856118 [details]
Bug 1353936 - Implement PREF_RegisterPriorityCallback.

https://reviewboard.mozilla.org/r/128062/#review131454

::: modules/libpref/prefapi.cpp:73
(Diff revision 2)
> -static struct CallbackNode* gCallbacks = nullptr;
> +static struct CallbackNode* gFirstCallback = nullptr;
> +static struct CallbackNode* gLastCallback = nullptr;

Making this a `mozilla::LinkedList` would be a nice cleanup after this lands; can you file a bug for that?
Attachment #8856118 - Flags: review?(nfroyd) → review+
Comment on attachment 8857012 [details]
Bug 1353936 - Use PREF_RegisterPriorityCallback from Add*VarCache.

https://reviewboard.mozilla.org/r/128910/#review131452

Thanks, this is much easier to follow!  One comment request below.

::: modules/libpref/Preferences.cpp:1815
(Diff revision 1)
>      gObserverTable->Remove(observer);
>    }
>    return NS_OK;
>  }
>  
>  static void BoolVarChanged(const char* aPref, void* aClosure)

Can you add a comment here describing why we're using `RegisterPriorityCallback` for caches rather than `RegisterCallback`?  Something like:

```
// We insert cache observers using RegisterPriorityCallback to ensure they are called
// prior to ordinary pref observers.  Doing this ensures that...
```

and then describe a scenario where things go wrong if we treated cache observers like ordinary observers.  It'd be better, of course, if this was templated or something so we have a single place to put this comment, but putting it here seems better than not putting anything at all.
Attachment #8857012 - Flags: review?(nfroyd) → review+
Comment on attachment 8856118 [details]
Bug 1353936 - Implement PREF_RegisterPriorityCallback.

The previous patch reversed the order of ordinary callbacks and it broke some tests because they depend on the order. Some non-test codes might also depend. So I reversed the order again.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=382f4a11ef5e4965076ea3f59632f1d3270a206c
Attachment #8856118 - Flags: review+ → review?(nfroyd)
Comment on attachment 8856118 [details]
Bug 1353936 - Implement PREF_RegisterPriorityCallback.

https://reviewboard.mozilla.org/r/128062/#review132618
Attachment #8856118 - Flags: review?(nfroyd) → review+
Comment on attachment 8857012 [details]
Bug 1353936 - Use PREF_RegisterPriorityCallback from Add*VarCache.

https://reviewboard.mozilla.org/r/128910/#review132712
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/bec1fdf2a886
Test to ensure var caches are updated before any other ExactMatch callbacks. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/86e51288bb41
Implement PREF_RegisterPriorityCallback. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/60b966337731
Use PREF_RegisterPriorityCallback from Add*VarCache. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/bec1fdf2a886
https://hg.mozilla.org/mozilla-central/rev/86e51288bb41
https://hg.mozilla.org/mozilla-central/rev/60b966337731
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1356565
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: