Closed Bug 1429885 Opened 2 years ago Closed 2 years ago

A reduceTimerPrecision rounding value of 0 is not supported

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 - fixed
firefox59 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

(Whiteboard: [fingerprinting])

Attachments

(1 file)

If we set 'privacy.resistFingerprinting.reduceTimerPrecision.round_microseconds' to 0 - we get bad values.  Setting it to 1 is fine though. 

If we set it to 0, we should not do any rounding.
Comment on attachment 8941952 [details]
Bug 1429885 Support a rounding value of 0 for reduceTimerPrecision

https://reviewboard.mozilla.org/r/212140/#review218010

In general, it looks good to me. But, there some minor improvements that could make the code much cleaner.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:84
(Diff revision 1)
>  
>  /* static */
>  double
>  nsRFPService::ReduceTimePrecisionAsMSecs(double aTime)
>  {
> -  if (!IsTimerPrecisionReductionEnabled()) {
> +  if (!IsTimerPrecisionReductionEnabled() || sResolutionUSec == 0) {

Could you put the check inside the function 'IsTimerPrecisionReductionEnabled()' and add some comment around this?
Attachment #8941952 - Flags: review?(tihuang) → review+
Priority: -- → P2
[Tracking Requested - why for this release]:
I think we should uplift this to beta. This feature is in beta (Bug 1424341), and while it is not enabled for users (yet) we expect to do so midstream. This patch is a simple fix that closes off weird, undefined behavior (timers in javascript start returning negative numbers of NaN) that can be triggers innocently/accidentally.
Comment on attachment 8941952 [details]
Bug 1429885 Support a rounding value of 0 for reduceTimerPrecision

Hey Ben, I need a peer to r+ this too, could you take a look?


Approval Request Comment
[Feature/Bug causing the regression]: Bug 1424341

[User impact if declined]: If a user sets the preference to 0, they will get subtle, undefined behavior that will cause javascript errors in lots of websites.

[Is this code covered by automated tests?]: No, we don't have a test for 0

[Has the fix been verified in Nightly?]: No

[Needs manual test from QE? If yes, steps to reproduce]: It doesn't need it, but setting the pref to 0 and confirm performance.now() isn't NaN would be the steps to test it

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: No

[Why is the change risky/not risky?]: We move a couple functions and add a condition.

[String changes made/needed]: None
Attachment #8941952 - Flags: review?(bkelly)
Attachment #8941952 - Flags: approval-mozilla-beta?
Comment on attachment 8941952 [details]
Bug 1429885 Support a rounding value of 0 for reduceTimerPrecision

https://reviewboard.mozilla.org/r/212140/#review218266
Attachment #8941952 - Flags: review?(bkelly) → review+
Pushed by archaeopteryx@coole-files.de
https://hg.mozilla.org/integration/autoland/rev/cbb0db087e5e5960ee25391873cde70998efbf88
Support a rounding value of 0 for reduceTimerPrecision r=bkelly,timhuang
https://hg.mozilla.org/mozilla-central/rev/cbb0db087e5e
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8941952 [details]
Bug 1429885 Support a rounding value of 0 for reduceTimerPrecision

Fix a regression of bug 1424341. Beta58+.
Attachment #8941952 - Flags: approval-mozilla-release+
Attachment #8941952 - Flags: approval-mozilla-beta?
Attachment #8941952 - Flags: approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.