1348215 - WORKER_SIMPLE_PREF setup is slightly broken

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We do this with it:

#define WORKER_SIMPLE_PREF(name, getter, NAME)                                \
      NS_FAILED(Preferences::RegisterCallbackAndCall(                         \
                  WorkerPrefChanged,                                          \
                  name,                                                       \
                  reinterpret_cast<void*>(WORKERPREF_##NAME)) ||

and WorkerPrefChanged does:

  const WorkerPreference key =
    static_cast<WorkerPreference>(reinterpret_cast<uintptr_t>(aClosure));
  sDefaultPreferences[key] = Preferences::GetBool(aPrefName, false);

So now say we have a WORKER_SIMPLE_PREF("foo", something, FOOKEY) in WorkerPrefs.h.  This will register a callback for all preferences whose names start with "foo" (because the default value of the aMatchKind arg of RegisterCallbackAndCall is PrefixMatch, to match the behavior we used to have before bug 1267868 addded the aMatchKind arg at all; note that the worker code that registers callbacks for LoadContextOptions depends on the PrefixMatch behavior).

So now say I change a pref named "foobar".  We will get WorkerPrefChanged called, with "foobar" as aPrefName and FOOKEY as the closure, and proceed to set sDefaultPreferences[FOOKEY] to the value of the "foobar" pref.

Anyway, the fix is simple: smite it with Preferences::ExactMatch.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the SIMPLE_WORKER_PREF setup to not mishandle pref names that start with the worker pref name

Ehsan, this explains what I was seeing on try: I changed the test from setting the foo.bar pref to setting the foo.bar_not pref, but due to this bug it acted as if I had still set the foo.bar pref!

Comment 2

[(no longer active)]

What???  What kind of an insane default behavior is that?  Why is this desirable?  (I'm really shocked that this was the first time I managed to shoot off my foot like this.)

Comment 3

[(no longer active)]

Comment on attachment 8848399 [details] [diff] [review]
Fix the SIMPLE_WORKER_PREF setup to not mishandle pref names that start with the worker pref name

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

r+ even though I think it's libpref that's buggy. ;-)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> What???  What kind of an insane default behavior is that?

Well, it's really designed to let you observe prefbranches and whatnot...  That's why the callback is given the actual pref name that changed.

I do think now that we have this argument we should consider auditing the callsites and setting it to one or the other as needed.  And maybe flipping the default, or having no default at all.  I filed bug 1348331 on that and will just go through these callsites and adjust them as needed.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5491d388cdf4
Fix the SIMPLE_WORKER_PREF setup to not mishandle pref names that start with the worker pref name.  r=ehsan

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5491d388cdf4

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (PTO on March 20) from comment #2)
> What???  What kind of an insane default behavior is that?  Why is this
> desirable?  (I'm really shocked that this was the first time I managed to
> shoot off my foot like this.)

Hey, I just inherit 10-year-old behavior here. :)

bz's comment 4 seems like a reasonable guess at the rationale.  I reviewed the change to add the aMatchKind argument to RegisterCallback et al, but in retrospect, I probably should have asked things to go further..or at least filed a followup bug.