1178738 - SpeechSynthesisErrorEvent is not integrated into the speech synthesis machine

Status: NEW

(Core :: Web Speech, defect)

Description

[kdavis]

As a result of  Bug 1167542 SpeechSynthesisErrorEvent is implemented but not 
integrated into the speech synthesis machine. In other words it exists but is 
never used.

Hence, SpeechSynthesisErrorEvent must be used in accord with the WebSpeech API

https://dvcs.w3.org/hg/speech-api/raw-file/tip/webspeechapi.html

Comment 1

[Eitan Isaacson [:eeejay]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5793b344706b

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1178738 - Update nsISpeechTask.DispatchError to provide error code. r?smaug

Review commit: https://reviewboard.mozilla.org/r/53746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53746/

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1178738 - Implement dispatch of SpeechSynthesisErrorEvent. r?smaug

Review commit: https://reviewboard.mozilla.org/r/53748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53748/

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1178738 - Have cancel() dispatch an "interrupted" error on spoken utterance. r?smaug

Review commit: https://reviewboard.mozilla.org/r/53750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53750/

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

Review commit: https://reviewboard.mozilla.org/r/53752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53752/

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1178738 - Update narrator to use error events when narration is stopped.

Review commit: https://reviewboard.mozilla.org/r/53754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53754/

Comment 7

[:Gijs (he/him)]

Comment on attachment 8754118 [details]
Bug 1178738 - Update narrator to use error events when narration is stopped.

https://reviewboard.mozilla.org/r/53754/#review50584

LGTM. It does seem like this will intermittently cause tests to fail if people run them locally and their audio device is busy or something, but I don't see what we can do about that.

Comment 8

[Eitan Isaacson [:eeejay]]

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8754118 [details]
> MozReview Request: Bug 1178738 - Update narrator to use error events when
> narration is stopped. r?Gijs
> 
> https://reviewboard.mozilla.org/r/53754/#review50584
> 
> LGTM. It does seem like this will intermittently cause tests to fail if
> people run them locally and their audio device is busy or something, but I
> don't see what we can do about that.

Luckily we don't use an audio device or the system speech service in the narrate tests.

Comment 9

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

Comment on attachment 8754114 [details]
MozReview Request: Bug 1178738 - Update nsISpeechTask.DispatchError to provide error code. r?smaug

https://reviewboard.mozilla.org/r/53746/#review50980

Comment 10

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

Comment on attachment 8754115 [details]
MozReview Request: Bug 1178738 - Implement dispatch of SpeechSynthesisErrorEvent. r?smaug

https://reviewboard.mozilla.org/r/53748/#review50982

Comment 11

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

Comment on attachment 8754116 [details]
MozReview Request: Bug 1178738 - Have cancel() dispatch an "interrupted" error on spoken utterance. r?smaug

https://reviewboard.mozilla.org/r/53750/#review50988

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm:166
(Diff revision 1)
>    // XXX: Provide more specific error messages
>    mTask->DispatchError(GetTimeDurationFromStart(), aIndex,
>      uint32_t(dom::SpeechSynthesisErrorCode::Synthesis_failed));
>  }
>  
>  void

This method is called on mainthread only, right?

::: dom/media/webspeech/synth/speechd/SpeechDispatcherService.cpp:255
(Diff revision 1)
>      case SPD_EVENT_RESUME:
>        mTask->DispatchResume((TimeStamp::Now() - mStartTime).ToSeconds(), 0);
>        break;
>  
>      case SPD_EVENT_CANCEL:
> +      mTask->DispatchError((TimeStamp::Now() - mStartTime).ToSeconds(), 0,

ditto for this method

::: dom/media/webspeech/synth/windows/SapiService.cpp:135
(Diff revision 1)
>    case SPEI_START_INPUT_STREAM:
>      mTask->DispatchStart();
>      break;
>    case SPEI_END_INPUT_STREAM:
>      if (mSpeakTextLen) {
> +      // mSpeakTextLen will be > 0 on any utterance except a cancel utterance.

tiny bit odd that the nSpeakTextLen has such double meaning, but fine.

Comment 12

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

Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

https://reviewboard.mozilla.org/r/53752/#review50990

Because of that, r-

::: dom/media/webspeech/synth/SpeechSynthesis.cpp:199
(Diff revision 1)
> -    // Remove all queued utterances except for current one, we will remove it
> -    // in OnEnd
> -    mSpeechQueue.RemoveElementsAt(1, mSpeechQueue.Length() - 1);
> -  } else {
> -    mSpeechQueue.Clear();
> +
> +  while (mSpeechQueue.Length() > startIndex) {
> +    RefPtr<SpeechSynthesisUtterance> utterance = mSpeechQueue.ElementAt(startIndex);
> +    mSpeechQueue.RemoveElementAt(startIndex);
> +
> +    utterance->DispatchSpeechSynthesisErrorEvent(

So what if some event listener adds more stuff to the queue? Dispatching events using the existing queue looks error prone.
Couldn't you swap the existing queue to some local variable and operate on that?

What happens if cancel() is called again while cancel() is being handled?

Comment 13

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/53752/#review50990

> So what if some event listener adds more stuff to the queue? Dispatching events using the existing queue looks error prone.
> Couldn't you swap the existing queue to some local variable and operate on that?
> 
> What happens if cancel() is called again while cancel() is being handled?

This is a good point!

Comment 14

[Eitan Isaacson [:eeejay]]

Attachment: MozReview Request: Bug 1178738 - Have SpeechSynthesis::mSpeechQueue not contain spoken utterance. r?smaug

Review commit: https://reviewboard.mozilla.org/r/55548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55548/

Comment 15

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754114 [details]
MozReview Request: Bug 1178738 - Update nsISpeechTask.DispatchError to provide error code. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53746/diff/1-2/

Comment 16

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754115 [details]
MozReview Request: Bug 1178738 - Implement dispatch of SpeechSynthesisErrorEvent. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53748/diff/1-2/

Comment 17

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754116 [details]
MozReview Request: Bug 1178738 - Have cancel() dispatch an "interrupted" error on spoken utterance. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53750/diff/1-2/

Comment 18

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53752/diff/1-2/

Comment 19

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754118 [details]
Bug 1178738 - Update narrator to use error events when narration is stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53754/diff/1-2/

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to Eitan Isaacson [:eeejay] from comment #14)
> Created attachment 8757030 [details]
> MozReview Request: Bug 1178738 - Have SpeechSynthesis::mSpeechQueue not
> contain spoken utterance. r?smaug
> 
> Review commit: https://reviewboard.mozilla.org/r/55548/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/55548/

Here is an additional patch..

This simplifies some of the queue manipulations while slightly sacrificing the encapsulation of nsSpeechTask.

Comment 21

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

Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

https://reviewboard.mozilla.org/r/53752/#review52708

::: dom/media/webspeech/synth/SpeechSynthesis.cpp:213
(Diff revision 2)
> +    utterance->DispatchSpeechSynthesisErrorEvent(
> +      0, 0, SpeechSynthesisErrorCode::Canceled);
> +  }
>  
>    if (mCurrentTask) {
>      mCurrentTask->Cancel();

So some event listener may have initiated a new task, which you end up cancelling here. That is somewhat unexpected.
Shouldn't you take a strong ref to the current task and call Cancel using that or something?

Comment 22

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

Comment on attachment 8757030 [details]
MozReview Request: Bug 1178738 - Have SpeechSynthesis::mSpeechQueue not contain spoken utterance. r?smaug

https://reviewboard.mozilla.org/r/55548/#review52710

::: dom/media/webspeech/synth/SpeechSynthesis.cpp:125
(Diff revision 1)
>    return mHoldQueue || (mCurrentTask && mCurrentTask->IsPrePaused()) ||
> -         (!mSpeechQueue.IsEmpty() && mSpeechQueue.ElementAt(0)->IsPaused());
> +         (utterance && utterance->IsPaused());
>  }
>  
>  bool
>  SpeechSynthesis::HasEmptyQueue() const

ok, so empty queue is about current task and speech queue... please document that somewhere.

Comment 23

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/55548/#review52710

> ok, so empty queue is about current task and speech queue... please document that somewhere.

I know I made that function, but TBH I am not sure why it is necessary. It looks like you could simply do !Pending() && !Speaking(). I'm going to propose that in a followup bug. In the meantime, I'll document HasEmptyQueue in SpeechSynthesis.h.

Comment 24

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/53752/#review52708

> So some event listener may have initiated a new task, which you end up cancelling here. That is somewhat unexpected.
> Shouldn't you take a strong ref to the current task and call Cancel using that or something?

Are you concerned mCurrentTask might be replaced by a new task from one of the listeners above, and then we call cancel() on the wrong one?

I don't think that is possible, mCurrentTask won't get replaced until it has "ended". If one of the listeners calls speak() it will wait and not advance the queue if mCurrentTask != null.

Comment 25

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/53752/#review52708

> Are you concerned mCurrentTask might be replaced by a new task from one of the listeners above, and then we call cancel() on the wrong one?
> 
> I don't think that is possible, mCurrentTask won't get replaced until it has "ended". If one of the listeners calls speak() it will wait and not advance the queue if mCurrentTask != null.

.. and if that were the case we would get in trouble even if we did what you suggest, because OnEnd would set the wrong task to null.

Comment 26

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

(In reply to Eitan Isaacson [:eeejay] from comment #24)

> I don't think that is possible, mCurrentTask won't get replaced until it has
> "ended". If one of the listeners calls speak() it will wait and not advance
> the queue if mCurrentTask != null.

What if there are nested cancel()/speak() calls?

Comment 27

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #26)
> (In reply to Eitan Isaacson [:eeejay] from comment #24)
> 
> > I don't think that is possible, mCurrentTask won't get replaced until it has
> > "ended". If one of the listeners calls speak() it will wait and not advance
> > the queue if mCurrentTask != null.
> 
> What if there are nested cancel()/speak() calls?

I'm adding some mochitests and finding interesting things.. so good catch :P

Comment 28

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754114 [details]
MozReview Request: Bug 1178738 - Update nsISpeechTask.DispatchError to provide error code. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53746/diff/2-3/

Comment 29

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754115 [details]
MozReview Request: Bug 1178738 - Implement dispatch of SpeechSynthesisErrorEvent. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53748/diff/2-3/

Comment 30

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754116 [details]
MozReview Request: Bug 1178738 - Have cancel() dispatch an "interrupted" error on spoken utterance. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53750/diff/2-3/

Comment 31

[Eitan Isaacson [:eeejay]]

Comment on attachment 8757030 [details]
MozReview Request: Bug 1178738 - Have SpeechSynthesis::mSpeechQueue not contain spoken utterance. r?smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55548/diff/1-2/

Comment 32

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53752/diff/2-3/

Comment 33

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754118 [details]
Bug 1178738 - Update narrator to use error events when narration is stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53754/diff/2-3/

Comment 34

[Eitan Isaacson [:eeejay]]

https://reviewboard.mozilla.org/r/53752/#review52708

> .. and if that were the case we would get in trouble even if we did what you suggest, because OnEnd would set the wrong task to null.

Changed it to have a stored list of canceled utterances to dispatch "canceled" to after current task ended, or immediately if there is no task.

Comment 35

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

Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

https://reviewboard.mozilla.org/r/53752/#review53724

::: dom/media/webspeech/synth/SpeechSynthesis.cpp:198
(Diff revision 3)
>  
>    return;
>  }
>  
>  void
> +SpeechSynthesis::DispatchToCanceledQueue() {

{ goes to its own line.

Comment 36

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754117 [details]
Bug 1178738 - Have utterances dispatch "canceled" error when they never start.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53752/diff/3-4/

Comment 37

[Eitan Isaacson [:eeejay]]

Comment on attachment 8754118 [details]
Bug 1178738 - Update narrator to use error events when narration is stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53754/diff/3-4/

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21944f25fbf0
Update nsISpeechTask.DispatchError to provide error code. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fe5dd5c3f33
Implement dispatch of SpeechSynthesisErrorEvent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c66c0ddf562
Have cancel() dispatch an "interrupted" error on spoken utterance. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab50796d2616
Have SpeechSynthesis::mSpeechQueue not contain spoken utterance. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c995bbdfa3a
Have utterances dispatch "canceled" error when they never start. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/54d1e8caf69b
Update narrator to use error events when narration is stopped. r=Gijs

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for intermittently crashing in test_global_queue_cancel.html with [@ RefPtr<mozilla::dom::GlobalQueueItem>::operator->]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0836d0b9fd52f4d82364a5facc8821f237f4f4a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/3756806c593a9c7e50adbb7bae37f24764190354
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d2e6a40be15cd9fcb17a3c5e8da11c8b8a85a27
https://hg.mozilla.org/integration/mozilla-inbound/rev/61db0bb716b2ec54fc79d12d69311115380a7c40
https://hg.mozilla.org/integration/mozilla-inbound/rev/f19dab43cca7b07844907792d861570933f3f500
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb32941f1d4633db39f4b1bd5bb3bca46071dd3f

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=29328280&repo=mozilla-inbound

Comment 40

[Eitan Isaacson [:eeejay]]

I'm having trouble pinpointing the issue. I'm going to remove this as a blocker for bug 1268633. No other browser supports SpeechSynthesisError events this yet.