Fix speech crash on render view swap.

Fix speech crash on render view swap.
ChromeSRMDelegate should not be reading the WebContents* back from
  WebContentsObserver after it has explicitly set to observe nullptr
  WebContents.
This was introduced by my earlier CL: r330213, I never tested the
  feature from NTP.

BUG=489182, 489157
Test=1) From new tab page, click on the mic beside search box,
  feed it some search query, it should not crash chrome.
2) Similar to step #1, but before the query completes, close the
  tab, observe no crash.

Committed: https://crrev.com/ada65a7b1dfe82b1711ec6a6d29a5fe74c5a71a0
Cr-Commit-Position: refs/heads/master@{#330784}

[lazyboy, 2015-05-18 19:46:35]

lazyboy@chromium.org changed reviewers:
+ tommi@chromium.org

[lazyboy, 2015-05-18 19:46:36]

Hi tommi@,
Can you give me pointer to write a browser_test for this?
I'm planning to add the test on a follow up CL.

I want to test ChromeSpeechRecognitionManagerDelegate
in two scenarios:
a. open speech search mode from NTP, feed some query and see if it
navigates the tab correctly.
b. open speech search mode from NTP, close the tab while the
tab is still listening for speech input.

Will FakeSpeechRecognitionManager use
ChromeSpeechRecongitionManagerDelegate in tests properly?
Thanks.

[lazyboy, 2015-05-20 00:07:36]

lazyboy@chromium.org changed reviewers:
+ thestig@chromium.org

[lazyboy, 2015-05-20 00:07:37]

Delegating to thestig@, tommi@ seems unresponsive, would you
be able to take a look? This is crashing badly.

[Lei Zhang, 2015-05-20 00:27:01]

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:182:
DCHECK(iter != registered_web_contents_.end());
Did you blow through this DCHECK()? Insufficient test coverage?

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:229: const
content::WebContents* web_contents_;
Since this never changes, you can do "const content::WebContents* const" for
maximum const-ness.

[lazyboy, 2015-05-20 00:43:54]

Thanks for quick review, uploaded Patch Set #2.

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:182:
DCHECK(iter != registered_web_contents_.end());
On 2015/05/20 00:27:01, Lei Zhang wrote:
> Did you blow through this DCHECK()?
No, the vector has the correct WebContentsTracker entry, but getting
WebContents* out of that
entry using WebContentsObserver::web_contents() fails, because it would return
null after the
WebContentsObserver stopped tracking the web_contents. It stops tracking because
we
explicitly call WCO::Observe(nullptr);
Insufficient test coverage?
Yes, surprisingly none. I couldn't find a quick way to test this in
browsertests,
and I've mentioned that in the first comment in this CL.

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc
(right):

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:229: const
content::WebContents* web_contents_;
On 2015/05/20 00:27:01, Lei Zhang wrote:
> Since this never changes, you can do "const content::WebContents* const" for
> maximum const-ness.

Done.

[Lei Zhang, 2015-05-20 01:10:21]

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:182:
DCHECK(iter != registered_web_contents_.end());
On 2015/05/20 00:43:53, lazyboy wrote:
> On 2015/05/20 00:27:01, Lei Zhang wrote:
> > Did you blow through this DCHECK()?
> No, the vector has the correct WebContentsTracker entry, but getting
> WebContents* out of that
> entry using WebContentsObserver::web_contents() fails, because it would return
> null after the
> WebContentsObserver stopped tracking the web_contents. It stops tracking
because
> we
> explicitly call WCO::Observe(nullptr);

Hrm, then I don't understand what's causing the crash. In my head with the
existing code:

1) WebContentsObserver::web_contents() returns nullptr.
2) As a result, TabWatcher::FindWebContents(web_contents) returns
registered_web_contents_.end()

3A) Then TabWatcher::OnTabClosed() gets registered_web_contents_.end() and fails
the DCHECK. Dereferencing an invalid iterator -> crash. 

or

3B) TabWatcher::Watch() ends up doing a double registration onto
|registered_web_contents_|. Not sure how this leads to a crash?

I'm guessing you hit situation 3A, but you replied no. What's crashing then?

> Insufficient test coverage?
> Yes, surprisingly none. I couldn't find a quick way to test this in
> browsertests,
> and I've mentioned that in the first comment in this CL.

Can you promise to work on a test as your new CL?

[lazyboy, 2015-05-20 01:29:32]

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
File chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc (left):

https://chromiumcodereview.appspot.com/1134773004/diff/1/chrome/browser/speec...
chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc:182:
DCHECK(iter != registered_web_contents_.end());
On 2015/05/20 01:10:21, Lei Zhang wrote:
> On 2015/05/20 00:43:53, lazyboy wrote:
> > On 2015/05/20 00:27:01, Lei Zhang wrote:
> > > Did you blow through this DCHECK()?
> > No, the vector has the correct WebContentsTracker entry, but getting
> > WebContents* out of that
> > entry using WebContentsObserver::web_contents() fails, because it would
return
> > null after the
> > WebContentsObserver stopped tracking the web_contents. It stops tracking
> because
> > we
> > explicitly call WCO::Observe(nullptr);
> 
> Hrm, then I don't understand what's causing the crash. In my head with the
> existing code:
> 
> 1) WebContentsObserver::web_contents() returns nullptr.
> 2) As a result, TabWatcher::FindWebContents(web_contents) returns
> registered_web_contents_.end()
> 
> 3A) Then TabWatcher::OnTabClosed() gets registered_web_contents_.end() and
fails
> the DCHECK. Dereferencing an invalid iterator -> crash. 
> 
> or
> 
> 3B) TabWatcher::Watch() ends up doing a double registration onto
> |registered_web_contents_|. Not sure how this leads to a crash?
> 
> I'm guessing you hit situation 3A, but you replied no. What's crashing then?
Uggh, yes 3A is correct, my bad, I should call it a day.
For some reason, I thought you were saying |web_contents| is null,
> 
> > Insufficient test coverage?
> > Yes, surprisingly none. I couldn't find a quick way to test this in
> > browsertests,
> > and I've mentioned that in the first comment in this CL.
> 
> Can you promise to work on a test as your new CL?

http://crbug.com/489953

[Lei Zhang, 2015-05-20 02:00:37]

Ok, all makes sense, lgtm.

[lazyboy, 2015-05-20 19:39:55]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2015-05-20 19:40:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134773004/20001

[commit-bot: I haz the power, 2015-05-20 19:44:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-05-20 19:45:25]

Patchset 2 (id:??) landed as
https://crrev.com/ada65a7b1dfe82b1711ec6a6d29a5fe74c5a71a0
Cr-Commit-Position: refs/heads/master@{#330784}

[tommi (sloooow) - chröme, 2015-05-21 09:01:47]

lgtm and sorry for the delay