Closed Bug 1259831 Opened 6 years ago Closed 6 years ago

Regression: Web Audio cuts off when devtools is opened on the page.

Categories

(Core :: Web Audio, defect, P1)

46 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- verified

People

(Reporter: jujjyl, Assigned: padenot)

References

Details

(Keywords: regression)

Attachments

(2 files)

STR:

1. Visit http://floooh.github.io/oryol/asmjs/Paclone.html
2. Observe that audio is playing once the page loads.
3. Right-click on the blue page background, and click on "Inspect Element (Q)".

Observed: Once the devtools window pops up, the game continues to play as normal, but audio cuts off from the page completely. Closing the devtools window does not bring audio back.
Mozregression points to the following bisection range where this broke:

 6:34.48 INFO: Narrowed inbound regression window from [89c1cc2f, 8639f1a6] (3 revisions) to [89c1cc2f, 256e3e81] (2 revisions) (~1 steps left)
 6:34.48 INFO: Oh noes, no (more) inbound revisions :(
 6:34.48 INFO: Last good revision: 89c1cc2faea8903570688dd38d9f71ec0c5f0b87
 6:34.49 INFO: First bad revision: 256e3e81fed77b17ea9158786a349698bb59a6ab
 6:34.49 INFO: Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=89c1cc2faea8903570688dd38d9f71ec0c5f0b87&tochange=256e3e81fed77b17ea9158786a349698bb59a6ab

 6:35.60 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1132501

James, would you have a moment to peek at this?
Flags: needinfo?(jlong)
It's not a regression in the classic sense. All it does is attach the debugger earlier, when the toolbox opens instead of when the debugger is clicked. Previous to that patch you would see this behavior when the debugger is clicked.

This is a bug when how the audio behaves when a debugger is initially attached to the window. I have no idea why attached it cuts it off. That's definitely a platform problem.
Flags: needinfo?(jlong)
James, your commit was on January the 6th. I tried using an earlier build

https://archive.mozilla.org/pub/firefox/nightly/2016/01/2016-01-05-03-02-11-mozilla-central/firefox-46.0a1.en-US.win64.zip

and opened the console, switched to debugger tab, and clicked around to browse the different script files emsc.js, Paclone.html and Paclone.js, and tried adding a breakpoint to emsc.js line 8 (not hit), and another location in Paclone.js line 1. This did not cut the audio off. (once the breakpoint was hit, audio was naturally paused along with the execution, but resuming execution resumed the audio as well)

James, do you have a STR to repro in a build earlier from the commit on Jan 6th from bug 1132501? You write "Previous to that patch you would see this behavior when the debugger is clicked.", but I'm not sure which action that would exactly be?
I assumed it would happen because all the patch does is attach the thread earlier. I did not actually test it. I don't know why it wouldn't happen before the patch; there isn't any difference on how the thread connects, just timing.

I'm not the right person to debug this. We need to start in the platform in the audio code and see why it stops and go from there, and see why the debugger attachment is triggering it.
This looks like it works, but I need to test it more. Jukka, I'd have provided
builds for you to test, but try is closed, if you don't have a tree handy, I'll
get you build tomorrow or something.
Assignee: nobody → padenot
Status: NEW → ASSIGNED
We can do this now because authors can "suspend()" and "resume()" the context as needed, but we can also take the approach of finding the bad interaction between the auto-suspend and the suspend/resume the devtools are causing.
Component: Developer Tools → Developer Tools: Web Audio Editor
Component: Developer Tools: Web Audio Editor → Web Audio
Product: Firefox → Core
Priority: -- → P1
This is not really needed anymore now that suspend and resume are everywhere,
and simplifies the code quite a lot.
Attachment #8745311 - Flags: review?(karlt)
Comment on attachment 8745311 [details] [diff] [review]
Remove the auto-suspend logic for AudioContext

(In reply to Paul Adenot (:padenot) from comment #7)
> This is not really needed anymore now that suspend and resume are everywhere,
> and simplifies the code quite a lot.

I think auto-suspend is actually useful to reduce resource usage, and that
authors shouldn't need to suspend explicitly because the implementation knows
when there is no sound.

It is difficult for authors to know exactly when the sound will fade out.

However, as currently implemented, the auto-suspend is of limited use because
it does not happen when there are quiescent/suspended nodes anywhere in the
context, which is probably the case more often than not.  So, I guess it is
questionable whether it is worth having the code, if we are not going to make
it work in more cases.

Please remove the ExtraCurrentTime() declaration, if you'd like to go ahead and remove the definition.
Attachment #8745311 - Flags: review?(karlt) → review+
sorry had to back this out due the fact that bug 1268313 needed to be backed out for causing memory leaks and that push touched dom/media/webaudio/AudioDestinationNode.cpp - so this change here had to be backed out to make the backout of bug 1268313 possible, sorry paul!
Flags: needinfo?(padenot)
(clearing NI, I've re-landed this).
Flags: needinfo?(padenot)
https://hg.mozilla.org/mozilla-central/rev/d2f1d8369eae
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Duplicate of this bug: 1257407
Version: unspecified → 46 Branch
Flags: qe-verify+
Hi,
I have tested this issue using the latest Nightly (id: 20160904030201) build on:
- Windows 10 x64
- MacOS 10.12 Sierra Developer Beta 8
- Ubuntu 14.04 LTS x64

This is Verified Fixed on Mac and Windows but the sound DOESN'T work altogether on Ubuntu. 

Please note that the link in Comment #0 is broken but I have found the page at:
http://floooh.github.io/oryol-samples/asmjs/Paclone.html

Leaving the status flag as is for now.
This works for me on linux, ubuntu 16.04.
Since this only has a "status-firefox49: fixed" flag i have also tested it on the current Firefox RC 49.0 (id: 20160905130425) on:
- Windows 7 x64
- Ubuntu 16.04 x32
- Mac OSX 10.9.5

and the issue is still reproducible on all OS's using the STR in Comment #0 and the link:

http://floooh.github.io/oryol-samples/asmjs/Paclone.html
Paul, according to Comment 19, this is still an issue on Fx49 -- should we file a new bug or just reopen this one?
Flags: needinfo?(padenot)
This needs bug 1269741. It's too late in beta to take it, I believe, but the fix is in 50.
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #21)
> This needs bug 1269741. It's too late in beta to take it, I believe, but the
> fix is in 50.

Thank you for following up on this. Updating flags accordingly.
I agree with Paul that it's too late for Beta, and it won't affect most users (those not playing with devtools).  Plus, it can be worked around.
This issue is verified fixed on the latest Aurora 50.0a2 (2016-14-09), using Windows 10 x64, Ubuntu 16.04 X64 LTS and Mac OS X 10.11. 

---Marking flags here accordingly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.