1431455 - Restore the 100ms timer clamping min when Resistfingerprinting is enabled

Status: RESOLVED FIXED

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

Description

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

We regressed this, but it should be easy to fix with a std::min

Comment 1

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

Err std::max

Comment 2

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

Attachment: Bug 1431455 Fix a regression for ResistFingerprinting: use the larger of the reduceTimerPrecision pref and the constant 100ms

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

Comment 3

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

Comment on attachment 8943700 [details]
Bug 1431455 Fix a regression for ResistFingerprinting: use the larger of the reduceTimerPrecision pref and the constant 100ms

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

Comment 4

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

Comment on attachment 8943700 [details]
Bug 1431455 Fix a regression for ResistFingerprinting: use the larger of the reduceTimerPrecision pref and the constant 100ms

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

Comment on attachment 8943700 [details]
Bug 1431455 Fix a regression for ResistFingerprinting: use the larger of the reduceTimerPrecision pref and the constant 100ms

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Julien Cristau [:jcristau]]

Too late for 58.0, but might be a ride along candidate.

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/a88543d4c4ac

Comment 10

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

I guess this impacts android too, right?

Comment 12

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

(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.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

OK, do you think we should take this patch in an android dot-release?
thanks

Comment 14

[Tom Ritter [:tjr] (sec-approvals paused until after merge)]

(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 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/849c090094db

Comment 17

[Andrei Vaida [:avaida]]

(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.