File API leaks ms-resolution time
The following calls leak time to content scripts:
new File([], "").lastModifiedDate
new File([], "").lastModifiedActivity
Here are two patches for review:
https://github.com/arthuredelstein/tor-browser/commits/19478
979baf5c75605ad0dc15b8638baa02aaac835214 (fix) b72f23eb82153d95ecf553d65aea0b98b604d7fd (regression test)
Trac:
Keywords: TorBrowserTeam201506 deleted, TorBrowserTeam201506R added
Status: accepted to needs_review
Replying to gk:
Hm.
new Date.getTime()gives me something like 1467036079100 andMath.floor(new File([], "").lastModified / 100000) * 100000)1467036100000 or 1467036400000 or something similar. It seems your code is not rounding to 100ms?The units for mLastModificationDate are microseconds, so part of the patch will need to be changed.
I am not sure how to fix the tests to detect when values are rounded too much. Maybe by using a real file with a known modified date?
Replying to mcs:
Replying to gk:
Hm.
new Date.getTime()gives me something like 1467036079100 andMath.floor(new File([], "").lastModified / 100000) * 100000)1467036100000 or 1467036400000 or something similar. It seems your code is not rounding to 100ms?The units for mLastModificationDate are microseconds, so part of the patch will need to be changed.
Thanks everyone for having a look at this patch. I used 100000 exactly because that mLastModificationDate variable is in microseconds, so my feeling is the patch is correct. What part do you have in mind that needs to be changed?
Here are relevant lines from the mochitest on OS X. I think it is correctly rounding to the most recent 100 ms.
7 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new Date().getTime()' should be rounded to nearest 100 ms; saw 1467057044400 [snip] 13 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new File([], "").lastModified' should be rounded to nearest 100 ms; saw 1467057044500 14 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new File([], "").lastModifiedDate.getTime()' should be rounded to nearest 100 ms; saw 1467057044500I am not sure how to fix the tests to detect when values are rounded too much. Maybe by using a real file with a known modified date?
If we bind this behavior to a pref (perhaps in the upstreamed version) and then measure the same value with pref on and off, it should be possible to test that the two values are within 100 ms. But since we don't have that pref right now, I would be inclined to just do manual tests for now.
-
Replying to arthuredelstein:
Thanks everyone for having a look at this patch. I used 100000 exactly because that mLastModificationDate variable is in microseconds, so my feeling is the patch is correct. What part do you have in mind that needs to be changed?
Hmm. On second thought, I think your patch is correct. I will try to compile and run it later, unless GeKo has second thoughts about the problem he saw.
Replying to arthuredelstein:
Replying to mcs:
Replying to gk:
Hm.
new Date.getTime()gives me something like 1467036079100 andMath.floor(new File([], "").lastModified / 100000) * 100000)1467036100000 or 1467036400000 or something similar. It seems your code is not rounding to 100ms?The units for mLastModificationDate are microseconds, so part of the patch will need to be changed.
Thanks everyone for having a look at this patch. I used 100000 exactly because that mLastModificationDate variable is in microseconds, so my feeling is the patch is correct. What part do you have in mind that needs to be changed?
Here are relevant lines from the mochitest on OS X. I think it is correctly rounding to the most recent 100 ms. {{{ 7 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new Date().getTime()' should be rounded to nearest 100 ms; saw 1467057044400 [snip] 13 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new File([], "").lastModified' should be rounded to nearest 100 ms; saw 1467057044500 14 INFO TEST-PASS | tbb-tests/test_tor_bug1517.html | 'new File([], "").lastModifiedDate.getTime()' should be rounded to nearest 100 ms; saw 1467057044500 }}}
Interesting. I assumed testing in the scratchpad of an unpatched Tor Browser would be enough in a first pass. But it seems I was wrong for some reason. I've built the patch and verified that it is working. Thus, looks fine to me.
I am not sure how to fix the tests to detect when values are rounded too much. Maybe by using a real file with a known modified date?
If we bind this behavior to a pref (perhaps in the upstreamed version) and then measure the same value with pref on and off, it should be possible to test that the two values are within 100 ms. But since we don't have that pref right now, I would be inclined to just do manual tests for now.
Sounds good to me.