863852 - Speech Recognition is not prepared to handle reentrance

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Guilherme Gonçalves [:ggp]]

The state machine behind speech recognition is driven by events implemented with nsRunnables. These events can be queued by calls to speech recognition methods.

By calling one such method and then window.showModalDialog() inside the handler for a DOM speech recognition event it is possible to cause the state machine events run and re-enter the state machine code, which causes an assertion.

Comment 1

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Log event and state names rather than codes

A logging patch that makes it a little easier to debug problems like this in the future.

Comment 2

[Guilherme Gonçalves [:ggp]]

Attachment: Part 2 - Handle reentrance in the speech recognition state machine

This patch handles reentrance by basically rewriting the transition functions to change to the desired state as soon as possible, and dispatch DOM events as late as possible.

There are some harder cases, such as StartRecording and AbortSilently, which dispatch two DOM events in sequence. I included a test for the latter, but we probably need broader testing than that.

Comment 3

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

Comment on attachment 739775 [details] [diff] [review]
Part 1 - Log event and state names rather than codes

>+SpeechRecognition::GetName(FSMState aId)
>+{
>+  const char* names[] = {
static const char* names[]

>+    "STATE_IDLE",
>+    "STATE_STARTING",
>+    "STATE_ESTIMATING",
>+    "STATE_WAITING_FOR_SPEECH",
>+    "STATE_RECOGNIZING",
>+    "STATE_WAITING_FOR_RESULT"
>+  };
could you add something like
MOZ_ASSERT(ArrayLength(names) == FSMSTATE::STATE_COUNT + 1)
and add STATE_COUNT to the enum


>+SpeechRecognition::GetName(SpeechEvent* aEvent)
Something similar for this.

That way we don't start overflowing the arrays so easily 
if we add new states or events

Comment 4

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

Comment on attachment 739783 [details] [diff] [review]
Part 2 - Handle reentrance in the speech recognition state machine

Could you actually make SET_STATE a real method. Same with STATE_BETWEEN.
And STATE_EQUALS looks pretty useless.

I know some of them are aren't added in this patch just started to think about the readability of this code.


> SpeechRecognition::StartedAudioCapture(SpeechEvent* aEvent)
> {
>+  SET_STATE(STATE_ESTIMATING);
>+
>   mEndpointer.SetEnvironmentEstimationMode();
>   mEstimationSamples += ProcessAudioSegment(aEvent->mAudioSegment);
> 
>-  DispatchTrustedEvent(NS_LITERAL_STRING("start"));
>   DispatchTrustedEvent(NS_LITERAL_STRING("audiostart"));
>-
>-  return STATE_ESTIMATING;
>+  if (STATE_EQUALS(STATE_ESTIMATING)) {
>+    DispatchTrustedEvent(NS_LITERAL_STRING("start"));
>+  }
> }
hmm, you change the order of events. Looks wrong to me.


>+    if (STATE_EQUALS(STATE_RECOGNIZING)) {
>+      // FIXME: StopRecordingAndRecognize should only be called for single
>+      // shot services for continous we should just inform the service
continuous 


r- because of "start"

Comment 5

[Guilherme Gonçalves [:ggp]]

(In reply to Olli Pettay [:smaug] from comment #4)
> hmm, you change the order of events. Looks wrong to me.

This is meant to preserve the dependence between "audioend" and "audiostart".

If, for example, the handler for "audiostart" calls stop() and spins the event loop, we will stop recording and issue "audioend". Without changing the order of the events, calling stop() from the "start" handler would make "audioend" be issued before "audiostart".

Please correct me if I'm wrong, but the spec does seem to allow "audiostart" before "start". Did you see a cleaner way to do this?

Comment 6

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

the spec isn't too clear, but as far as I know, "start" is the first event.

Comment 7

[Guilherme Gonçalves [:ggp]]

Attachment: Part 1 - Log event and state names rather than codes

Updating the patch with the changes smaug asked for, and keeping his r+.

Comment 8

[Guilherme Gonçalves [:ggp]]

Attachment: Part 2 - Handle reentrance in the speech recognition state machine

Removed macros. The order of events has been kept as it is, as the spec doesn't really impose any restriction in that area.

For future reference, smaug filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=21772 to clarify the spec.

Comment 9

[Guilherme Gonçalves [:ggp]]

Attachment: Part 2 - Handle reentrance in the speech recognition state machine

Updating patch with fixes to test_nested_eventloop.html

- Don't expect assertions on Windows
- Disable the test on android, where we get NS_ERROR_NOT_AVAILABLE
- GC more times and asynchronously to keep assertions confined to the test

Keeping r+ from smaug.

Comment 10

[John Schoenick [:johns]]

Comment on attachment 740466 [details] [diff] [review]
Part 1 - Log event and state names rather than codes

https://hg.mozilla.org/integration/mozilla-inbound/rev/36c5e72589bc

Comment 11

[John Schoenick [:johns]]

Comment on attachment 741414 [details] [diff] [review]
Part 2 - Handle reentrance in the speech recognition state machine

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36c5e72589bc
https://hg.mozilla.org/mozilla-central/rev/d73d6d6be727