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

RESOLVED FIXED in mozilla23

Status

()

Core
DOM
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla23
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 742853 [details]
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.
(Reporter)

Comment 1

5 years ago
Created attachment 742854 [details]
stack (gdb)

Comment 2

5 years ago
On Windows, opening a new tab is enough to make it crash: bp-7027a3ab-eeeb-4a1b-a3a0-8a2892130429.
Crash Signature: [@ nsRefreshDriver::Tick] → [@ nsRefreshDriver::Tick(long long, mozilla::TimeStamp) ] [@ nsRefreshDriver::Tick(__int64, mozilla::TimeStamp) ]
OS: Mac OS X → All
Hardware: x86_64 → All

Updated

5 years ago
Crash Signature: [@ nsRefreshDriver::Tick(long long, mozilla::TimeStamp) ] [@ nsRefreshDriver::Tick(__int64, mozilla::TimeStamp) ] → [@ nsRefreshDriver::Tick(long long, mozilla::TimeStamp)] [@ nsRefreshDriver::Tick(__int64, mozilla::TimeStamp)]

Comment 3

5 years ago
(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.
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.
Assignee: nobody → bzbarsky
Flags: needinfo?(bugs)

Comment 5

5 years ago
Performance has been there for ages now. I think we could just remove the pref.
Flags: needinfo?(bugs)
According to bug 866402 Tor may be using this pref.

I guess I'll just work around it.
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?
Flags: needinfo?(dveditz)
Created attachment 743852 [details] [diff] [review]
Remove the obsolete dom.enable_performance preference, so we can rely on things like performance.now() existing.
Attachment #743852 - Flags: review?(bugs)
Whiteboard: [need review]
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->...
  }
}
Attachment #743852 - Flags: review?(bugs) → review+
Made that change and https://hg.mozilla.org/integration/mozilla-inbound/rev/a6c9a09469c6
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/a6c9a09469c6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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.
Flags: needinfo?(dveditz)
Flags: needinfo?(mikeperry)

Comment 13

5 years ago
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...
Flags: needinfo?(mikeperry)
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?
Flags: needinfo?(mikeperry)

Comment 15

5 years ago
(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.
Flags: needinfo?(mikeperry)
> 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.
(Reporter)

Comment 17

5 years ago
(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.
Depends on: 870667
You need to log in before you can comment on or make changes to this bug.