~Browser: Don't create TabRestoreService on exit.

~Browser: Don't create TabRestoreService on exit.

Interaction between http://codereview.chromium.org/11377 and
http://codereview.chromium.org/6901031 caused TabRestoreService getting created on exit.

It was easy to break TabRestoreService, e.g., like this:
http://codereview.chromium.org/8769013 (Adding a call to SessionBackend::Init()
to BaseSessionService ctor.) This broke the "Recently closed" menu: crbug.com/110785.

After this CL, TabRestoreService is no longer created at exit and it is
hopefully more difficult to break. This also removes the workaround added by
https://codereview.chromium.org/9361056.

BUG=NONE
TEST=crbug.com/110785 no longer reproduces even if the fix is removed (see steps in the bug).


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

[marja, 2012-03-20 18:33:10]

sky: Please review.

joi: FYI, This is the TabRestoreService weirdness you bumped into.

torne: This fixes your ProfileKeyedService changes (
http://codereview.chromium.org/6901031 ) Previously,
Profile::GetTabRestoreService wouldn't recreate it if
Profile::ShutdownSessionService had been called.
It would be great to prevent this kind of breakage in the ProfileKeyedService.
Idk how to do that though; ProfileKeyedService::ProfileShutdown() could set a
flag that prevents creating the service again, but we cannot force subclasses to
call the base class impl of ProfileShutdown.

[sky, 2012-03-20 19:44:51]

LGTM

[commit-bot: I haz the power, 2012-03-20 20:57:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/9751015/1

[commit-bot: I haz the power, 2012-03-20 22:06:16]

Change committed as 127807

[Jói, 2012-03-21 10:47:42]

LGTM

Awesome, thanks for CCing me Marja, great that you found and fixed the
root cause :)

Cheers,
Jói


On Tue, Mar 20, 2012 at 10:06 PM,  <commit-bot@chromium.org> wrote:
> Change committed as 127807
>
> http://codereview.chromium.org/9751015/

[sky, 2012-03-21 22:21:30]

I thought of a couple of scenarios that might be problematic. Chromeos allows
the browser list to go to zero, and then create a new browser. I believe
background tasks can trigger this too, and I suspect multiple profile code could
too. That means we'll trigger asking for the TabRestoreService after it's
shutdown. Is that going to be problematic?

[marja, 2012-03-26 07:36:28]

On 2012/03/21 22:21:30, sky wrote:
> I thought of a couple of scenarios that might be problematic. Chromeos allows
> the browser list to go to zero, and then create a new browser. I believe
> background tasks can trigger this too, and I suspect multiple profile code
could
> too. That means we'll trigger asking for the TabRestoreService after it's
> shutdown. Is that going to be problematic?

The "last tabs" bug appeared in the case that there it was possible to
initialize the SessionBackend if it's done in the ctor, but it wasn't used
before shutdown (so, it wouldn't get initialized unless the task was set up in
the BaseSessionService ctor). In any case, if the backend is used, it gets
initialized, too, and this CL doesn't change that.

If the browser count goes to 0 and back up again, and the user continues
browsing, the backend is deleted, recreated and initialized just as it was
before.

So, the behavior changes only in the "created but not used" case. If this
reasoning is correct, this shouldn't cause any new problems in the scenario you
mentioned... (Not sure how it's working atm, though, since the backend will be
deleted and recreated when it's used.)

[sky, 2012-03-26 15:06:43]

Glad to hear it.

Thanks,

  -Scott

On Mon, Mar 26, 2012 at 12:36 AM,  <marja@chromium.org> wrote:
> On 2012/03/21 22:21:30, sky wrote:
>>
>> I thought of a couple of scenarios that might be problematic. Chromeos
>> allows
>> the browser list to go to zero, and then create a new browser. I believe
>> background tasks can trigger this too, and I suspect multiple profile code
>
> could
>>
>> too. That means we'll trigger asking for the TabRestoreService after it's
>> shutdown. Is that going to be problematic?
>
>
> The "last tabs" bug appeared in the case that there it was possible to
> initialize the SessionBackend if it's done in the ctor, but it wasn't used
> before shutdown (so, it wouldn't get initialized unless the task was set up
> in
> the BaseSessionService ctor). In any case, if the backend is used, it gets
> initialized, too, and this CL doesn't change that.
>
> If the browser count goes to 0 and back up again, and the user continues
> browsing, the backend is deleted, recreated and initialized just as it was
> before.
>
> So, the behavior changes only in the "created but not used" case. If this
> reasoning is correct, this shouldn't cause any new problems in the scenario
> you
> mentioned... (Not sure how it's working atm, though, since the backend will
> be
> deleted and recreated when it's used.)
>
> http://codereview.chromium.org/9751015/