Closed
Bug 1429647
Opened 4 years ago
Closed 4 years ago
Switch privacy.reduceTimerPrecision and privacy.resistFingerprinting to use Relaxed Semantics
Categories
(Core :: Preferences: Backend, enhancement, P1)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: tjr, Assigned: tjr)
References
Details
(Whiteboard: [fingerprinting][fp-triaged])
Attachments
(1 file)
We don't need ReleaseAcquire, we can use relaxed, per :mystor. We'll land this early in the 60-cycle.
Updated•4 years ago
|
Whiteboard: [fingerprinting] → [fingerprinting][fp-triaged]
Assignee | ||
Updated•4 years ago
|
Priority: P2 → P1
Updated•4 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•4 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 3•4 years ago
|
||
(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 4•4 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•4 years ago
|
Keywords: checkin-needed
Comment 5•4 years ago
|
||
We can't land the attachment because mozreview says that it needs a suitable reviewer.
Flags: needinfo?(tom)
Updated•4 years ago
|
Keywords: checkin-needed
Comment 7•4 years ago
|
||
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)
![]() |
||
Comment 8•4 years ago
|
||
(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)
Comment 9•4 years ago
|
||
(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)
![]() |
||
Comment 10•4 years ago
|
||
(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•4 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Flags: needinfo?(tom)
Keywords: checkin-needed
Assignee | ||
Updated•4 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•4 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Comment 16•4 years ago
|
||
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
Comment 17•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/43679b41bece
Status: NEW → RESOLVED
Closed: 4 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.
Description
•