Closed
Bug 1446346
Opened 6 years ago
Closed 6 years ago
Intermittent dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0.8999999985098839, first bad index: 0 with test-data offset 0 and expected-data offset 0; corresponding values 1 and 0.10000000149011612
Categories
(Core :: Web Audio, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox-esr60 | --- | fixed |
firefox59 | --- | unaffected |
firefox60 | --- | wontfix |
firefox61 | --- | fixed |
People
(Reporter: intermittent-bug-filer, Assigned: tjr)
References
Details
(Keywords: intermittent-failure, regression, Whiteboard: [stockwell disabled][Pernosco])
Attachments
(1 file, 5 obsolete files)
4.73 KB,
patch
|
jib
:
review+
ritu
:
approval-mozilla-beta-
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
Filed by: ncsoregi [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=168449240&repo=mozilla-central https://queue.taskcluster.net/v1/task/BYbI5lN3QXG1jwADghD7nw/runs/0/artifacts/public/logs/live_backing.log 04:22:27 INFO - TEST-START | dom/media/webaudio/test/test_audioParamLinearRamp.html 04:22:27 INFO - TEST-INFO | started process screencapture 04:22:27 INFO - TEST-INFO | screencapture: exit 0 04:22:27 INFO - Buffered messages logged at 04:22:27 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of channels for expected buffer 0 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of expected frames 04:22:27 INFO - Buffered messages finished 04:22:27 INFO - TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0.8999999985098839, first bad index: 0 with test-data offset 0 and expected-data offset 0; corresponding values 1 and 0.10000000149011612 --- differences - got 1535, expected +0 04:22:27 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 04:22:27 INFO - compareChannels@dom/media/webaudio/test/webaudio.js:78:3 04:22:27 INFO - compareBuffers@dom/media/webaudio/test/webaudio.js:103:5 04:22:27 INFO - testOutput/sp.onaudioprocess@dom/media/webaudio/test/webaudio.js:214:11 04:22:27 INFO - EventHandlerNonNull*testOutput@dom/media/webaudio/test/webaudio.js:211:9 04:22:27 INFO - runTestOnContext@dom/media/webaudio/test/webaudio.js:202:9 04:22:27 INFO - testOnNormalContext@dom/media/webaudio/test/webaudio.js:222:7 04:22:27 INFO - runTestFunction@dom/media/webaudio/test/webaudio.js:253:5 04:22:27 INFO - rval@SimpleTest/SimpleTest.js:146:17 04:22:27 INFO - EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:171:13 04:22:27 INFO - @SimpleTest/SimpleTest.js:1434:5 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of channels for expected buffer 0 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of expected frames 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of input buffer channels 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0, first bad index: -1 with test-data offset 0 and expected-data offset 0; corresponding values undefined and undefined --- differences 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of channels for expected buffer 0 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of expected frames 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of input buffer channels 04:22:27 INFO - TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0, first bad index: -1 with test-data offset 0 and expected-data offset 0; corresponding values undefined and undefined --- differences 04:22:27 INFO - GECKO(870) | MEMORY STAT | vsize 4246MB | residentFast 236MB | heapAllocated 22MB 04:22:27 INFO - TEST-OK | dom/media/webaudio/test/test_audioParamLinearRamp.html | took 104ms
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 4•6 years ago
|
||
There have been 42 failures in the last week. Summary: Intermittent dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0.8999999985098839, first bad index: 0 with test-data offset 0 and expected-data offset 0; corresponding values 1 and 0.10000000149011612 Affected platforms and builds: - windows10-64-qr / debug & opt : 13 - Linux x64 / debug & opt & pgo & asan: 8 - windows10-64 / pgo & opt: 4 - Windows 7 / debug & pgo: 4 - Linux / opt & debug: 4 - android-4-3-armv7-api16 / debug & opt: 4 - OS X 10.10 /debug & opt: 3 - linux64-qr / opt: 2 Recent log file and snippet with the failure: https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=170940706&lineNumber=61277 01:08:25 INFO - TEST-INFO | started process screenshot 01:08:25 INFO - TEST-INFO | screenshot: exit 0 01:08:25 INFO - Buffered messages logged at 01:08:25 01:08:25 INFO - 1949 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of channels for expected buffer 0 01:08:25 INFO - 1950 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of expected frames 01:08:25 INFO - Buffered messages finished 01:08:25 ERROR - 1951 INFO TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0.8999999985098839, first bad index: 0 with test-data offset 0 and expected-data offset 0; corresponding values 1 and 0.10000000149011612 --- differences - got 5, expected +0 01:08:25 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 01:08:25 INFO - compareChannels@dom/media/webaudio/test/webaudio.js:78:3 01:08:25 INFO - compareBuffers@dom/media/webaudio/test/webaudio.js:103:5 01:08:25 INFO - testOutput/sp.onaudioprocess@dom/media/webaudio/test/webaudio.js:214:11 01:08:25 INFO - EventHandlerNonNull*testOutput@dom/media/webaudio/test/webaudio.js:211:9 01:08:25 INFO - runTestOnContext@dom/media/webaudio/test/webaudio.js:202:9
Flags: needinfo?(drno)
Whiteboard: [stockwell needswork]
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 8•6 years ago
|
||
when this switches to disable-recommended, we should disable it- this might be related to bug 1446120.
Comment hidden (Intermittent Failures Robot) |
Comment 10•6 years ago
|
||
Attachment #8964897 -
Flags: review?(jmaher)
Comment 11•6 years ago
|
||
Comment on attachment 8964897 [details] [diff] [review] Skipped on linux and windows Review of attachment 8964897 [details] [diff] [review]: ----------------------------------------------------------------- we might have to end up skipping osx- lets see if this reduces the failures enough.
Attachment #8964897 -
Flags: review?(jmaher) → review+
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 14•6 years ago
|
||
Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc34ae9a1867 Disable test_audioParamLinearRamp.html on linux and windows for frequent failures. r=jmaher
Comment hidden (Intermittent Failures Robot) |
Updated•6 years ago
|
Keywords: leave-open
Whiteboard: [stockwell disable-recommended] → [stockwell disabled]
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc34ae9a1867
Comment hidden (Intermittent Failures Robot) |
Comment 18•6 years ago
|
||
Alex, it looks like in the middle of March a whole set of webaudio tests started failing. I'm hoping that they share a common root cause. Can I ask you to have an initial look of what could potentially have caused this?
Flags: needinfo?(drno) → needinfo?(achronop)
Updated•6 years ago
|
Assignee: nobody → achronop
Flags: needinfo?(achronop)
Comment 19•6 years ago
|
||
This is caused by the fuzzing on timer introduced recently because of Spectre [1]. Tom, this is making the Web Audio API rather hard to test and causes a very large number of intermittents, do you think it's feasible to flip the pref for testing, or would you have any concerns? [1]: https://searchfox.org/mozilla-central/source/dom/media/webaudio/AudioContext.cpp#673
Flags: needinfo?(tom)
Comment 20•6 years ago
|
||
Webaudio logs of this error: https://taskcluster-artifacts.net/Ef2STjUUREGuGlAgUi7EeA/0/public/logs/live_backing.log [task 2018-04-10T15:17:33.035Z] 15:17:33 INFO - TEST-START | dom/media/webaudio/test/test_audioParamLinearRamp.html ... [task 2018-04-10T15:17:33.343Z] 15:17:33 INFO - GECKO(2130) | [2181:Main Thread]: D/WebAudioAPI 0.000100: gain for 168 SetValueAtTime value=0.1 time=0.000100 constant=0 ... [task 2018-04-10T15:17:33.944Z] 15:17:33 INFO - TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0.8999999985098839, first bad index: 0 with test-data offset 0 and expected-data offset 0; corresponding values 1 and 0.10000000149011612 --- differences - got 5, expected +0
Assignee | ||
Comment 21•6 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be9ad9a0025276972f8ad194f0f67232ff7c4a0
Comment hidden (mozreview-request) |
Whiteboard: [stockwell disabled] → [stockwell disabled][Pernosco]
Comment 23•6 years ago
|
||
(In reply to Tom Ritter [:tjr] from comment #22) > Created attachment 8966627 [details] > Bug 1446346 Do not jitter time on several webaudio tests > > Review commit: https://reviewboard.mozilla.org/r/235346/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/235346/ Thanks. This should help, but I'm afraid we're playing whack-a-mole here.
Flags: needinfo?(tom)
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8966627 [details] Bug 1446346 If an AudioContext hasn't been started, do not bother clamping/jittering the CurrentTime https://reviewboard.mozilla.org/r/235346/#review241342
Attachment #8966627 -
Flags: review?(padenot) → review+
Comment 25•6 years ago
|
||
should we reenable the tests on linux/windows as well: https://hg.mozilla.org/mozilla-central/rev/fc34ae9a1867 Also as a note I see :roc added the [Pernosco] tag to this bug, there is an opportunity to debug this in the Pernosco toolchain as it has been reproduced in there. That should help reduce the whack-a-mole attempts here.
Comment 26•6 years ago
|
||
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #25) > Also as a note I see :roc added the [Pernosco] tag to this bug, there is an > opportunity to debug this in the Pernosco toolchain as it has been > reproduced in there. That should help reduce the whack-a-mole attempts here. roc contacted me by email with a Pernosco link for this one, which allowed me to quickly find the cause for this issue, but it was unclear to me how public Persnoco is at the minute so I didn't mention anything. The style of testing we've adopted to test the Web Audio API in Firefox very often requires that an `OfflineAudioContext` that has not been started via `startRendering` has a `currentTime` of zero. This seems like an invariant we can conserve even with the timer fuzzing. Now, it's unclear to me at the minute whether this also affects normal `AudioContext`s, maybe they have the same issue. In any case, we can certainly change our code so that the aforementioned invariant still holds. Tom, thoughts?
Flags: needinfo?(tom)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•6 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=403afaa20bff4bc4df2b6cfd79875706ae71b0ce
Flags: needinfo?(tom)
Comment hidden (mozreview-request) |
Comment 30•6 years ago
|
||
The WebAudio API defines that currentTime returns a value that is a multiple of 128/sampleRate. Even for sampleRate of 96k/s, that is 0.0013s. Is there a need to reduce resolution more than that (outside of the cases where prefs are changed because users do not want to follow the standards)? RFPOnly is not documented, but looks like is may be what is wanted here? Would it be sufficient to use that, if we ensure that sampleRate cannot be higher than 96k/s?
Flags: needinfo?(tom)
Comment 31•6 years ago
|
||
I wonder why testing/web-platform/tests/webaudio/the-audio-api/the-offlineaudiocontext-interface/current-time-block-size.html is not detecting this flaw.
Assignee | ||
Comment 32•6 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #30) > The WebAudio API defines that currentTime returns a value that is a multiple > of 128/sampleRate. > Even for sampleRate of 96k/s, that is 0.0013s. > > Is there a need to reduce resolution more than that (outside of the cases > where prefs are changed because users do not want to follow the standards)? > > RFPOnly is not documented, but looks like is may be what is wanted here? > > Would it be sufficient to use that, if we ensure that sampleRate cannot be > higher than 96k/s? The jittering we do provides additional defense against constructing a higher resolution timer using clock edge techniques or clock arbitrage; and given the ease at which one can construct multiple AudioContexts and get them all going at the same time I think that's important. I think/hope the patch I attached here will address a lot (if not all) of these bugs.
Flags: needinfo?(tom)
Assignee | ||
Comment 33•6 years ago
|
||
Comment on attachment 8964897 [details] [diff] [review] Skipped on linux and windows ># HG changeset patch ># User Eliza Balazs <ebalazs@mozilla.com> ># Parent dd31fb345c11732432fa61a7d8a1b727d41b4a1c >Bug 1446346 - Disable test_audioParamLinearRamp.html on linux and windows for frequent failures. r=jmaher > >diff --git a/dom/media/webaudio/test/mochitest.ini b/dom/media/webaudio/test/mochitest.ini >--- a/dom/media/webaudio/test/mochitest.ini >+++ b/dom/media/webaudio/test/mochitest.ini >@@ -69,6 +69,7 @@ tags=capturestream > [test_audioParamExponentialRamp.html] > [test_audioParamGain.html] > [test_audioParamLinearRamp.html] >+skip-if = os == "linux" || os == "win" #bug 1446346 > [test_audioParamSetCurveAtTime.html] > [test_audioParamSetCurveAtTimeTwice.html] > [test_audioParamSetCurveAtTimeZeroDuration.html]
Attachment #8964897 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Keywords: regression
Version: unspecified → 60 Branch
Comment 36•6 years ago
|
||
There is little value in applying such a jitter (1ms) that is less than the typical precision of currentTime (minimum of 2.6ms, but more likely at least 5.4ms), because the jittered time can be rounded back to the pre-jittered value. Even if jittering by a larger value, there are plenty of other techniques that can be used to detect the MSG (audio) processing intervals. If you are concerned about clock edge techniques, then a couple of options are available: 1) Jitter a delay on messages from the MSG thread to the main thread. Increasing latency either way would be sad, but this may be simpler than 2, for an interim solution. 2) Reorder the processing of messages on the main thread, so that an attacker can't aim to bracket the MSG message with its own events. We'd like to have a separate event queue for MSG messages anyway as that would reduce their latency, and so this is definitely the preferred solution.
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8967241 [details] Bug 1446346 Remove skips for webaudio tests that should now be fixed https://reviewboard.mozilla.org/r/235944/#review241764
Attachment #8967241 -
Flags: review?(padenot) → review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Updated•6 years ago
|
Comment 38•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c0900d55df7b If an AudioContext hasn't been started, do not bother clamping/jittering the CurrentTime r=padenot https://hg.mozilla.org/integration/autoland/rev/2b5a2f284d0f Remove skips for webaudio tests that should now be fixed r=padenot
Keywords: checkin-needed
Comment 39•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c0900d55df7b https://hg.mozilla.org/mozilla-central/rev/2b5a2f284d0f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 40•6 years ago
|
||
Looks like there was a failure on autoland after this landed :( https://treeherder.mozilla.org/logviewer.html#?repo=autoland&job_id=173413252
Flags: needinfo?(achronop)
Assignee | ||
Comment 42•6 years ago
|
||
Going to try to reproduce this with debugging information....
Flags: needinfo?(tom)
Comment 43•6 years ago
|
||
https://treeherder.mozilla.org/logviewer.html#?job_id=173628884&repo=mozilla-central&lineNumber=11475 23:13:08 INFO - 171 INFO TEST-START | dom/media/webaudio/test/test_audioParamLinearRamp.html 23:13:08 INFO - TEST-INFO | started process screenshot 23:13:08 INFO - TEST-INFO | screenshot: exit 0 23:13:08 INFO - Buffered messages logged at 23:13:08 23:13:08 INFO - 172 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of channels for expected buffer 0 23:13:08 INFO - 173 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of expected frames 23:13:08 INFO - Buffered messages finished 23:13:08 ERROR - 174 INFO TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0.8999999985098839, first bad index: 0 with test-data offset 0 and expected-data offset 0; corresponding values 1 and 0.10000000149011612 --- differences - got 999, expected +0 23:13:08 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 23:13:08 INFO - compareChannels@dom/media/webaudio/test/webaudio.js:78:3 23:13:08 INFO - compareBuffers@dom/media/webaudio/test/webaudio.js:103:5 23:13:08 INFO - testOutput/sp.onaudioprocess@dom/media/webaudio/test/webaudio.js:214:11 23:13:08 INFO - EventHandlerNonNull*testOutput@dom/media/webaudio/test/webaudio.js:211:9 23:13:08 INFO - runTestOnContext@dom/media/webaudio/test/webaudio.js:202:9 23:13:08 INFO - testOnNormalContext@dom/media/webaudio/test/webaudio.js:222:7 23:13:08 INFO - runTestFunction@dom/media/webaudio/test/webaudio.js:253:5 23:13:08 INFO - rval@SimpleTest/SimpleTest.js:146:17 23:13:08 INFO - EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:171:13 23:13:08 INFO - @SimpleTest/SimpleTest.js:1444:5 23:13:08 INFO - 175 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of channels for expected buffer 0 23:13:08 INFO - 176 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of expected frames 23:13:08 INFO - 177 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of input buffer channels 23:13:08 INFO - 178 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0, first bad index: -1 with test-data offset 0 and expected-data offset 0; corresponding values undefined and undefined --- differences 23:13:08 INFO - 179 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of channels for expected buffer 0 23:13:08 INFO - 180 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of expected frames 23:13:08 INFO - 181 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | Correct number of input buffer channels 23:13:08 INFO - 182 INFO TEST-PASS | dom/media/webaudio/test/test_audioParamLinearRamp.html | maxDifference: 0, first bad index: -1 with test-data offset 0 and expected-data offset 0; corresponding values undefined and undefined --- differences 23:13:08 INFO - GECKO(5504) | MEMORY STAT | vsize 438MB | vsizeMaxContiguous 720MB | residentFast 77MB | heapAllocated 14MB 23:13:08 INFO - 183 INFO TEST-OK | dom/media/webaudio/test/test_audioParamLinearRamp.html | took 81ms Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•6 years ago
|
Target Milestone: mozilla61 → ---
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 45•6 years ago
|
||
I got to the bottom of reopened Bug 1446346. I guess it's not surprising: This is still occurring because we (occasionally) will jitter up a CurrentTime of 0 that _has_ been started. I think karlt's approach is best at this point.
Attachment #8966627 -
Attachment is obsolete: true
Attachment #8967241 -
Attachment is obsolete: true
Attachment #8968653 -
Flags: review?(padenot)
Assignee | ||
Comment 46•6 years ago
|
||
Comment on attachment 8968653 [details] [diff] [review] Do not jitter if sampleRate is greater than our interval ># HG changeset patch ># User Tom Ritter <tom@mozilla.com> ># Date 1523997351 18000 ># Tue Apr 17 15:35:51 2018 -0500 ># Node ID 9e4d896dd34a201c84397d187a64995f7e1c11de ># Parent 9a37394c21c6a378815f1f9ae1eeefb3eb3db106 >Bug 1446346 Do not clamp or jitter the AudioContext's CurrentTime if its interval is larger than our precision r?padenot > >MozReview-Commit-ID: Bc1cto3pBKL > >diff --git a/dom/media/webaudio/AudioContext.cpp b/dom/media/webaudio/AudioContext.cpp >--- a/dom/media/webaudio/AudioContext.cpp >+++ b/dom/media/webaudio/AudioContext.cpp >@@ -663,27 +663,32 @@ AudioContext::DestinationStream() const > return nullptr; > } > > double > AudioContext::CurrentTime() > { > MediaStream* stream = Destination()->Stream(); > >- if (!mIsStarted && >- stream->StreamTimeToSeconds(stream->GetCurrentTime()) == 0) { >- return 0; >+ double rawTime = stream->StreamTimeToSeconds(stream->GetCurrentTime()); >+ >+ // CurrentTime increments in intervals of 128/sampleRate. If the Timer >+ // Precision Reduction is smaller than this interval, the jittered time >+ // can always be reversed to the raw step of the interval. In that case >+ // we can simply return the un-reduced time; and avoid breaking tests. >+ // We have to convert each variable into a common magnitude, we choose ms. >+ if ((128/mSampleRate) * 1000.0 > nsRFPService::TimerResolution() / 1000.0) { >+ return rawTime; > } > > // The value of a MediaStream's CurrentTime will always advance forward; it will never > // reset (even if one rewinds a video.) Therefore we can use a single Random Seed > // initialized at the same time as the object. > return nsRFPService::ReduceTimePrecisionAsSecs( >- stream->StreamTimeToSeconds(stream->GetCurrentTime()), >- GetRandomTimelineSeed()); >+ rawTime, GetRandomTimelineSeed()); > } > > void AudioContext::DisconnectFromOwner() > { > mIsDisconnecting = true; > Shutdown(); > DOMEventTargetHelper::DisconnectFromOwner(); > } >diff --git a/toolkit/components/resistfingerprinting/nsRFPService.cpp b/toolkit/components/resistfingerprinting/nsRFPService.cpp >--- a/toolkit/components/resistfingerprinting/nsRFPService.cpp >+++ b/toolkit/components/resistfingerprinting/nsRFPService.cpp >@@ -102,18 +102,19 @@ nsRFPService::GetOrCreate() > > ClearOnShutdown(&sRFPService); > sInitialized = true; > } > > return sRFPService; > } > >-inline double >-TimerResolution() >+/* static */ >+double >+nsRFPService::TimerResolution() > { > if(nsRFPService::IsResistFingerprintingEnabled()) { > return max(100000.0, (double)sResolutionUSec); > } > return sResolutionUSec; > } > > /* static */ >diff --git a/toolkit/components/resistfingerprinting/nsRFPService.h b/toolkit/components/resistfingerprinting/nsRFPService.h >--- a/toolkit/components/resistfingerprinting/nsRFPService.h >+++ b/toolkit/components/resistfingerprinting/nsRFPService.h >@@ -157,16 +157,17 @@ class nsRFPService final : public nsIObs > { > public: > NS_DECL_ISUPPORTS > NS_DECL_NSIOBSERVER > > static nsRFPService* GetOrCreate(); > static bool IsResistFingerprintingEnabled(); > static bool IsTimerPrecisionReductionEnabled(TimerPrecisionType aType); >+ static double TimerResolution(); > > enum TimeScale { > Seconds = 1, > MilliSeconds = 1000, > MicroSeconds = 1000000 > }; > > // The following Reduce methods can be called off main thread.
Attachment #8968653 -
Attachment is patch: true
Comment 47•6 years ago
|
||
Comment on attachment 8968653 [details] [diff] [review] Do not jitter if sampleRate is greater than our interval Review of attachment 8968653 [details] [diff] [review]: ----------------------------------------------------------------- Thanks you for this! ::: dom/media/webaudio/AudioContext.cpp @@ +667,5 @@ > { > MediaStream* stream = Destination()->Stream(); > + double rawTime = stream->StreamTimeToSeconds(stream->GetCurrentTime()); > + > + // CurrentTime increments in intervals of 128/sampleRate. If the Timer This is true at a spec level. In practice, we update in multiples of 128/sampleRate. With builds we ship (with default prefs), the finest resolution we have is on macbook pros, where it's 256. However, if this is acceptable to you, I think keeping it at 128/sampleRate is good, one less thing to think about when we align our implementation with the spec.
Attachment #8968653 -
Flags: review?(padenot) → review+
Updated•6 years ago
|
Assignee: achronop → tom
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 48•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/088d40fae53d Do not clamp or jitter the AudioContext's CurrentTime if its interval is larger than our precision. r=padenot
Keywords: checkin-needed
Comment 49•6 years ago
|
||
Backed out for failing browser/components/resistfingerprinting/test/mochitest/test_reduce_time_precision.html Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=088d40fae53da887d353577b5b855475a8617bf9 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=174398714&repo=mozilla-inbound&lineNumber=3333 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e277eb0ce37b3fa09b321f991d72d07f5c4202a
Flags: needinfo?(tom)
Assignee | ||
Comment 50•6 years ago
|
||
Attachment #8968653 -
Attachment is obsolete: true
Flags: needinfo?(tom)
Attachment #8969701 -
Flags: review?(padenot)
Assignee | ||
Comment 51•6 years ago
|
||
Corrected the test
Comment hidden (Intermittent Failures Robot) |
Updated•6 years ago
|
Attachment #8969701 -
Flags: review?(padenot) → review+
Assignee | ||
Comment 53•6 years ago
|
||
I had two lint issues....
Attachment #8969701 -
Attachment is obsolete: true
Comment 54•6 years ago
|
||
Comment on attachment 8970207 [details] [diff] [review] Do not jitter if sampleRate is greater than our interval Review of attachment 8970207 [details] [diff] [review]: ----------------------------------------------------------------- using includes() instead of indexOf()
Attachment #8970207 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 55•6 years ago
|
||
Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8981a88a2360 Do not clamp or jitter the AudioContext's CurrentTime if its interval is larger than our precision. r=jib
Keywords: checkin-needed
Comment 56•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8981a88a2360
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(In reply to Paul Adenot (:padenot) from comment #26) > roc contacted me by email with a Pernosco link for this one, which allowed > me to quickly find the cause for this issue, but it was unclear to me how > public Persnoco is at the minute so I didn't mention anything. It's not super secret, say whatever you need to say :-).
Comment 58•6 years ago
|
||
Please request Beta approval on this when you're comfortable doing so.
Flags: needinfo?(tom)
Updated•6 years ago
|
status-firefox59:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 59•6 years ago
|
||
Comment on attachment 8970207 [details] [diff] [review] Do not jitter if sampleRate is greater than our interval Approval Request Comment [Feature/Bug causing the regression]: Test Intermittents [User impact if declined]: None. [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]: None [Is the change risky?]: No [Why is the change risky/not risky?]: We're relaxing time precision rounding for AudioContext, which is not an invasive change. [String changes made/needed]: No
Flags: needinfo?(tom)
Attachment #8970207 -
Flags: approval-mozilla-beta?
Comment on attachment 8970207 [details] [diff] [review] Do not jitter if sampleRate is greater than our interval While we do want to fix intermittent test failures promptly, this is the last beta build and I'd prefer to let this one ride the 61 train.
Attachment #8970207 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 61•6 years ago
|
||
I'd like to keep this on the radar for 60.1 consideration during the next cycle after it's had some more bake time.
status-firefox-esr60:
--- → affected
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment 64•6 years ago
|
||
Comment on attachment 8970207 [details] [diff] [review] Do not jitter if sampleRate is greater than our interval Taking this now for ESR60 that it's had time to bake.
Attachment #8970207 -
Flags: approval-mozilla-esr60+
Comment 65•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/d06fb3839361 https://hg.mozilla.org/releases/mozilla-esr60/rev/c7e494e5888d
You need to log in
before you can comment on or make changes to this bug.
Description
•