654550 - Preference to disable video statistics

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Need a pref in about:config to disable video stats to reduce fingerprinting threat

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

The framerate and other information contains information about the user's machine's capabilities; users who want to be more anonymous will want to turn off entropy sources like this (and so will privacy-enhancing add-ons like torbutton) -- adding an about:config preference to turn it off enables this.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Can you tell me more about how this is an actual fingerprinting threat?

The data we have for statistics is basically frames parsed/decoded/presented/painted. Frames aren't painted if the video is not visible. But then the person who wanted to fingerprint could just use the Page Visibility API. Are we blocking that too?

Comment 4

[leonard.beck]

Attachment: Not finalized! Pref added, 0 returned when enabled, display TODO

Problem in /toolkit/content/widgets/videocontrols.xml for reading the pref value.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 760941 [details] [diff] [review]
Not finalized! Pref added, 0 returned when enabled, display TODO

Review of attachment 760941 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/HTMLVideoElement.cpp
@@ +160,5 @@
>  
>  uint32_t HTMLVideoElement::MozParsedFrames() const
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "Should be on main thread.");
> +  if (Preferences::GetBool("media.video_stats.disabled", false)) {

Checking this pref on each request will result in pretty poor performance. It would be better to cache the value of the pref.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I should note that I don't think this bug is a worthwhile pursuit until I hear more from Sid or Curtis about why this is an actual threat.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jared Wein [:jaws] from comment #3)
> Can you tell me more about how this is an actual fingerprinting threat?

To expand upon comment 2:  The stats in themselves are not a fingerprint, but they contribute to a fingerprint.  As we deploy new features, we should also enable add-ons and privacy-conscious users to reduce the number of features that can be used as a constellation of entropy sources to fingerprint them.

> But then the person who wanted to fingerprint could just use the
> Page Visibility API. Are we blocking that too?

I'm not asking for us to "block" video stats.  We should just create a pref for entropy-minimizing add-ons like Torbutton (and privacy-conscious individuals) to disable this source of entropy if they want.  We should probably have some (perhaps hidden) pref control the page visibility API too.

Mainly, the goal here is to reinforce our User Control privacy principle:
https://wiki.mozilla.org/Privacy/Principles#User_Control

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7)
> (In reply to Jared Wein [:jaws] from comment #3)
> > Can you tell me more about how this is an actual fingerprinting threat?
> 
> To expand upon comment 2:  The stats in themselves are not a fingerprint,
> but they contribute to a fingerprint.  As we deploy new features, we should
> also enable add-ons and privacy-conscious users to reduce the number of
> features that can be used as a constellation of entropy sources to
> fingerprint them.
> 
> > But then the person who wanted to fingerprint could just use the
> > Page Visibility API. Are we blocking that too?
> 
> I'm not asking for us to "block" video stats.  We should just create a pref
> for entropy-minimizing add-ons like Torbutton (and privacy-conscious
> individuals) to disable this source of entropy if they want.  We should
> probably have some (perhaps hidden) pref control the page visibility API too.
> 
> Mainly, the goal here is to reinforce our User Control privacy principle:
> https://wiki.mozilla.org/Privacy/Principles#User_Control

I understand the goal here, but we have to be practical about it too.

There are probably many other APIs that we have that are getting more usage of in fingerprinting circles and we could getter better outcomes out of.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Also, we should not be striving to make Firefox the next Tor. In fact, Torbutton is deprecated and they recommend using their patched version of Firefox.

Would it be better to submit a patch to the Tor repo that completely disables these statistics?

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Jared Wein [:jaws] from comment #8)
> I understand the goal here, but we have to be practical about it too.

Sure.  What are the trade-offs?  How much engineering time do we anticipate spending on implementing this?  What's the expected perf overhead of a cached pref?  If it's seemingly low-impact and low-effort, lets spend the energy on implementation and not discussion.

> There are probably many other APIs that we have that are getting more usage
> of in fingerprinting circles and we could getter better outcomes out of.

Probably, and we should enable controls for those too.

re: tor... I agree, but I would like to see us help them by implementing these controls in the main line.  Tor is not the only consumer of privacy settings...

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Should it be a separate pref for each type of privacy control, or a single pref that the user can flip?

If we go for separate prefs, then a user can get more fine-grained control. If we go for a single pref, then add-ons won't need to be updated when we want to guard another API using the same preference.

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

I like the idea of a one-size-fits-all pref, but due to the variety of entropy sources it's probably not actually one-size-fits all; I can imagine one add-on wanting a subset disabled but not all of 'em.

My opinion is that the onus is on the privacy tool implementation developers to decide which features they want and which they don't; if we do it for 'em, we will likely mess it up.

So perhaps the best way forward is to go for separate prefs now and then revisit as a longer-term effort to distill sets of prefs into one.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Ok, then let's go with separate prefs. But please update the patch to cache the pref value.

Comment 14

[leonard.beck]

We're on it.

Patch with cache attached in a couple of hours.

Our other question still remains: we can't read the pref value in videocontrols.xml (error: permission denied to access property 'classes'). Any idea? (See comment #241 in the current patch.)

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

You won't be able to read preferences from within videocontrols.xml since it runs with the same privileges as page content. Showing 0 here should be sufficient.

Comment 16

[leonard.beck]

Attachment: Pref added, 0 returned if enabled, pref value cached

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 761726 [details] [diff] [review]
Pref added, 0 returned if enabled, pref value cached

Review of attachment 761726 [details] [diff] [review]:
-----------------------------------------------------------------

Make sure to change the commit message to something like:

Bug xxxxxx - Add a preference to disable media statistics. r=reviewers

::: content/html/content/src/HTMLVideoElement.cpp
@@ +39,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +static bool mVideoStatsDisabled;

|mVideoStatsDisabled| (with a "m" prefix) is how we call class members.

Please call this |sVideoStatsDisabled| instead.

@@ +75,5 @@
>  HTMLVideoElement::HTMLVideoElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>    : HTMLMediaElement(aNodeInfo)
>  {
>    SetIsDOMBinding();
> +  Preferences::AddBoolVarCache(&mVideoStatsDisabled, "media.video_stats.disabled");

Actually, we want this done only once. Create a static HTMLVideoElement::Init static method, in which you register the pref.

Then call the HTMLVideoElement::Init method from LayoutStatics.cpp.

@@ +164,5 @@
>  uint32_t HTMLVideoElement::MozParsedFrames() const
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "Should be on main thread.");
> +  if (mVideoStatsDisabled)
> +    return 0;

Braces even on single if statement, please, and other occurrences below.

::: content/media/test/test_bug654550.html
@@ +15,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +  /* Test for Bug 654550 */
> +  

Remove trailing spaces in this file.

@@ +18,5 @@
> +  /* Test for Bug 654550 */
> +  
> +  var manager = new MediaTestManager;
> +
> +  function statsDisabled(video) {

Maybe this could be called |checkStatsDisabled|.

@@ +30,5 @@
> +  }
> +  
> +  function onplay(event) {
> +    var v = event.target;
> +    SpecialPowers.setBoolPref("media.video.stats_disabled", false);

You should make sure that time has actually advanced before checking that, so that we had the chance to paint/decode/etc. at least one frame. One way would be to do something along the lines of: 

>  function ontimeupate(e) {
>   var t = e.target;
>   if (t.currentTime == 0) {
>     return;
>   }
>   statsDisabled(t);
>   //disable pref, and do the other stuff.
>   document.removeEventListener("timeupdate", ontimeupdate);
> });
>
> document.addEventListener("timeupdate", ontimeupdate);

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Paul Adenot (:padenot) from comment #17)
> @@ +30,5 @@
> > +  }
> > +  
> > +  function onplay(event) {
> > +    var v = event.target;
> > +    SpecialPowers.setBoolPref("media.video.stats_disabled", false);
> 
> You should make sure that time has actually advanced before checking that,
> so that we had the chance to paint/decode/etc. at least one frame. One way
> would be to do something along the lines of: 

Using the MozAfterPaint event might be better.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 761726 [details] [diff] [review]
Pref added, 0 returned if enabled, pref value cached

Clearing review until updated patch is posted.

Comment 20

[leonard.beck]

Attachment: Patch updated: cache, test, coding style

Wi did'nt manage to use |MozAfterPaint| but it seems to work with |timeupdate|.
Test only on video format supporting video stats (.ogv and .webm).

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 762072 [details] [diff] [review]
Patch updated: cache, test, coding style

Review of attachment 762072 [details] [diff] [review]:
-----------------------------------------------------------------

Looks real good so far.

::: content/media/test/test_bug654550.html
@@ +18,5 @@
> +  /* Test for Bug 654550 */
> +
> +  var manager = new MediaTestManager;
> +
> +  //SpecialPowers.setBoolPref("dom.send_after_paint_to_content", true);

This line can be deleted.

@@ +24,5 @@
> +  function checkStatsDisabled(video) {
> +    if ((video.mozParsedFrames == 0)
> +        && (video.mozDecodedFrames == 0)
> +        && (video.mozPresentedFrames == 0)
> +        && (video.mozPaintedFrames == 0)) {

Replace these with calls to ok() such that if one of them fails we will know which one failed.

You can do it like so:
function checkStats(aVideo, aShouldBeEnabled) {
  is(!!video.mozParsedFrames, aShouldBeEnabled, "mozParsedFrames should be 0 if it is disabled");
  ...
}

@@ +36,5 @@
> +    if (SpecialPowers.getBoolPref("media.video_stats.disabled")) {
> +      ok(checkStatsDisabled(v), "All stats should be equal to 0");
> +      SpecialPowers.setBoolPref("media.video_stats.disabled", false);
> +    } else {
> +      var v = event.target;

This line shouldn't be necessary.

::: modules/libpref/src/init/all.js
@@ +222,5 @@
>  // MediaDecoderReader's mVideoQueue.
>  pref("media.video-queue.default-size", 10);
>  
> +// Whether to disable the video stats to prevent fingerprinting
> +pref("media.video_stats.disabled", false);

I would prefer the pref name be in the affirmative as it's usually easier to read negations of it. Please change it to:
pref("media.video_stats.enabled", true);

Comment 22

[leonard.beck]

Attachment: Patch updated: pref cached, test modified, coding style

Test slightly modified to catch the value of the interesting stats.
Ancient version running on test servers.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 762168 [details] [diff] [review]
Patch updated: pref cached, test modified, coding style

Review of attachment 762168 [details] [diff] [review]:
-----------------------------------------------------------------

Forwarding the review to Paul.

::: content/media/test/test_bug654550.html
@@ +33,5 @@
> +        "mozPresentedFrames should be 0 if stats are disabled");
> +      ok(!v.mozPaintedFrames,
> +        "mozPaintedFrames should be 0 if stats are disabled");
> +    }
> +      

nit: please remove the trailing whitespace here and on line 28.

Comment 24

[leonard.beck]

Attachment: Update: coding style

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 762221 [details] [diff] [review]
Update: coding style

Review of attachment 762221 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, just make the one change below. No need to re-request review from me.

::: content/media/test/test_bug654550.html
@@ +22,5 @@
> +  function checkStats(v, aShouldBeEnabled) {
> +    if (aShouldBeEnabled) {
> +      ok(v.mozParsedFrames + v.mozDecodedFrames +
> +         v.mozPresentedFrames + v.mozPaintedFrames != 0,
> +         "At least one value should be different from 0 if stats are enabled");

These values are all related. A frame can't be painted if it's not presented. A frame can't be presented if it's not decoded. A frame can't be decoded if it's not parsed. Thus, you should only need to check mozParsedFrames. See http://blog.pearce.org.nz/2011/03/html5-video-painting-performance.html for more background on this.

Comment 26

[bastien42]

Attachment: Patch updated

Patch corrected

Comment 27

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8710fcb40a32

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8710fcb40a32