1230533 - Speech synthesis doesn't stop when the page is reloaded

Status: VERIFIED FIXED

(Core :: Web Speech, defect)

Description

[Camelia Badau [:cbadau], Desktop QA]

Reproducible on latest Nightly 45.0a1 (buildID: 20151203053521) 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0

Steps to reproduce: 
1. Ensure that the speech synthesis is enabled (media.webspeech.synth.enabled must be true)
2. Go to http://codepen.io/matt-west/pen/wGzuJ
3. Type some long text. 
4. Click the 'Speak' button
5. Why the synthesis is running reload the page. 

Expected results: Speech synthesis stops. 

Actual results: Speech synthesis doesn't stop. The synthesis continues even if the page was reloaded.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

I won't be surprised if this is a dupe of bug 1221443.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

You must have expected to cc Makoto-san instead of me :-p

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Indeed, sorry!

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> I won't be surprised if this is a dupe of bug 1221443.

I don't think it is. We never tell the external service to stop speaking after page reload/tab close.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Cancel speech when controlling window goes away.

Also put in some safeguards in e10s if we fail to observe a window go away.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8701643 [details] [diff] [review]
Cancel speech when controlling window goes away.

> SpeechSynthesis::SpeechSynthesis(nsPIDOMWindow* aParent)
>   : mParent(aParent)
>   , mHoldQueue(false)
>+  , mInnerID(0)
> {
>   MOZ_ASSERT(aParent->IsInnerWindow());
>+  mInnerID = aParent->WindowID();
>+
>+  if (NS_IsMainThread()) {
We sure can't execute this code in non-main thread, so just make that check MOZ_ASSERT.


But don't we still have the issue with page going into bfache? inner window isn't destroyed then.
That isn't about reload but about loading a new page, sure, but somewhat similar issue.

Do we need to add something to nsDocument::CanSavePresentation so that documents using tts don't ever go to bfcache?
Or could we observe DOM_WINDOW_FROZEN_TOPIC/DOM_WINDOW_THAWED_TOPIC and when window is frozen, we pause the tts?

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #6)
> 
> But don't we still have the issue with page going into bfache? inner window
> isn't destroyed then.

Good point!

> That isn't about reload but about loading a new page, sure, but somewhat
> similar issue.

Yes, this belongs here.

> 
> Do we need to add something to nsDocument::CanSavePresentation so that
> documents using tts don't ever go to bfcache?

Maybe if the window has a non-empty speech queue it would return false? I think that is a decent requirement.

> Or could we observe DOM_WINDOW_FROZEN_TOPIC/DOM_WINDOW_THAWED_TOPIC and when
> window is frozen, we pause the tts?

Pausing I think would be bad, because it would hold up the global queue if there is one.

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Cancel speech when controlling window goes away.

Don't allow bfcache when there is an active or queued utterance in the window.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8703796 [details] [diff] [review]
Cancel speech when controlling window goes away.


>   mozilla::dom::SpeechSynthesis*
>     GetSpeechSynthesis(mozilla::ErrorResult& aError);
>+  bool HasActiveSpeech();
I think this should be something like
HasActiveSpeechSynthesis(). (to me "speech" is no more synthesis than recognition)


> SpeechSynthesis::SpeechSynthesis(nsPIDOMWindow* aParent)
>   : mParent(aParent)
>   , mHoldQueue(false)
>+  , mInnerID(0)
> {
>   MOZ_ASSERT(aParent->IsInnerWindow());
>+  mInnerID = aParent->WindowID();
>+
>+  if (NS_IsMainThread()) {
We don't execute this ever on main thread (we'd crash when addreffing aParent), so just have assertion here, no if check


>+    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>+    if (obs) {
>+      obs->AddObserver(this, "inner-window-destroyed", false);
So as long as the inner window is alive, 'this' is kept alive too, since RemoveObserver is called only in ::Observe.
Since observer service is a singleton, adding any cycle collector stuff here wouldn't really help here, but I think SpeechSynthesis should
just support weakreferences and then you could pass true-as-last-param to AddObserver.
Extending nsSupportsWeakReference + adding the right stuff to QI is rather simple.

r- because of that memory handling.

Comment 10

[Eitan Isaacson [:eeejay]]

Attachment: Cancel speech when controlling window goes away.

Don't allow bfcache when there is an active or queued utterance in the window.

I ended up needing to use NS_ISUPPORTS_CAST because the compiler was complaining about ambigious inheritence. Did I do that right?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8703863 [details] [diff] [review]
Cancel speech when controlling window goes away.

> SpeechSynthesis::SpeechSynthesis(nsPIDOMWindow* aParent)
>   : mParent(aParent)
>   , mHoldQueue(false)
>+  , mInnerID(0)
You could just mInner(aParent->WindowID())

>+SpeechSynthesis::Observe(nsISupports* aSubject, const char* aTopic,
>+                 const char16_t* aData)
align the last param

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a51283b8a7d6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a51283b8a7d6

Comment 14

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit using latest Nightly 46.0a1 (buildID: 20160113141333).

Comment 15

[Robert Kaiser]

Eitan, is bug 1252076 as dup of this? If so, why hasn't this patch been uplifted to the 45 train?

Comment 16

[Eitan Isaacson [:eeejay]]

Speech synth is still only preffed on in nightly. So I don't see the need to uplift patches. Am I wrong?

Comment 17

[Robert Kaiser]

(In reply to Eitan Isaacson [:eeejay] from comment #16)
> Speech synth is still only preffed on in nightly. So I don't see the need to
> uplift patches. Am I wrong?

Oh, OK, then it's fine, yes, sounded to me like the QE folks were saying that this is a feature to go out with this train. If it's not (off by default), everything is fine.