Closed Bug 1348215 Opened 5 years ago Closed 5 years ago

WORKER_SIMPLE_PREF setup is slightly broken

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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.
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!
Attachment #8848399 - Flags: review?(ehsan)
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.)
Flags: needinfo?(nfroyd)
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. ;-)
Attachment #8848399 - Flags: review?(ehsan) → review+
> 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.
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/5491d388cdf4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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.
Flags: needinfo?(nfroyd)
You need to log in before you can comment on or make changes to this bug.