1429647 - Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement, P1)

Description

[Tom Ritter [:tjr] (ni for sec-approvals and such)]

We don't need ReleaseAcquire, we can use relaxed, per :mystor.

We'll land this early in the 60-cycle.

Comment 1

[Tom Ritter [:tjr] (ni for sec-approvals and such)]

Chris, I don't believe this requires a dev doc; the operation of these prefs shouldn't be changed from a user or extension-writer perspective.

Comment 2

[Tom Ritter [:tjr] (ni for sec-approvals and such)]

Attachment: Bug 1429647 Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics

Review commit: https://reviewboard.mozilla.org/r/215486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215486/

Comment 3

[Chris Mills [:cmills]]

(In reply to Tom Ritter [:tjr] from comment #1)
> Chris, I don't believe this requires a dev doc; the operation of these prefs
> shouldn't be changed from a user or extension-writer perspective.

OK, thanks! I've removed the keyword again.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8945265 [details]
Bug 1429647 Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics

https://reviewboard.mozilla.org/r/215486/#review222050

Comment 5

[Dorel Luca [:dluca]]

We can't land the attachment because mozreview  says that it needs a suitable reviewer.

Comment 6

[Tom Ritter [:tjr] (ni for sec-approvals and such)]

Andrew can you r+ too?

Comment 7

[Andrew Overholt [:overholt]]

njn owns prefs but he's probably asleep. froydnj and erahm are prefs peers - maybe one of them can r+?

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Tom Ritter [:tjr] from comment #0)
> We don't need ReleaseAcquire, we can use relaxed, per :mystor.

I guess I can believe this, but what's the justification?  Performance (on what platforms?)?  I'm really hesistant to just r+ changes around memory ordering without some kind of explanation of why we think this is OK.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nathan Froyd [:froydnj] from comment #8)
> (In reply to Tom Ritter [:tjr] from comment #0)
> > We don't need ReleaseAcquire, we can use relaxed, per :mystor.
> 
> I guess I can believe this, but what's the justification?  Performance (on
> what platforms?)?  I'm really hesistant to just r+ changes around memory
> ordering without some kind of explanation of why we think this is OK.

The basic summary of what happened was:

When I originally added the atomic pref value which used ReleaseAcquire semantics before, it was a mostly arbitrary choice. Racyness in setting this pref isn't a big worry (as we don't need to make sure that different threads have a consistent view, just that the pref is eventually updated).

When Tom was making changes to this component he was asking why I made the decision, and I pretty much explained that it was mostly arbitrary. I wanted to avoid SeqCst loads and stores because they seemed unnecessary, so I used ReleaseAcquire instead.

I also said it would probably be fine to have the pref be stored with Relaxed ordering as well, and that ended up spawning this bug. I think the motivation is that this pref is loaded every timer read, so making that read fast is valuable.

I don't feel strongly either way about this change.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Nika Layzell [:mystor] from comment #9)
> When I originally added the atomic pref value which used ReleaseAcquire
> semantics before, it was a mostly arbitrary choice. Racyness in setting this
> pref isn't a big worry (as we don't need to make sure that different threads
> have a consistent view, just that the pref is eventually updated).
> 
> I also said it would probably be fine to have the pref be stored with
> Relaxed ordering as well, and that ended up spawning this bug. I think the
> motivation is that this pref is loaded every timer read, so making that read
> fast is valuable.
> 
> I don't feel strongly either way about this change.

I don't feel strongly either way, either.  Everywhere except Android, these are going to generate the same code.

I will r+ with a comment here in a sec.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8945265 [details]
Bug 1429647 Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics

https://reviewboard.mozilla.org/r/215486/#review223352

r=me with some kind of comment, see below.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:62
(Diff revision 1)
> -Atomic<bool, ReleaseAcquire> nsRFPService::sPrivacyResistFingerprinting;
> -Atomic<bool, ReleaseAcquire> nsRFPService::sPrivacyTimerPrecisionReduction;
> +Atomic<bool, Relaxed> nsRFPService::sPrivacyResistFingerprinting;
> +Atomic<bool, Relaxed> nsRFPService::sPrivacyTimerPrecisionReduction;
>  // Note: anytime you want to use this variable, you should probably use TimerResolution() instead
> -Atomic<uint32_t, ReleaseAcquire> sResolutionUSec;
> +Atomic<uint32_t, Relaxed> sResolutionUSec;

Something along the lines of mystor's comment 9 would be nice here:

"We don't particularly care that all threads have a perfectly consistent view of the values of these prefs, just that they are eventually consistent.  As these pref caches can be read frequently (e.g. every timer [setting,firing,etc.]), performance is important."

I'm not too fussed about the exact wording, but having some evidence that people though about non-sequentially consistent memory orderings is a good thing.

Comment 12

[Tom Ritter [:tjr] (ni for sec-approvals and such)]

Comment on attachment 8945265 [details]
Bug 1429647 Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215486/diff/1-2/

Comment 13

[Tom Ritter [:tjr] (ni for sec-approvals and such)]

Comment on attachment 8945265 [details]
Bug 1429647 Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215486/diff/2-3/

Comment 14

[Tom Ritter [:tjr] (ni for sec-approvals and such)]

Comment on attachment 8945265 [details]
Bug 1429647 Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215486/diff/3-4/

Comment 15

[Tom Ritter [:tjr] (ni for sec-approvals and such)]

Alright, lets land this.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43679b41bece
Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics r=froydnj,mystor

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/43679b41bece