917182 - Adding [Pref=prefname] to worker specific WebIDL interfaces causes "not thread-safe" assertions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Jan Varga [:janv]]

Vendo found it when he wanted to add [Pref="dom.indexedDB.experimental"] to IDBDatabaseSync interface.

partial interface IDBDatabaseSync {
    //[Pref="dom.indexedDB.experimental"]
    readonly    attribute StorageType        storage;
};

here's the assertion:
assertion failure: _mOwningThread.GetThread() == PR_GetCurrentThread() (ValueObserver not thread-safe), at jamun/modules/libpref/src/Preferences.cpp:133

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah we can't check preferences off the main thread.  We need a better story here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Seems like the only real option is a function that calls Preferences::GetBool on the main thread and sync-proxies the call over to the main thread on a worker thread?

Also, this code pattern:

  static bool sPrefCachesInited = false;
  if (!sPrefCachesInited) {
    sPrefCachesInited = true;
    Preferences::AddBoolVarCache(&sMethods[1].enabled, "svg.text.css-frames.enabled");
  }

used if you have preffed methods/attributes, is no good either.  That one is even worse, though, since it fundamentally involves writes on the main thread to a memory location that will be read on whatever thread the binding is set up on.  Maybe that's OK, as long as the AddBoolVarCache hapens on mainthread...

If you were hand-writing code for this on workers, how would you do it?  It seems like it would be pretty sucky.  :(

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think I would capture the relevant set of preferences at worker creation and ship them over at the start.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  We could try something like that, since the set of interfaces defined on workers is hardcoded...

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

We're a bit closer to solving this now that we have:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrefs.h

We could in theory make the webidl generated code do something like:

  if (NS_IsMainThread()) {
    // do normal pref stuff
    return value;
  }

  // Only returns a value if its in the WorkerPrefs.h header list
  return GetCurrentThreadWorkerPrivate()->GetPref(pref);

Comment 6

[Boris Zbarsky [:bzbarsky]]

At this point codegen will protect you from using [Pref] on worker-exposed things, so this is fixed as filed.  Bug 1490044 tracks making prefs usable on workes in webidl.