Assert when non-Atomic static prefs are accessed off the main thread

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Seems like this could be a footgun.
Hrm.  Try says that we hit that assert in various cases.  I wonder what the practical upshot is of access to these prefs on background threads...
In a lot of cases, it's probably just that readers might not see the writes from other threads.  We might have enough memory fences running around (e.g. mutex lock/unlock) that it doesn't matter too much in practice, and I'd bet there are prefs whose owners have said "we don't care *that* much about consistency".

Still seems like it'd be something good to fix.
> In a lot of cases, it's probably just that readers might not see the writes from other threads.

That seems fine if that's all it is.  That said, those are also the RelaxedAtomic semantics, no?

A concrete case I'm looking at used to have a pref cache on the main thread writing to int globals and then used those globals off the main thread by just reading the global.  It was converted to a non-Atomic static pref, which does seem like it preserves the old semantics; the question is whether those old semantics make sense.  Could we end up seeing a partial write?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> > In a lot of cases, it's probably just that readers might not see the writes from other threads.
> 
> That seems fine if that's all it is.  That said, those are also the
> RelaxedAtomic semantics, no?

They are; the Relaxed bit just makes it more obvious to the compiler that's what you're doing, so it doesn't format your hard drive or something.  The Relaxed atomics also signal to TSan etc. that you know what you're doing.

> A concrete case I'm looking at used to have a pref cache on the main thread
> writing to int globals and then used those globals off the main thread by
> just reading the global.  It was converted to a non-Atomic static pref,
> which does seem like it preserves the old semantics; the question is whether
> those old semantics make sense.  Could we end up seeing a partial write?

I think the guarantee on everything that we care about is that writes <= word size are "atomic" in the sense that you never see part of the old value and part of the new.
Hmm, ok.  So is this still worth doing?
I think it's worth doing; the underlying problem was filed as bug 1436916. However, we can't land it as-is if we're getting failures. Also, the lack of Atomic<float> may make life difficult. I was contemplating using Atomic<int32_t> to represent VarCache floats and then casting on get/set... I'm not sure if it's a brilliant idea or a terrible idea.
Comment on attachment 8982274 [details] [diff] [review]
Enforce use of Atomic for static prefs gotten off the main thread

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

r=me, once this doesn't trigger assertions in practice.
Attachment #8982274 - Flags: review?(n.nethercote) → review+
"Fun" fact: MSVC emits static initializers for global Atomic<T>s.
Yeah, I'm going to post an updated patch once I have a green try run.  Slowly getting there.  ;)  Mostly media bits need to become atomic...

Comment 9 does not sound good for our purposes here.  :(
Oh, and at the moment we have no float-typed StaticPrefs...  When we want to add one that's accessed on a background thread, we will need to worry about lack of Atomic<float>.
Attachment #8982676 - Flags: review?(n.nethercote)
Attachment #8982274 - Attachment is obsolete: true
> we will need to worry about lack of Atomic<float>.

Yep, as per comment 7 I wonder if we could fake it with Atomic<int32_t>.
Comment on attachment 8982676 [details] [diff] [review]
Now green on try

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

I'm pleased by how simple the checking functionality is. I'm also pleased that so many pref accesses are now race-free!
Attachment #8982676 - Flags: review?(n.nethercote) → review+

Comment 15

Last year
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0df4de6a931f
Enforce use of Atomic for static prefs gotten off the main thread.  r=njn
I guess one downside here is that this change could block the conversion of float prefs to StaticPrefs, unless there is a way to carve out an exception for floats in the checking.
  template<>
  struct IsAtomic<float> : TrueType {}

should do it, I would think.  Probably with a renaming from IsAtomic to IsOKOffMainThread or something...

That said, if the float prefs are not accessed off-main-thread there is no problem, and if they are accessed off-main-thread then we have racy access, right?
> That said, if the float prefs are not accessed off-main-thread there is no
> problem, and if they are accessed off-main-thread then we have racy access,
> right?

Yes, but unlike the non-float cases, we don't have an easy way to fix the racy-ness.

Comment 19

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/0df4de6a931f
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1467134
You need to log in before you can comment on or make changes to this bug.