866545 - Crash opening a new window with user_pref("dom.enable_performance", false);

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (requires pref)

1. Set:
     user_pref("dom.enable_performance", false);
2. Load the testcase
3. Click the button

I suspect this is a regression from bug 753453.  Topcrash bug 866402 has the same signature.

Comment 1

[Jesse Ruderman]

Attachment: stack (gdb)

Comment 2

[Scoobidiver (away)]

On Windows, opening a new tab is enough to make it crash: bp-7027a3ab-eeeb-4a1b-a3a0-8a2892130429.

Comment 3

[Robert Kaiser]

(In reply to Jesse Ruderman from comment #0)
> Topcrash bug 866402 has the same signature.

Yes, I guess this and that are dupes in some way.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Does anyone object to me just removing this preference at this point?

Olli, I think you reviewed this stuff initially, right?

The other obvious option is to just have the internal GetPerformance() always work, I guess.  I'm not sure whether that would produce useful results with the pref turned off, though.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Performance has been there for ages now. I think we could just remove the pref.

Comment 6

[Boris Zbarsky [:bzbarsky]]

According to bug 866402 Tor may be using this pref.

I guess I'll just work around it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I thought about this some more, and turning off window.performance per se makes no sense for Tor...  Just going to nuke the pref, and try to ask them what they actually want here.  dveditz, what's a good contact for that?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Remove the obsolete dom.enable_performance preference, so we can rely on things like performance.now() existing.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 743852 [details] [diff] [review]
Remove the obsolete dom.enable_performance preference, so we can rely on things like performance.now() existing.

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

::: layout/base/nsRefreshDriver.cpp
@@ +913,3 @@
>          } else {
>            timeStamp = 0;
>          }

Nit:

This would be cleaner to write as

DOMHighResTimestamp timeStamp = 0;
if (innerWindow && innerWindow->IsInnerWindow()) {
  if (nsPerformance* perf = innerWindow->GetPerformance()) {
    timeStamp = perf->...
  }
}

Comment 10

[Boris Zbarsky [:bzbarsky]]

Made that change and https://hg.mozilla.org/integration/mozilla-inbound/rev/a6c9a09469c6

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> I thought about this some more, and turning off window.performance per se
> makes no sense for Tor... Just going to nuke the pref, and try to ask them
> what they actually want here.  dveditz, what's a good contact for that?

Mike Perry would be a good place to start (CC'd, though I don't know if he gets/reads bugspam). I can only imagine fine-grained performance data could be used for fingerprinting, but seems like you can decent guesses at the same without the API.

Comment 13

[Mike Perry]

Yes, we are concerned about the fingerprinting issues involved with exposing rendering and network performance data directly and especially in high resolution. The high-resolution rendering performance metrics that this pref allows are a concern because they can be used to expedite performance fingerprinting. The high-resolution network performance metrics are also a concern for Tor, because they can be used to measure the user's latency and allow repeated observations (tracked by a cookie or an account) to make inferences about their Guard nodes and ultimately their location. They can also be used to more closely measure the difference between the server's clock and the user's clock, allowing for clock-drift fingerprints to be more effective as well. I can provide citations for these attacks if you need them...

Tor Browser has not yet done anything about JS timing-based fingerprints (first on my list is event.timeStamp), but we certainly would like a clean path to make all time measurements become low resolution, especially if we kill this pref.

If you want a "realistic" adversary for this specific situation: I think it is reasonable to assume an adserver will perform simple O(1) DOM property queries such as this to fingerprint you, but it is unlikely that that same adserver will run some costly JS performance benchmark suite that manually averages a large number of low-resolution timing measurements of the same activity over a long period of time - because such expensive amortized measurements will delay the display of any ad...

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK.  So just to be clear, you have no problem with the browser collecting the information and exposing it to extensions and other internal code; you just don't want to expose it to the web page, right?

Are you OK with exposing performance.now() to the web page?

If so, it seems like the right fix is not "have no performance object when the pref is set" (which is likely to break pages, especially as they start assuming the existence of a performance.now()) but either "have no performance.timing when the pref is set" or "have all the numbers in performance.timing be 0 when the pref is set".

For what it's worth, adding a pref (and it could even be called "dom.enable_performance") to control whether the performance object has a .timing is quite easy.  What about the .navigation on that object?

Comment 15

[Mike Perry]

(In reply to Boris Zbarsky (:bz) from comment #14)
> OK.  So just to be clear, you have no problem with the browser collecting
> the information and exposing it to extensions and other internal code; you
> just don't want to expose it to the web page, right?

Yes, that's correct.

> Are you OK with exposing performance.now() to the web page?

What is performance.now()? I don't see it in either URL for the docs: https://developer.mozilla.org/en-US/docs/Navigation_timing or https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.html#sec-navigation-info-interface

Is it a higher-precision alias for Date.now()? If so, I eventually do want to figure out how to reduce the precision on Date.now() and other millisecond JS timesources (like event.timeStamp). It's on the TODO list, but below other fingerprinting and general Tor Browser issues. See https://trac.torproject.org/projects/tor/ticket/3059.

> If so, it seems like the right fix is not "have no performance object when
> the pref is set" (which is likely to break pages, especially as they start
> assuming the existence of a performance.now()) but either "have no
> performance.timing when the pref is set" or "have all the numbers in
> performance.timing be 0 when the pref is set".

Hrmm.. Yeah, I suppose 0 might be better for all fields to avoid throwing exceptions?

> For what it's worth, adding a pref (and it could even be called
> "dom.enable_performance") to control whether the performance object has a
> .timing is quite easy.  What about the .navigation on that object?

I don't see any fingerprinting or serious privacy issues with window.performance.navigation.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> What is performance.now()

See http://www.w3.org/TR/hr-time/

The performance object has all sorts of things hanging off it, with more being added.  performance.timing has load-timing information, but performance.now() has nothing to do with that, and neither does performance.navigation.  And there are, as I said, more things being added under performance.

Comment 17

[Jesse Ruderman]

(In reply to Boris Zbarsky (:bz) from comment #14)

> If so, it seems like the right fix is not "have no performance object when
> the pref is set" (which is likely to break pages, especially as they start
> assuming the existence of a performance.now()) but either "have no
> performance.timing when the pref is set" or "have all the numbers in
> performance.timing be 0 when the pref is set".

Done in bug 870667.