Closed Bug 1230533 Opened 4 years ago Closed 4 years ago

Speech synthesis doesn't stop when the page is reloaded

Categories

(Core :: Web Speech, defect)

45 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox45 --- affected
firefox46 --- verified

People

(Reporter: cbadau, Assigned: eeejay)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.
Component: DOM → Web Speech
I won't be surprised if this is a dupe of bug 1221443.
Blocks: 1003457
Assignee: nobody → eitan
You must have expected to cc Makoto-san instead of me :-p
Indeed, sorry!
(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.
Also put in some safeguards in e10s if we fail to observe a window go away.
Attachment #8701643 - Flags: review?(bugs)
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?
Attachment #8701643 - Flags: review?(bugs) → review-
(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.
Don't allow bfcache when there is an active or queued utterance in the window.
Attachment #8701643 - Attachment is obsolete: true
Attachment #8703796 - Flags: review?(bugs)
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.
Attachment #8703796 - Flags: review?(bugs) → review-
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?
Attachment #8703796 - Attachment is obsolete: true
Attachment #8703863 - Flags: review?(bugs)
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
Attachment #8703863 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/a51283b8a7d6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Verified fixed on Windows 7 64bit using latest Nightly 46.0a1 (buildID: 20160113141333).
Depends on: 1245011
Eitan, is bug 1252076 as dup of this? If so, why hasn't this patch been uplifted to the 45 train?
Flags: needinfo?(eitan)
Speech synth is still only preffed on in nightly. So I don't see the need to uplift patches. Am I wrong?
Flags: needinfo?(kairo)
(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.
Flags: needinfo?(kairo)
Flags: needinfo?(eitan)
Duplicate of this bug: 1252076
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.