Closed
Bug 1230533
Opened 9 years ago
Closed 9 years ago
Speech synthesis doesn't stop when the page is reloaded
Categories
(Core :: Web Speech, defect)
Tracking
()
VERIFIED
FIXED
mozilla46
People
(Reporter: cbadau, Assigned: eeejay)
References
()
Details
Attachments
(1 file, 2 obsolete files)
14.07 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Component: DOM → Web Speech
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → eitan
Comment 2•9 years ago
|
||
You must have expected to cc Makoto-san instead of me :-p
Comment 3•9 years ago
|
||
Indeed, sorry!
Assignee | ||
Comment 4•9 years ago
|
||
(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.
Assignee | ||
Comment 5•9 years ago
|
||
Also put in some safeguards in e10s if we fail to observe a window go away.
Attachment #8701643 -
Flags: review?(bugs)
Comment 6•9 years ago
|
||
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-
Assignee | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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-
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Reporter | ||
Comment 14•9 years ago
|
||
Verified fixed on Windows 7 64bit using latest Nightly 46.0a1 (buildID: 20160113141333).
Comment 15•9 years ago
|
||
Eitan, is bug 1252076 as dup of this? If so, why hasn't this patch been uplifted to the 45 train?
Flags: needinfo?(eitan)
Assignee | ||
Comment 16•9 years ago
|
||
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)
Comment 17•9 years ago
|
||
(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)
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•