Fix a browser crash on opening 'chrome://voicesearch' in guest mode.

Fix a browser crash on opening 'chrome://voicesearch' in guest mode.

Navigating directly to chrome://voicesearch in guest mode is causing
a crash, since the HotwordService is NULL in the guest mode. This
patch adds a NULL check on HotwordService to fix the crash.

BUG=411784
TEST=With --new-profile-management enabled, try to open chrome://voicesearch
in guest mode.

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

[babu, 2014-09-08 08:32:16]

sudarsana.nagineni@intel.com changed reviewers:
+ dbeam@chromium.org, rlp@chromium.org

[rpetterson, 2014-09-08 17:35:01]

https://codereview.chromium.org/554603002/diff/20001/chrome/browser/search/ho...
File chrome/browser/search/hotword_service_factory.cc (right):

https://codereview.chromium.org/554603002/diff/20001/chrome/browser/search/ho...
chrome/browser/search/hotword_service_factory.cc:117: return
chrome::GetBrowserContextRedirectedInIncognito(context);
With this change, this will now return the original profile when in Incognito
which will have a valid HotwordService. Even for calls such as
HotwordServiceFactory::GetForProfile. While this is fine for the
about://voicesearch page, it's not fine for the rest of hotwording which relies
on having a NULL service for incognito since hotwording should not be available
then.

I put in a fix to redirect for incognito profiles
(https://codereview.chromium.org/450763002/). Do you know what the difference
between guest mode and incognito mode is such that it doesn't work for guest
mode?

[babu, 2014-09-09 18:43:06]

On 2014/09/08 17:35:01, rpetterson wrote:
>
https://codereview.chromium.org/554603002/diff/20001/chrome/browser/search/ho...
> File chrome/browser/search/hotword_service_factory.cc (right):
> 
>
https://codereview.chromium.org/554603002/diff/20001/chrome/browser/search/ho...
> chrome/browser/search/hotword_service_factory.cc:117: return
> chrome::GetBrowserContextRedirectedInIncognito(context);
> With this change, this will now return the original profile when in Incognito
> which will have a valid HotwordService. Even for calls such as
> HotwordServiceFactory::GetForProfile. While this is fine for the
> about://voicesearch page, it's not fine for the rest of hotwording which
relies
> on having a NULL service for incognito since hotwording should not be
available
> then.
> 
> I put in a fix to redirect for incognito profiles
> (https://codereview.chromium.org/450763002/). Do you know what the difference
> between guest mode and incognito mode is such that it doesn't work for guest
> mode?

@rpetterson, thanks for review.

I guess the difference is that when navigating chrome://voicesearch from an
Incognito mode, a new non-incognito window is opened for it since you made a fix
to redirect. But in Guest mode, it always opens in a Incognito window [1], and
therefore calling GetBrowserContextToUse() returns NULL by default.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[rpetterson, 2014-09-09 19:02:43]

On 2014/09/09 18:43:06, babu wrote:
> On 2014/09/08 17:35:01, rpetterson wrote:
> >
>
https://codereview.chromium.org/554603002/diff/20001/chrome/browser/search/ho...
> > File chrome/browser/search/hotword_service_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/554603002/diff/20001/chrome/browser/search/ho...
> > chrome/browser/search/hotword_service_factory.cc:117: return
> > chrome::GetBrowserContextRedirectedInIncognito(context);
> > With this change, this will now return the original profile when in
Incognito
> > which will have a valid HotwordService. Even for calls such as
> > HotwordServiceFactory::GetForProfile. While this is fine for the
> > about://voicesearch page, it's not fine for the rest of hotwording which
> relies
> > on having a NULL service for incognito since hotwording should not be
> available
> > then.
> > 
> > I put in a fix to redirect for incognito profiles
> > (https://codereview.chromium.org/450763002/). Do you know what the
difference
> > between guest mode and incognito mode is such that it doesn't work for guest
> > mode?
> 
> @rpetterson, thanks for review.
> 
> I guess the difference is that when navigating chrome://voicesearch from an
> Incognito mode, a new non-incognito window is opened for it since you made a
fix
> to redirect. But in Guest mode, it always opens in a Incognito window [1], and
> therefore calling GetBrowserContextToUse() returns NULL by default.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

IMO this is fundamentally an issue with the way chrome:// pages are handled for
Guest mode since most services don't exist for guest mode -- so why open them
for guest mode? I imagine it's because there is not original profile to default
to.

Changing the behavior of GetBrowserContextToUse is pretty invasive here (you'd
need to change the behavior throughout the codebase that deals with
HotwordServiceFactory::GetForProfile).

In the meantime, I think your original patchset (Patch Set 1) is a better fix.
The information on this page isn't going to be very relevant since hotwording
won't be available in guest mode, but until chrome:// pages are handled better
for guest mode, it's consistent with other services.

Thank you for helping out with this!

[babu, 2014-09-09 21:04:44]

On 2014/09/09 19:02:43, rpetterson wrote:
> In the meantime, I think your original patchset (Patch Set 1) is a better fix.
> The information on this page isn't going to be very relevant since hotwording
> won't be available in guest mode, but until chrome:// pages are handled better
> for guest mode, it's consistent with other services.
> 
> Thank you for helping out with this!

Thanks again for explaining. Now the initial patch up for review.

[rpetterson, 2014-09-09 21:06:40]

LGTM from a hotword standpoint. Passing to dbeam for OWNERS approval.

[Dan Beam, 2014-09-09 21:17:37]

lgtm

[babu, 2014-09-10 06:32:56]

The CQ bit was checked by sudarsana.nagineni@intel.com

[commit-bot: I haz the power, 2014-09-10 06:35:48]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sudarsana.nagineni@intel.com/554603002...

[commit-bot: I haz the power, 2014-09-10 08:36:11]

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

[commit-bot: I haz the power, 2014-09-10 08:36:12]

Exceeded time limit waiting for builds to trigger.

[babu, 2014-09-10 08:38:16]

The CQ bit was checked by sudarsana.nagineni@intel.com

[commit-bot: I haz the power, 2014-09-10 08:38:39]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/sudarsana.nagineni@intel.com/554603002...

[commit-bot: I haz the power, 2014-09-10 09:32:35]

Committed patchset #3 (id:40001) as a58c9758d7226080de4e3a217698c91113a061c3

[commit-bot: I haz the power, 2014-09-10 10:56:16]

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