Fix an Autofill crash caused by accessing the g_browser_process on the DB thread.

Fix an Autofill crash caused by accessing the g_browser_process on the DB thread.

This is meant as a minimal-impact CL, so that it should be safe to merge to M16.  The more complete fix is being tracked in bug 100845.

BUG=100745
TEST=less crashy


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106822

[dhollowa, 2011-10-20 15:34:27]

http://codereview.chromium.org/8355025/diff/1/chrome/browser/autofill/phone_n...
File chrome/browser/autofill/phone_number_i18n.cc (right):

http://codereview.chromium.org/8355025/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number_i18n.cc:235:
i18n::phonenumbers::PhoneNumberUtil::NO_PARSING_ERROR) {
nit: indent seems off here.  The 'i' should line up with the 'p'.

http://codereview.chromium.org/8355025/diff/1/chrome/browser/autofill/phone_n...
File chrome/browser/autofill/phone_number_i18n_unittest.cc (right):

http://codereview.chromium.org/8355025/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number_i18n_unittest.cc:20: PhoneNumberI18NTest()
: ui_thread_(BrowserThread::UI, &message_loop_) {}
I may be missing something here, but how does this ensure that the tests are run
on the UI thread?  Is it that instantiating |ui_thread_| sets a global somewhere
that makes the current thread look like a UI thread?

[GeorgeY, 2011-10-20 19:40:00]

LGTM after addressing indent issue pointed by David

http://codereview.chromium.org/8355025/diff/1/chrome/browser/autofill/phone_n...
File chrome/browser/autofill/phone_number_i18n_unittest.cc (right):

http://codereview.chromium.org/8355025/diff/1/chrome/browser/autofill/phone_n...
chrome/browser/autofill/phone_number_i18n_unittest.cc:20: PhoneNumberI18NTest()
: ui_thread_(BrowserThread::UI, &message_loop_) {}
On 2011/10/20 15:34:27, dhollowa wrote:
> I may be missing something here, but how does this ensure that the tests are
run
> on the UI thread?  Is it that instantiating |ui_thread_| sets a global
somewhere
> that makes the current thread look like a UI thread?
AFAIK, yes, creating BrowserThread::UI thread does not create anything, but just
makes current thread a UI thread.

[Ilya Sherman, 2011-10-21 00:25:57]

Refactored a good bit of the code to fix various unit test failures, and reduce
the complexity a bit.

To the best of my understanding, George is correct about unit tests and the UI
thread -- initializing a |BrowserThread| with type |UI| seems to just associate
a name with the main thread, rather than creating any new threads.

[Paweł Hajdan Jr., 2011-10-21 08:57:59]

Drive-by:

This could be going an extra mile, but I think an even better fix would be to
stop exposing a global variable g_browser_process, but instead make the accessor
a function, like it is in content. Then you can DCHECK that it's called on the
right thread, and fix the problem once and for all. Otherwise someone at some
point will use it again on the wrong thread.

[Ilya Sherman, 2011-10-21 21:14:31]

On 2011/10/21 08:57:59, Paweł Hajdan Jr. wrote:
> Drive-by:
> 
> This could be going an extra mile, but I think an even better fix would be to
> stop exposing a global variable g_browser_process, but instead make the
accessor
> a function, like it is in content. Then you can DCHECK that it's called on the
> right thread, and fix the problem once and for all. Otherwise someone at some
> point will use it again on the wrong thread.

Thanks, that's a good idea -- but it should be done in a separate CL.  I'd like
to merge this CL to M16, so I'm aiming to keep it minimal.

[dhollowa, 2011-10-21 22:47:39]

lgtm

[commit-bot: I haz the power, 2011-10-21 22:48:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/8355025/7001

[commit-bot: I haz the power, 2011-10-22 00:46:20]

Change committed as 106822