Closed Bug 1429647 Opened 2 years ago Closed 2 years ago

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

Categories

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

Product:
Core
Component:
Preferences: Backend
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

(Whiteboard: [fingerprinting][fp-triaged])

Attachments

(1 file)

Bug 1429647 Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics
59 bytes, text/x-review-board-request
nika
: review+
Details 
We don't need ReleaseAcquire, we can use relaxed, per :mystor.

We'll land this early in the 60-cycle.
Whiteboard: [fingerprinting] → [fingerprinting][fp-triaged]
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.
Flags: needinfo?(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.
Flags: needinfo?(cmills)
Keywords: dev-doc-needed
Comment on attachment 8945265 [details]
Bug 1429647 Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics

https://reviewboard.mozilla.org/r/215486/#review222050
Attachment #8945265 - Flags: review?(nika) → review+
We can't land the attachment because mozreview  says that it needs a suitable reviewer.
Flags: needinfo?(tom)
Keywords: checkin-needed
Andrew can you r+ too?
Flags: needinfo?(tom) → needinfo?(overholt)
njn owns prefs but he's probably asleep. froydnj and erahm are prefs peers - maybe one of them can r+?
Flags: needinfo?(overholt)
Flags: needinfo?(nfroyd)
Flags: needinfo?(erahm)
(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.
Flags: needinfo?(tom)
Flags: needinfo?(nika)
Flags: needinfo?(nfroyd)
Flags: needinfo?(erahm)
(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.
Flags: needinfo?(nika)
(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 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.
Attachment #8945265 - Flags: review+
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43679b41bece
Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics r=froydnj,mystor
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/43679b41bece
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.