Closed Bug 968634 Opened 10 years ago Closed 10 years ago

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


(Core :: General, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)


(Blocks 1 open bug)



(2 files)

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 (!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.)
Assignee: nobody → dholbert
Attached patch fixSplinter Review
Attachment #8371231 - Flags: review?(bugs)
Mochitest run, to make sure this doesn't bust any tests:

(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.)
(:ggp, feel free to steal the review on this if you get to it first.)
Comment on attachment 8371231 [details] [diff] [review]

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: . 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.
Attachment #8371231 - Flags: review?(bugs) → review+
(In reply to Guilherme Gonçalves [:ggp] from comment #4)
> You're right, thanks for catching that!

Thanks for the review! Landed:

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

Cool. I nominate you to do that, if you have time. :)
Flags: in-testsuite?
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
This patch updates the test to make sure we throw InvalidStateError on every
event handler while not on STATE_IDLE.
Attachment #8373425 - Flags: review?(bugs)
Attachment #8373425 - Flags: review?(bugs) → review+
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.