Making sure that SpeechRecognitionDispatcherHost is not used after free

Making sure that SpeechRecognitionDispatcherHost is not used after free
A test have been discovered that under certain conditions a use after
free can occur.

BUG=339667
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252233

[tommi (sloooow) - chröme, 2014-02-18 12:34:18]

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
content/browser/speech/input_tag_speech_dispatcher_host.cc:41:
weak_factory_.InvalidateWeakPtrs();
This should always be invalid when we get to the destructor, right?
I.e. OnChannelClosing should have been called first.

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
content/browser/speech/input_tag_speech_dispatcher_host.cc:46: return
weak_factory_.GetWeakPtr();
could add a dcheck for the IO thread here.

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
content/browser/speech/input_tag_speech_dispatcher_host.cc:139:
config.event_listener = this->AsWeakPtr();
remove this->

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
File content/browser/speech/input_tag_speech_dispatcher_host.h (right):

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
content/browser/speech/input_tag_speech_dispatcher_host.h:29: public
SpeechRecognitionEventListener {
I was hoping that you could inherit from
SupportsWeakPtr<SpeecRecognitionEventListener> here instead but I see now why
that's not possible (since SupportsWeakPtr doesn't support explicit
invalidation).

I would still like it if InputTagSpeechDispatcherHost cannot be cast directly
over to SpeecRecognitionEventListener, so what do you think about adding an
internal class that inherits from SupportsWeakPtr<SpeecRecognitionEventListener>
that is created on the IO thread and deleted inside OnChannelClosing()?
The AsWeakPtr() method could then be renamed to e.g. GetEventListener() and
return a WeakPtr<SpeecRecognitionEventListener> (return listener_.AsWeakPtr())
instead of the full InputTagSpeechDispatcherHost type.
The listener would need some way to invoke methods on the
InputTagSpeechDispatcherHost when events occur and one way to do that without
defining extra interfaces or adding public methods, is by using closures.

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/speec...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/speec...
content/browser/speech/speech_recognition_dispatcher_host.h:28: public
SpeechRecognitionEventListener {
same thing for SpeechRecognitionEventListener here.  Maybe a common
SpeechRecognitionEventListener[Impl] class can be written that supports WeakPtr
and is single threaded (can DCHECK that all methods including ctor and dtor
occur on the IO thread)?

[Tommy Widenflycht, 2014-02-18 14:23:29]

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
File content/browser/speech/input_tag_speech_dispatcher_host.cc (right):

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
content/browser/speech/input_tag_speech_dispatcher_host.cc:41:
weak_factory_.InvalidateWeakPtrs();
On 2014/02/18 12:34:18, tommi wrote:
> This should always be invalid when we get to the destructor, right?
> I.e. OnChannelClosing should have been called first.

Done.

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
content/browser/speech/input_tag_speech_dispatcher_host.cc:46: return
weak_factory_.GetWeakPtr();
On 2014/02/18 12:34:18, tommi wrote:
> could add a dcheck for the IO thread here.

Done.

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
content/browser/speech/input_tag_speech_dispatcher_host.cc:139:
config.event_listener = this->AsWeakPtr();
On 2014/02/18 12:34:18, tommi wrote:
> remove this->

Done.

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
File content/browser/speech/input_tag_speech_dispatcher_host.h (right):

https://codereview.chromium.org/170243005/diff/1/content/browser/speech/input...
content/browser/speech/input_tag_speech_dispatcher_host.h:29: public
SpeechRecognitionEventListener {
As discussed irl the SpeecRecognitionEventListener is an interface implemented
by several classes so I prefer to keep the structure as it is. I do realize that
this is not an ideal design though.

On 2014/02/18 12:34:18, tommi wrote:
> I was hoping that you could inherit from
> SupportsWeakPtr<SpeecRecognitionEventListener> here instead but I see now why
> that's not possible (since SupportsWeakPtr doesn't support explicit
> invalidation).
> 
> I would still like it if InputTagSpeechDispatcherHost cannot be cast directly
> over to SpeecRecognitionEventListener, so what do you think about adding an
> internal class that inherits from
SupportsWeakPtr<SpeecRecognitionEventListener>
> that is created on the IO thread and deleted inside OnChannelClosing()?
> The AsWeakPtr() method could then be renamed to e.g. GetEventListener() and
> return a WeakPtr<SpeecRecognitionEventListener> (return listener_.AsWeakPtr())
> instead of the full InputTagSpeechDispatcherHost type.
> The listener would need some way to invoke methods on the
> InputTagSpeechDispatcherHost when events occur and one way to do that without
> defining extra interfaces or adding public methods, is by using closures.

[tommi (sloooow) - chröme, 2014-02-18 14:31:50]

lgtm

[Tommy Widenflycht, 2014-02-18 15:02:41]

The CQ bit was checked by tommyw@chromium.org

[Tommy Widenflycht, 2014-02-18 15:02:44]

The CQ bit was unchecked by tommyw@chromium.org

[commit-bot: I haz the power, 2014-02-18 15:02:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommyw@chromium.org/170243005/150001

[Tommy Widenflycht, 2014-02-19 09:12:37]

The CQ bit was checked by tommyw@chromium.org

[commit-bot: I haz the power, 2014-02-19 09:12:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommyw@chromium.org/170243005/150001

[commit-bot: I haz the power, 2014-02-19 09:12:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 09:12:57]

Try jobs failed on following builders: linux_chromium_rel

[Tommy Widenflycht, 2014-02-19 11:28:47]

Joi,
Could you take a look as well? I need your OWNERS approval.

[Jói, 2014-02-19 13:23:27]

//content/public LGTM

[Tommy Widenflycht, 2014-02-19 13:57:01]

The CQ bit was checked by tommyw@chromium.org

[commit-bot: I haz the power, 2014-02-19 13:57:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommyw@chromium.org/170243005/150001

[commit-bot: I haz the power, 2014-02-19 13:57:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 13:57:18]

Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel

[Tommy Widenflycht, 2014-02-20 09:18:07]

The CQ bit was checked by tommyw@chromium.org

[Tommy Widenflycht, 2014-02-20 11:52:12]

The CQ bit was unchecked by tommyw@chromium.org

[Tommy Widenflycht, 2014-02-20 11:52:13]

The CQ bit was checked by tommyw@chromium.org

[commit-bot: I haz the power, 2014-02-20 12:17:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommyw@chromium.org/170243005/150001

[commit-bot: I haz the power, 2014-02-20 14:36:04]

Change committed as 252233