Closed Bug 1401678 Opened 2 years ago Closed 2 years ago

[e10s] No sound alert when using Find in page (accessibility.typeaheadfind.enablesound)

Categories

(Core :: Find Backend, defect, P4)

55 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- verified

People

(Reporter: mail, Assigned: mikedeboer)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053622

Steps to reproduce:

I have set Firefox to automatically search for text in a page when I start typing. When typing a text that's not on the page, Firefox used to make a (system) "error" sound, so you would know that the text was not on the page, even when you were not looking at the screen. This sound is not being played anymore.

To reproduce: 
Search for a text on a page, and write something that is not on the page.

I have tried on two different computers, on the 64 bit and the 32 bit version of Firefox, using my old profile and using a completely new profile and on a clean install of Firefox without luck.

In about_config I have checked, that 
accessibility.typeaheadfind.soundURL is set to "beep" (I have also tried inserting a path to a wav file without result)
accessibility.typeaheadfind.enablesound set to true

I have also made sure that the Windows system sound in question is present and activated.

A curious detail is, that if you install the Blocksite Plus extension ( https://addons.mozilla.org/da/firefox/addon/blocksiteplus/?src=api  ), the sound works. This is even the case if you do it on a completely new profile in a fresh install.

Blocksite Plus has not, however, been installed on one of the two computers, I have tested the problem on, so it is not a case of some "leftovers" from an old installation messing up Firefox.

I have described the problem in the forums here as well: https://support.mozilla.org/en-US/questions/1176157


Actual results:

No sound was played


Expected results:

Firefox should play a sound.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
Ever confirmed: true
Keywords: regression
Summary: No sound alert when using Find in page function → [e10s] No sound alert when using Find in page (accessibility.typeaheadfind.enablesound)
Component: Untriaged → Find Toolbar
Product: Firefox → Toolkit
Ough, 'sounds' like a defect indeed! Thanks for filing!

Perhaps Jim know why the 'Beep' sound is not played anymore? I only checked Windows 7 and 10 at the moment, don't have more flavors at my direct disposal right now.
Component: Find Toolbar → Find Backend
Flags: needinfo?(jmathies)
Priority: -- → P1
Product: Toolkit → Core
Priority: P1 → P4
(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Ough, 'sounds' like a defect indeed! Thanks for filing!
> 
> Perhaps Jim know why the 'Beep' sound is not played anymore? I only checked
> Windows 7 and 10 at the moment, don't have more flavors at my direct
> disposal right now.

No idea, but I'd bet e10s related changes might be involved. A regression range would help.
Flags: needinfo?(jmathies)
I found the regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ecf20a2484b6&tochange=d812f80a0f1d

No sound in m-c 2014-02-11 with e10s on, and 2014-02-10 sound good.

bug 964209 is relevant?
Thanks Yang! I think bug 966467 is the culprit, actually, since it scopes the sound service to the main process only.
Flags: needinfo?(wmccloskey)
That seems like something we should be remoting. I won't be able to work on this, but it shouldn't be too hard to remote everything in nsISound.idl. It looks like beep() is the method used for the findbar.
Flags: needinfo?(wmccloskey)
'Sounds' like a fun new kind of thing for me to take a stab at ;-)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: qe-verify+
Comment on attachment 8927405 [details]
Bug 1401678 - Proxy nsISound::beep() and nsISound::play() calls from the content process to the parent process and beep away there.

https://reviewboard.mozilla.org/r/198714/#review203822


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: widget/nsSoundProxy.cpp:16
(Diff revision 1)
> +using namespace mozilla;
> +using namespace mozilla::dom;
> +
> +NS_IMPL_ISUPPORTS(nsSoundProxy, nsISound)
> +
> +nsSoundProxy::nsSoundProxy()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]
Comment on attachment 8927405 [details]
Bug 1401678 - Proxy nsISound::beep() and nsISound::play() calls from the content process to the parent process and beep away there.

https://reviewboard.mozilla.org/r/198714/#review203890

This looks good, but I'm going to r- out of caution.

::: dom/ipc/ContentParent.cpp:2687
(Diff revision 1)
> +
> +  nsresult rv;
> +  nsCOMPtr<nsISound> sound(do_GetService(NS_SOUND_CID, &rv));
> +  NS_ENSURE_SUCCESS(rv, IPC_OK());
> +
> +  sound->Play(soundURL);

I'm worried about the security of this. Using this API, the content process can ask the parent to download an arbitrary URL, interpret it as a sound file, and play it. That seems pretty dangerous.

I think it would be better to sanitize the URL somehow. Perhaps you could check either that it's a chrome URI or that it's equal to the value of some pref (accessibility.typeaheadfind.soundURL for example)?
Attachment #8927405 - Flags: review?(wmccloskey) → review-
Triaged on 12/6. ni on dolske for next steps here, since Mike it out until January.
Flags: needinfo?(dolske)
Flags: needinfo?(dolske)
(In reply to Bill McCloskey [inactive unless it's an emergency] (:billm) from comment #9)
> I'm worried about the security of this. Using this API, the content process
> can ask the parent to download an arbitrary URL, interpret it as a sound
> file, and play it. That seems pretty dangerous.

This sound proxy is available to chrome privileged scripts regardless, right? If not, how can I make it behave that way?

> I think it would be better to sanitize the URL somehow. Perhaps you could
> check either that it's a chrome URI or that it's equal to the value of some
> pref (accessibility.typeaheadfind.soundURL for example)?

I'd like to filter on chrome URIs.

Since you're inactive atm, I'll find someone to forward this request to ;)
Comment on attachment 8927405 [details]
Bug 1401678 - Proxy nsISound::beep() and nsISound::play() calls from the content process to the parent process and beep away there.

https://reviewboard.mozilla.org/r/198714/#review218210

r=me with comments addressed.  Thanks.

::: dom/ipc/ContentParent.cpp:2680
(Diff revision 2)
> +ContentParent::RecvPlaySound(const URIParams& aURI)
> +{
> +  nsCOMPtr<nsIURI> soundURI = DeserializeURI(aURI);
> +  bool isChrome = false;
> +  // Only allow playing a chrome:// URL from the content process.
> +  if (!soundURI || NS_FAILED(soundURI->SchemeIs("chrome", &isChrome)) || !isChrome) {

If you want to harden this a bit you could:

1. Validate the URL before calling SendPlaySound() in the content process and avoid sending the IPC message at all if it fails
2. Then do the check again here and if it fails call ContentParent::KillHard() since we've detect a spoofed message.

FWIW, I filed bug 1430159 to investigate making this easier in IPC.

::: dom/ipc/PContent.ipdl:852
(Diff revision 2)
>      sync ClipboardHasType(nsCString[] aTypes, int32_t aWhichClipboard)
>          returns (bool hasType);
>  
> +    // 'Play' and 'Beep' are the only nsISound methods used in the content process.
> +    async PlaySound(URIParams aURL);
> +    async Beep();

It might be worth using `compress` keyword on `Beep()` or maybe both messages.  If we get spammed with them at least we won't queue up a very long list of sounds to play.

::: widget/nsSoundProxy.cpp:19
(Diff revision 2)
> +NS_IMPL_ISUPPORTS(nsSoundProxy, nsISound)
> +
> +NS_IMETHODIMP
> +nsSoundProxy::Play(nsIURL *aURL)
> +{
> +  nsCOMPtr<nsIURI> soundURI(do_QueryInterface(aURL));

Can do `MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Content)` in the proxied methods?

::: widget/nsSoundProxy.cpp:27
(Diff revision 2)
> +  ContentChild::GetSingleton()->SendPlaySound(soundParams);
> +  return NS_ERROR_FAILURE;
> +}
> +
> +NS_IMETHODIMP
> +nsSoundProxy::PlaySystemSound(const nsAString &aSoundAlias)

It doesn't look like we have any consumers of nsISound.playSystemSound() any more.  Can you file a follow-up to remove it?

::: widget/nsSoundProxy.cpp:44
(Diff revision 2)
> +
> +NS_IMETHODIMP
> +nsSoundProxy::Init()
> +{
> +  MOZ_ASSERT(false, "Init is unimplemented.");
> +  return NS_ERROR_FAILURE;

NS_ERROR_NOT_IMPLEMENTED here and elsewhere in this file.

::: widget/nsSoundProxy.cpp:50
(Diff revision 2)
> +}
> +
> +NS_IMETHODIMP
> +nsSoundProxy::PlayEventSound(uint32_t aEventId)
> +{
> +  MOZ_ASSERT(false, "PlayEventSound is unimplemented.");

I feel like either we should proxy nsISound.playEventSound or we should have a stronger assertion with a comment here.  Like MOZ_DIAGNOSTIC_ASSERT() and something like "only called by XUL in the parent process".
Attachment #8927405 - Flags: review?(bkelly) → review+
Comment on attachment 8927405 [details]
Bug 1401678 - Proxy nsISound::beep() and nsISound::play() calls from the content process to the parent process and beep away there.

https://reviewboard.mozilla.org/r/198714/#review218210

> It doesn't look like we have any consumers of nsISound.playSystemSound() any more.  Can you file a follow-up to remove it?

Filed bug 1430179.
Thanks Ben, for the speedy review! :)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19af171d7718
Proxy nsISound::beep() and nsISound::play() calls from the content process to the parent process and beep away there. r=bkelly
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e6c23942e9b
Proxy nsISound::beep() and nsISound::play() calls from the content process to the parent process and beep away there. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/1e6c23942e9b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Not a new regression, we're past rc1 for 58, wontfix.
Flags: needinfo?(mdeboer)
I reproduced the issue using Firefox Nightly 57.0a1 (2017-09-20) on Windows 10 x64, Ubuntu 16.04 x64.
Verified fixed using the latest Nightly 59.0a1 (Build ID:20180122100120) on Windows 10 x64 and Mac OS X 10.12.

The issue is still reproducible on Ubuntu 16.04 x64 using the latest Nightly 59.0a1 (Build ID:20180122100120).

Mike, should I log a new bug for Ubuntu?
Flags: needinfo?(mdeboer)
Thanks for the verification, Roxana! Please log a new bug for the Linux audio issue.
Flags: needinfo?(mdeboer)
Filed bug 1432200 for Linux issue.

Based on comment 23 I will mark this bug Verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.