Make AppShimHostManager a RefCountedThreadSafe.

Make AppShimHostManager a RefCountedThreadSafe.

This prevents a crash when Chrome shuts down immediately and 
AppShimHostManager is destructed before InitOnFileThread is run.
Note this means AppShimHostManager can stay around longer than
BrowserProcessPlatformPart during initialization.

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

[tapted, 2013-07-24 07:32:26]

This looks good.. but were you able to make a repro that could reliably tickle
this? It would be good to have a test that gives coverage of the case where the
BrowserProcessImpl is destroyed while these closures are still in flight.

Also a description nit: re: "stays around" -> maybe something more specific like
"can stay around longer than BrowserProcessPlatformPart during initialization"

[jackhou1, 2013-09-03 04:27:39]

On 2013/07/24 07:32:26, tapted wrote:
> This looks good.. but were you able to make a repro that could reliably tickle
> this? It would be good to have a test that gives coverage of the case where
the
> BrowserProcessImpl is destroyed while these closures are still in flight.

I couldn't get it to crash the right way. Tried adding delays to various places,
and killing the process. It might need to be killed at the right moment. In any
case, there's not really any negative impact of this CL.

> Also a description nit: re: "stays around" -> maybe something more specific
like
> "can stay around longer than BrowserProcessPlatformPart during initialization"

Done.

[tapted, 2013-09-03 06:49:06]

On 2013/09/03 04:27:39, jackhou1 wrote:
> On 2013/07/24 07:32:26, tapted wrote:
> > This looks good.. but were you able to make a repro that could reliably
tickle
> > this? It would be good to have a test that gives coverage of the case where
> the
> > BrowserProcessImpl is destroyed while these closures are still in flight.
> 
> I couldn't get it to crash the right way. Tried adding delays to various
places,
> and killing the process. It might need to be killed at the right moment. In
any
> case, there's not really any negative impact of this CL.

Yeah I think the only way this happens is if Chrome gets a SIGTERM, like for the
repro in http://crbug.com/242941. Trying to add a regression test for that would
be doomed to flakiness. So, lgtm.

https://codereview.chromium.org/20065004/diff/12001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_mac.mm (right):

https://codereview.chromium.org/20065004/diff/12001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_mac.mm:7: #include
"apps/app_shim/app_shim_host_manager_mac.h"
nit: don't need this include

[jackhou1, 2013-09-04 01:24:11]

thakis, please review for OWNERS

https://codereview.chromium.org/20065004/diff/12001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_mac.mm (right):

https://codereview.chromium.org/20065004/diff/12001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_mac.mm:7: #include
"apps/app_shim/app_shim_host_manager_mac.h"
On 2013/09/03 06:49:06, tapted wrote:
> nit: don't need this include

Done.

[Nico, 2013-09-04 15:38:23]

lgtm

[commit-bot: I haz the power, 2013-09-05 06:20:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/20065004/19001

[commit-bot: I haz the power, 2013-09-05 06:57:53]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-06 05:26:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jackhou@chromium.org/20065004/19001

[commit-bot: I haz the power, 2013-09-06 05:28:18]

Change committed as 221580

[tapted, 2013-09-09 21:48:35]

https://chromiumcodereview.appspot.com/20065004/diff/19001/apps/app_shim/app_...
File apps/app_shim/app_shim_host_manager_mac.mm (right):

https://chromiumcodereview.appspot.com/20065004/diff/19001/apps/app_shim/app_...
apps/app_shim/app_shim_host_manager_mac.mm:32: BrowserThread::PostTask(
Ah, so since reference counts start from zero, this needs to be done after the
newly constructed object is assigned to the app_shim_host_manager_ member of
BrowserProcessPlatformPart. Some options are to make InitOnFileThread public, or
make a new Init() member function for
BrowserProcessPlatformPart::PreMainMessageLoopRun to call.

I'd probably go with essentially renaming this function to Init() and having a
constructor with an empty body. The empty constructor will still call default
initializers for data members, but I don't think we need to worry about
separating out the Init steps in ExtensionAppShimHandler() - the interesting
initialization there already waits for profile creation.