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

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
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.)
(Assignee)

Updated

5 years ago
Assignee: nobody → dholbert
(Assignee)

Comment 1

5 years ago
Created attachment 8371231 [details] [diff] [review]
fix
Attachment #8371231 - Flags: review?(bugs)
(Assignee)

Comment 2

5 years ago
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.)
(Assignee)

Comment 3

5 years ago
(:ggp, feel free to steal the review on this if you get to it first.)
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.
Attachment #8371231 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

5 years ago
(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. :)
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/9bb6f487c55f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Created attachment 8373425 [details] [diff] [review]
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
Attachment #8373425 - Flags: review?(bugs)

Updated

5 years ago
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.