Closed Bug 863852 Opened 7 years ago Closed 7 years ago

Speech Recognition is not prepared to handle reentrance

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: ggp, Assigned: ggp)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
A logging patch that makes it a little easier to debug problems like this in the future.
Attachment #739775 - Flags: review?(bugs)
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.
Attachment #739783 - Flags: review?(bugs)
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
Attachment #739775 - Flags: review?(bugs) → review+
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"
Attachment #739783 - Flags: review?(bugs) → review-
(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?
the spec isn't too clear, but as far as I know, "start" is the first event.
Updating the patch with the changes smaug asked for, and keeping his r+.
Attachment #739775 - Attachment is obsolete: true
Attachment #740466 - Flags: review+
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.
Attachment #739783 - Attachment is obsolete: true
Attachment #740470 - Flags: review?(bugs)
Attachment #740470 - Flags: review?(bugs) → review+
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.
Attachment #740470 - Attachment is obsolete: true
Attachment #741414 - Flags: review+
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
Attachment #741414 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/36c5e72589bc
https://hg.mozilla.org/mozilla-central/rev/d73d6d6be727
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 968634
You need to log in before you can comment on or make changes to this bug.