Currently we have a WorkerPref.h with a list of prefs needed by workers. We update them using runnables for any active WorkerPrivate. Instead, we can use Atomic<bool>. This bug is about replacing the current WorkerPrefs.h macros with Atomic<bool> variables.

Comment on attachment 8930910 [details] [diff] [review] part 4 - DOM cache testing Review of attachment 8930910 [details] [diff] [review]: ----------------------------------------------------------------- It looks like you missed converting the: Preferences::GetBool("dom.caches.testing.enabled", false) || line in CacheStorage::CreateOnMainThread(), although you got the serviceworker variant one in part 9. I assume this was an oversight.

Comment on attachment 8930947 [details] [diff] [review] part 20 - privacy fingerprint Review of attachment 8930947 [details] [diff] [review]: ----------------------------------------------------------------- (I affirm that there are a ton of callers to nsContentUtils::ShouldResistFingerprinting() and so it makes sense to leave that around, deferring to the new DOMPreferences check.)

Comment on attachment 8930904 [details] [diff] [review] part 1 - dump() Review of attachment 8930904 [details] [diff] [review]: ----------------------------------------------------------------- Notes to self/other reviewers: - The thread safety issues for Preferences.cpp are addressed in part 23. - Existing dump() semantics are maintained. ::: dom/base/DOMPreferences.cpp @@ +36,5 @@ > +#endif > + > +#undef PREF > + > +#define PREF_WEBIDL(name) \ A comment that provides some basic context about what makes the WEBIDL variant useful would be nice here. For example: WebIDL "Func" decorations require this method signature because other binding checks may desire the context, redirect it to the no-arg pref check.

Comment on attachment 8931215 [details] [diff] [review] part 23 - initialization on main-thread Review of attachment 8931215 [details] [diff] [review]: ----------------------------------------------------------------- With the introduction of DOMPreferences::Initialize(), the continued existence of the `static bool initialized` field and lazy initialization in each function seems awkward unless it's a hack to reduce boilerplate/macro magic. (But now that there are multiple definitions of PREF, that bridge seems like it's been crossed[1].) Specifically, nsLayoutStatics::Initialize() will be invoked during NS_InitXPCOM2() which is strictly before nsXREDirProvider::DoStartup() when profile-do-change/profile-after change will be invoked, and that's the first legal time to access prefs and get valid values. So there's no chance of a (sane) caller invoking these methods before they're initialized. (Once nice upside of the StylePrefs quasi-idiom of just directly calling AddBoolVarCache in the init method is that ASAN/valgrind/etc. should likely catch insane uses before the init method is invoked.) I don't really have a strong opinion on any of this, and I'm fine with a follow-up cleaning things up, but it'd be great to have at least a brief comment that explains why things are the way they are. Even if it's just something like: initialized and related logic exist because of a prior attempt to lazy-initialize the preferences; this should be eliminated in favor of macro-defined or boilerplate static members that are explicitly initialized via AddBoolVarCache in the Initialize method. 1: Note that I think that Searchfox recently gained the ability to do more magic on macros, so the prototypes may actually be able to use macros safely. Not 100% sure right now. ::: dom/base/DOMPreferences.h @@ +9,5 @@ > > namespace mozilla { > namespace dom { > > class DOMPreferences final Please add a block comment in this file that provides some context about why the file exists, when to add preferences here, and how to add preferences (sorta like WorkerPrefs.h had). Here's a candidate: """ DOMPreferences provides consolidated, consistent access to preferences for DOM APIs that may be invoked on Worker threads as well as the main thread. Previously this was handled through a combination of per-API boilerplate helper functions that would determine what thread they were on and defer to the WorkerPrefs mechanism if they weren't the main thread. These methods can safely be invoked on any thread. Add preferences to this file when they need to be checked from both the main thread and worker threads or when adding a new DOM API that is preference controlled. ## How to add a new preference For preferences to be checked from code: - Add a `FooEnabled();` prototype to DOMPreferences.h preceded by a comment. The prototypes could be implemented as macro-expansions automatically, but the macros can confuse code analysis tools (that are less clever than searchfox). - Add a `PREF(FooEnabled, "the.pref.name")` line to DOMPreferencesInternal.h. For preferences to also be checked from WebIDL "Func" decorator checks: - Add a `FooEnabled(JSContext* aCx, JSObject* aObj);` prototype to DOMPreferences.h. - Add a `PREF_WEBIDL(FooEnabled)` line to DOMPreferences.cpp. """

Thanks for the comment and the review! The reason why I added this DOMPreferences::Initialize() is that I need the pref to be initialized on the main-thread in order to have them accessible on any thread. I add a comment about this.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a68e6500d9 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 1 - dump enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/4050a1b8db2b Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 2 - image bitmap, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/d9999a624097 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 3 - DOM Caches enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/f182eba574f9 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 4 - DOM Cache testing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0a4bfd654f Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 5 - Performance logging enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/c58ef2a91bad Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 6 - Notification API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc192478bf5 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 7 - Notification Request-Interaction enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/862aeaa2fefd Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 8 - ServiceWorkers enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/0d23880842d8 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 9 - ServiceWorker testing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/65998b0740f3 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 10 - ServiceWorker Client OpenWindow enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/108806f14ad8 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 11 - StorageManager enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/60464b3e468f Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 12 - Promise Rejection Event enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/b7f342f436ef Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 13 - Push enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/e32cd55b4c96 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 14 - Streams API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/8a54c1c8d45d Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 15 - Request Context enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/a8154698f782 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 16 - Offscreen Canvas enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/ed146ac42a7f Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 17 - Webkit/Blink directory picker enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/7f9de012861e Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 18 - Network Information enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/c03970363433 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 19 - FetchObserver API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/caf7428013f2 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 20 - Resist finger-printing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/c39ee002c825 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 21 - DevTools enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc3c29a9e36 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 22 - Get rid of WorkerPrefs.h, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/2242edc902a5 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 23 - DOMPreferences initialized at startup, r=asuth

Backed out for frequently failing service workers related devtools tests, e.g. devtools/client/aboutdebugging/test/browser_service_workers_push.js https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2242edc902a5b99f97c00677053a78eff3c9b4ef&filter-searchStr=dt&selectedJob=148862513 https://treeherder.mozilla.org/logviewer.html#?job_id=148862513&repo=mozilla-inbound&lineNumber=2002 https://hg.mozilla.org/integration/mozilla-inbound/rev/f5457b8b352a1d7c503baa81c33108b1d12a1278 Also there are frequent failures like this one that need to be checked: https://treeherder.mozilla.org/logviewer.html#?job_id=148872886&repo=mozilla-inbound&lineNumber=2195 https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e5dd080198e6577eebdc0916745243d65f981db7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&selectedJob=148872886

I think we can fix the intermittents by removing all usage of waitForMutation from aboutdebugging tests [1]. In my investigation I focused on [browser_service_workers.js] and [browser_service_workers_push.js]. By swapping waitForMutations with waitUntil() statements, I managed to get rid of the intermittent failures locally. I'll need some time to apply this to the whole suite but I'm confident it should solve the problem here. We already identified this issue and logged Bug 1386613 to fix it but had no strong reason to address it until now. I'll block your bug on it and will fix the tests in the dedicated bug. [1] https://searchfox.org/mozilla-central/search?q=waitForMutation&case=false&regexp=false&path=devtools%2Fclient%2Faboutdebugging%2Ftest

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b628d9298be8 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 1 - dump enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/2e263a2519c5 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 2 - image bitmap, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/000c8d5fbc03 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 3 - DOM Caches enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/19b14deed8fe Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 4 - DOM Cache testing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/8ecc91474621 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 5 - Performance logging enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/0866d79873ab Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 6 - Notification API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/6c863ab2e986 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 7 - Notification Request-Interaction enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/d12dc5557982 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 8 - ServiceWorkers enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/601b49f51b41 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 9 - ServiceWorker testing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/5e44aeda4196 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 10 - StorageManager enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d674bdc723 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 11 - Promise Rejection Event enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/7624e70b2965 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 12 - Push enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/5f6df771459a Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 13 - Streams API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/7e77a00fa8b5 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 14 - Request Context enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/1397a6bbb446 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 15 - Offscreen Canvas enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd9d964df8a Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 16 - Webkit/Blink directory picker enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/f500a61f564c Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 17 - Network Information enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/9f40cc12d6c6 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 18 - FetchObserver API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/4b52904694f4 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 19 - Resist finger-printing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f4b98a07b6 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 20 - DevTools enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/0206657d2ea2 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 21 - Get rid of WorkerPrefs.h, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b7b46c8ad0 Introduce DOMPreferences, a thread-safe access to preferences for DOM - part 22 - DOMPreferences initialized at startup, r=asuth

Heads up, while I fixed a bunch of race conditions in about:debugging tests, I still had one intermittent failure when rebased on top of the patches from this bug. The test that keeps failing is browser_service_workers_push.js.

Backout by rgurzau@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ccd7b237c433 Backed out 22 changesets for build bustage build/src/dom/base/FuzzingFunctions.cpp on a CLOSED TREE

(In reply to Julian Descottes [:jdescottes][:julian] from comment #36) > Heads up, while I fixed a bunch of race conditions in about:debugging tests, > I still had one intermittent failure when rebased on top of the patches from > this bug. The test that keeps failing is browser_service_workers_push.js. Have you filed a new bug?

njn, sorry if I haven't asked you a feedback about this DOMPreferences set of patches. It is something I wrote in order to avoid the complexity we have in workers. The basic idea is to use atomic getters instead having runnables dispatched to any active worker. I'm asking you to take a look at the first patch. The following ones are just the migration of each WorkerPrefs to DOMPreferences.

Just keeping an eye on this

(In reply to Andrea Marchesini [:baku] from comment #38) > (In reply to Julian Descottes [:jdescottes][:julian] from comment #36) > > Heads up, while I fixed a bunch of race conditions in about:debugging tests, > > I still had one intermittent failure when rebased on top of the patches from > > this bug. The test that keeps failing is browser_service_workers_push.js. > > Have you filed a new bug? Just filed 1425897. I can push a workaround to silence the issue if you would like to land this soon, but it's unclear to me if you should disregard this failure as a test only issue.

Comment on attachment 8930904 [details] [diff] [review] part 1 - dump() Review of attachment 8930904 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/DOMPreferences.cpp @@ +19,5 @@ > + static Atomic<bool> cachedValue; \ > + if (!initialized) { \ > + initialized = true; \ > + Preferences::AddAtomicBoolVarCache(&cachedValue, \ > + pref, false); \ This will fit on a single line.

First, I would call it DOMPrefs, because it's shorter and matches gfxPrefs and MediaPrefs. Second, gfxPrefs and MediaPrefs use macros more (including in the .h file) which means that there can be less boilerplate. E.g. you could generate `static bool DevToolsEnabled(); from the macro `PREF(DevToolsEnabled, "devtools.enabled")`.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/83a53b3e6edc Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 1 - dump enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ab6f8f55cd Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 2 - image bitmap, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/035af44209d0 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 3 - DOM Caches enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/5f29519f40ba Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 4 - DOM Cache testing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/c00691ea962f Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 5 - Performance logging enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/bf04487d746a Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 6 - Notification API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/33ef0d8ca138 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 7 - Notification Request-Interaction enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/0763044fef05 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 8 - ServiceWorkers enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/785e52d7a0fe Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 9 - ServiceWorker testing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb06ac64f58 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 10 - StorageManager enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/6b6ce0862fd4 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 11 - Promise Rejection Event enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/c5c4104210b2 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 12 - Push enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/ab06b612becf Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 13 - Streams API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6aadebae4f Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 14 - Request Context enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1abb98a4c3 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 15 - Offscreen Canvas enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/0918da01e554 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 16 - Webkit/Blink directory picker enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/676de6824043 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 17 - Network Information enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/0e377b6fb316 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 18 - FetchObserver API enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf5ddac1e20 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 19 - Resist finger-printing enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/895ca9b0e923 Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 20 - DevTools enabled, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9c890371bd Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 21 - Get rid of WorkerPrefs.h, r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/92324d3a0edf Introduce DOMPrefs, a thread-safe access to preferences for DOM - part 22 - DOMPrefs initialized at startup, r=asuth

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1f40908daa Fixing compilation issue, r=me - CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/83a53b3e6edc https://hg.mozilla.org/mozilla-central/rev/c0ab6f8f55cd https://hg.mozilla.org/mozilla-central/rev/035af44209d0 https://hg.mozilla.org/mozilla-central/rev/5f29519f40ba https://hg.mozilla.org/mozilla-central/rev/c00691ea962f https://hg.mozilla.org/mozilla-central/rev/bf04487d746a https://hg.mozilla.org/mozilla-central/rev/33ef0d8ca138 https://hg.mozilla.org/mozilla-central/rev/0763044fef05 https://hg.mozilla.org/mozilla-central/rev/785e52d7a0fe https://hg.mozilla.org/mozilla-central/rev/ccb06ac64f58 https://hg.mozilla.org/mozilla-central/rev/6b6ce0862fd4 https://hg.mozilla.org/mozilla-central/rev/c5c4104210b2 https://hg.mozilla.org/mozilla-central/rev/ab06b612becf https://hg.mozilla.org/mozilla-central/rev/cf6aadebae4f https://hg.mozilla.org/mozilla-central/rev/cb1abb98a4c3 https://hg.mozilla.org/mozilla-central/rev/0918da01e554 https://hg.mozilla.org/mozilla-central/rev/676de6824043 https://hg.mozilla.org/mozilla-central/rev/0e377b6fb316 https://hg.mozilla.org/mozilla-central/rev/1cf5ddac1e20 https://hg.mozilla.org/mozilla-central/rev/895ca9b0e923 https://hg.mozilla.org/mozilla-central/rev/0c9c890371bd https://hg.mozilla.org/mozilla-central/rev/92324d3a0edf https://hg.mozilla.org/mozilla-central/rev/cd1f40908daa

Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b1db70bc89 followup: fix mingw build bustage