Closed Bug 1431455 Opened 4 years ago Closed 4 years ago

Restore the 100ms timer clamping min when Resistfingerprinting is enabled

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 + fixed
firefox59 + fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

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

Attachments

(1 file)

We regressed this, but it should be easy to fix with a std::min
Comment on attachment 8943700 [details]
Bug 1431455 Fix a regression for ResistFingerprinting: use the larger of the reduceTimerPrecision pref and the constant 100ms

https://reviewboard.mozilla.org/r/214100/#review219910

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:96
(Diff revision 3)
>  
> +inline double
> +TimerResolution()
> +{
> +  if(nsRFPService::IsResistFingerprintingEnabled())
> +    return max(100000.0, (double)sResolutionUSec);

Please use enclosing `{ }` around single line blocks.
Attachment #8943700 - Flags: review?(bkelly) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a88543d4c4ac
Fix a regression for ResistFingerprinting: use the larger of the reduceTimerPrecision pref and the constant 100ms r=bkelly
Keywords: checkin-needed
Too late for 58.0, but might be a ride along candidate.
https://hg.mozilla.org/mozilla-central/rev/a88543d4c4ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1431842
Comment on attachment 8943700 [details]
Bug 1431455 Fix a regression for ResistFingerprinting: use the larger of the reduceTimerPrecision pref and the constant 100ms

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

[User impact if declined]: Our small group of users with Resist Fingerprinting turned on have had their protections regressed.

[Is this code covered by automated tests?]: Yes

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

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: Bug 1431842 and Bug 1431425 both of which are test-only changes.

[Is the change risky?]: No

[Why is the change risky/not risky?]: While the code paths are taken when the functionality is disabled, it only changes behavior when a non-default, hidden pref is enabled. 

[String changes made/needed]: None
Attachment #8943700 - Flags: approval-mozilla-release?
I guess this impacts android too, right?
(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> I guess this impacts android too, right?

In theory yes, but we haven't really given a lot of consideration to the Resist Fingerprinting mode on Android. There are mostly like multiple problems with our current implementation there.
OK, do you think we should take this patch in an android dot-release?
thanks
(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> OK, do you think we should take this patch in an android dot-release?
> thanks

As a ride-along yea, I don't see why not.
Comment on attachment 8943700 [details]
Bug 1431455 Fix a regression for ResistFingerprinting: use the larger of the reduceTimerPrecision pref and the constant 100ms

safe ride-along for 58.0.2, Release58+
Attachment #8943700 - Flags: approval-mozilla-release? → approval-mozilla-release+
(In reply to Tom Ritter [:tjr] from comment #10)
> [User impact if declined]: Our small group of users with Resist
> Fingerprinting turned on have had their protections regressed.
> 
> [Is this code covered by automated tests?]: Yes
> 
> [Has the fix been verified in Nightly?]: Yes
> 
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Tom's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.