1088336 - SpeechGrammarList should use SpeechGrammar to store grammars

Status: RESOLVED DUPLICATE of bug 1051148

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andre Natal]

Attachment: SpeechGrammarList should use SpeechGrammar to store grammars

Comment 2

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8510679 [details] [diff] [review]
SpeechGrammarList should use SpeechGrammar to store grammars

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

Thanks! This is very much on the right track.

::: content/media/webspeech/recognition/SpeechGrammar.cpp
@@ +60,5 @@
>  
>  void
>  SpeechGrammar::SetSrc(const nsAString& aArg, ErrorResult& aRv)
>  {
> +  mGram = ToNewUTF8String(aArg);

Note that ToNewUTF8String copies aArg, and you're leaking the copy. Hence my suggestion to use one of our string classes for mGram.

::: content/media/webspeech/recognition/SpeechGrammar.h
@@ +14,5 @@
> +
> +#include "nsISpeechRecognitionService.h"
> +
> +struct JSContext;
> +

Looks like you don't need this.

@@ +54,5 @@
>    ~SpeechGrammar();
>  
>    nsCOMPtr<nsISupports> mParent;
> +
> +  char * mGram;

This should probably be one of our string classes instead of a raw pointer, most likely a nsString (some docs: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings). I'm not really sure who will own the memory associated with this string, please ask :smaug.

::: content/media/webspeech/recognition/SpeechGrammarList.cpp
@@ +105,5 @@
> +    return nullptr;
> +  }
> +  ErrorResult rv;
> +  aPresent = true;
> +  return Item(aIndex, rv);

A bit of a nit, but I think you can use nsTArray::SafeElementAt to make this a bit simpler. So you'd basically do |nsRefPtr ret = mItems.SafeElementAt(aIndex, nullptr)|, set |aPresent = (ret == nullptr)| and return |ret.forget()|.

Also I'm not really sure what we're supposed to do with aRv here -- is it supposed to be set to an error code when aPresent is false?

::: content/media/webspeech/recognition/SpeechGrammarList.h
@@ +56,5 @@
>    ~SpeechGrammarList();
>  
>    nsCOMPtr<nsISupports> mParent;
> +
> +  nsTArray<nsRefPtr<SpeechGrammar>> mItems;

Nit: mGrammars would be more descriptive.

::: content/media/webspeech/recognition/nsISpeechRecognitionService.idl
@@ +24,3 @@
>  [ptr] native SpeechGrammarPtr(mozilla::dom::SpeechGrammar);
> +[ptr] native SpeechGrammarListPtr(mozilla::dom::SpeechGrammarList);
> +

Do you really need this change?

@@ +34,4 @@
>  interface nsISpeechRecognitionService : nsISupports {
>      void initialize(in SpeechRecognitionWeakPtr aSpeechRecognition);
>      void processAudioSegment(in AudioSegmentPtr aAudioSegment, in long aSampleRate);
> +    void validateAndSetGrammarList(in SpeechGrammarPtr aSpeechGrammar, in nsISpeechGrammarCompilationCallback aCallback);

Why do you no longer want to receive a SpeechGrammarList here? A recognition service could conceivably work with a list of grammars at once, right?

If you have a good reason for this change, please also update the method name (validateAndSetGrammar), otherwise please undo it.

Comment 3

[Guilherme Gonçalves [:ggp]]

Oh, and you only really need r=smaug in the commit message, please update it :)

Comment 4

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

Comment on attachment 8510679 [details] [diff] [review]
SpeechGrammarList should use SpeechGrammar to store grammars


>   nsCOMPtr<nsISupports> mParent;
>+
>+  char * mGram;
Why not just nsString mGram;

> already_AddRefed<SpeechGrammar>
> SpeechGrammarList::Item(uint32_t aIndex, ErrorResult& aRv)
> {
>-  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
>-  return nullptr;
>+  nsRefPtr<SpeechGrammar> result = mItems.ElementAt(aIndex);
Please check that aIndex points to a valid entry. 
Otherwise you hit http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h?rev=c984a4104674&mark=916-916#910
in debug builds, and in optimized builds you just have a security bug.

> SpeechGrammarList::IndexedGetter(uint32_t aIndex, bool& aPresent,
>                                  ErrorResult& aRv)
> {
>-  aRv.Throw(NS_ERROR_NOT_IMPLEMENTED);
>-  return nullptr;
>+  if (aIndex >= Length()) {
>+    aPresent = false;
>+    return nullptr;
>+  }
>+  ErrorResult rv;
>+  aPresent = true;
>+  return Item(aIndex, rv);
... and I guess then you wouldn't need to do if (aIndex >= Length()) { check here since Item() would be safe.