968634 - The "!mCurrentState == STATE_IDLE" comparison in SpeechRecognition.cpp probably wants to be "!=" rather than "convert to boolean, negate, and check for equality"

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daniel Holbert [:dholbert]]

Just noticed this build warning go by, and it looks like a real issue:

{
content/media/webspeech/recognition/SpeechRecognition.cpp:699:7: note: add parentheses around left hand side expression to silence this warning
 3:22.93   if (!mCurrentState == STATE_IDLE) {
}

I'm pretty sure that wants to be "!=" comparison, rather than "compare-the-boolean-negation-of-this-value".

Some 'hg blame' research confirms my suspicion. This line was changed to its current state in bug 863852, like so, with the removal of the STATE_EQUALS macro. The change was:
>-  if (!STATE_EQUALS(STATE_IDLE)) {
>+  if (!mCurrentState == STATE_IDLE) {

In the old code, STATE_EQUALS wrapped its comparison in parenthesis, so "!" would negate the condition rather than the boolean-coercion of the first argument.

So, it looks like this has been broken since bug 863852 landed.

(For the record: I ran across this using a local build of clang, from SVN rev 185949. It reports itself as being version 3.4.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 2

[Daniel Holbert [:dholbert]]

Mochitest run, to make sure this doesn't bust any tests:
  https://tbpl.mozilla.org/?tree=Try&rev=67719d4d39e7

(Ideally it'd be nice to have a regression test for this, too, but I don't know this code well enough or have enough cycles free at the moment to take a crack at it.)

Comment 3

[Daniel Holbert [:dholbert]]

(:ggp, feel free to steal the review on this if you get to it first.)

Comment 4

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8371231 [details] [diff] [review]
fix

Review of attachment 8371231 [details] [diff] [review]:
-----------------------------------------------------------------

You're right, thanks for catching that!

For the record, there is one test that exercises that part of the code: https://mxr.mozilla.org/mozilla-central/source/content/media/webspeech/recognition/test/test_call_start_from_end_handler.html?force=1 . It tries to make sure that no InvalidStateError is raised when on STATE_IDLE, though it should
also probably check that InvalidStateError _is_ raised otherwise.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Guilherme Gonçalves [:ggp] from comment #4)
> You're right, thanks for catching that!

Thanks for the review! Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bb6f487c55f

> For the record, there is one test that exercises that part of the code:
[snip]
> it should
> also probably check that InvalidStateError _is_ raised otherwise.

Cool. I nominate you to do that, if you have time. :)

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9bb6f487c55f

Comment 7

[Guilherme Gonçalves [:ggp]]

Attachment: Test for InvalidStateError on all speech recognition handlers.

This patch updates the test to make sure we throw InvalidStateError on every
event handler while not on STATE_IDLE.

https://tbpl.mozilla.org/?tree=Try&rev=194c24bc300e

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02bb81a03398

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02bb81a03398