Allow SessionBackend::Init() to occur just-in-time in production builds.

Allow SessionBackend::Init() to occur just-in-time in production builds.

This fixes a regression introduced in r113377 that caused the "old
session" to be switched at start-up before being used to populate the
recently closed menu.

This is the simplest possible fix, intended for merge to M18.  A more
robust fix that prevents future regressions is likely possible and may
be investigated as a follow-up.

BUG=110785
TEST=see bug repro steps

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

[Finnur, 2012-02-14 11:17:26]

Yup, this looks like it reverts back to the old-style functionality of not
initializing the backend when in production.

LGTM.

[Jói, 2012-02-14 13:07:45]

+marja for chrome/browser/sessions/OWNERS approval.

Cheers,
Jói

[marja, 2012-02-14 13:28:16]

chrome/browser/sessions/* LGTM, this seems to revert the change introduced in
http://codereview.chromium.org/8769013 (extra call to SessionBackend::Init()).

sky: FYI.

[marja, 2012-02-14 13:28:35]

... and now really cc:ing sky.

[sky, 2012-02-14 19:25:25]

You piqued my interest. What was breaking?

  -Scott

On Tue, Feb 14, 2012 at 5:28 AM,  <marja@chromium.org> wrote:
> ... and now really cc:ing sky.
>
> http://codereview.chromium.org/9361056/

[Jói, 2012-02-14 19:57:00]

See http://crbug.com/110785.  I was able to reproduce with the change
dbeam pointed to, and not with the previous revision of the tree.  I
tracked it down pretty easily to a mistake I had made while
refactoring, where I posted the Init task to the backend thread after
refactoring, whereas before that did not happen.

My theory on how to fix this better, and I have yet to look at this in
more detail, would be to add a method to get the "last run of Chrome
session" that would work regardless of whether Init() has been called
or not.  That would hopefully make it robust to the timing of when
Init() is called.

Cheers,
Jói



On Tue, Feb 14, 2012 at 7:25 PM, Scott Violet <sky@chromium.org> wrote:
> You piqued my interest. What was breaking?
>
>  -Scott
>
> On Tue, Feb 14, 2012 at 5:28 AM,  <marja@chromium.org> wrote:
>> ... and now really cc:ing sky.
>>
>> http://codereview.chromium.org/9361056/

[Jói, 2012-02-20 13:40:18]

Hi Scott,

I've been looking at the code to figure out what was happening before
the bugfix to http://crbug.com/110785 (change at the top of this
review thread).

From reviewing the code, the difference I can see is that now (and
before I introduced the regression),
SessionBackend::MoveCurrentSessionToLastSession would be the first
method posted to the IO thread for SessionBackend (the task is posted
from SessionService::RestoreIfNecessary), whereas while the bug was
there, SessionBackend::Init would get posted first, and later
SessionBackend::MoveCurrentSessionToLastSession.

What I don't properly understand is (a) how the change fixes the bug,
and (b) why things work at all :)
SessionBackend::MoveCurrentSessionToLastSession starts by calling
Init(), which if it hasn't been called before turns around and calls
MoveCurrentSessionToLastSession.  So I would think that in both the
case before the bugfix, and in the present case, the current session
would be "demoted" to the last session twice in a row, thus most
likely losing the last session information.  Perhaps there is
something about recursively calling MoveCurrentSessionToLastSession
within itself that somehow lets things work, but I suspect there is a
different explanation.

I'm wondering whether a fix to make this more robust might be to make
MoveCurrentSessionToLastSession private, and have it only be called by
Init().

Let me know what you think.  I think I'm probably missing something so
I won't make any change without insight from somebody more familiar
with the code.

Cheers,
Jói



2012/2/14 Jói Sigurðsson <joi@chromium.org>:
> See http://crbug.com/110785.  I was able to reproduce with the change
> dbeam pointed to, and not with the previous revision of the tree.  I
> tracked it down pretty easily to a mistake I had made while
> refactoring, where I posted the Init task to the backend thread after
> refactoring, whereas before that did not happen.
>
> My theory on how to fix this better, and I have yet to look at this in
> more detail, would be to add a method to get the "last run of Chrome
> session" that would work regardless of whether Init() has been called
> or not.  That would hopefully make it robust to the timing of when
> Init() is called.
>
> Cheers,
> Jói
>
>
>
> On Tue, Feb 14, 2012 at 7:25 PM, Scott Violet <sky@chromium.org> wrote:
>> You piqued my interest. What was breaking?
>>
>>  -Scott
>>
>> On Tue, Feb 14, 2012 at 5:28 AM,  <marja@chromium.org> wrote:
>>> ... and now really cc:ing sky.
>>>
>>> http://codereview.chromium.org/9361056/

[marja, 2012-02-20 15:49:59]

Hi Joi,

I didn't write the code, so I cannot tell why things are the way they
are, but I can experimentally try to find out how it works :)

On 20 February 2012 14:39, Jói Sigurðsson <joi@chromium.org> wrote:
> (b) why things work at all :)

When I start the browser, and no session restore is done ("reopen last
pages" not selected), the order of the events is:

- TabRestoreService::LoadTabsFromLastSession() ->
RecentlyClosedTabsHandler::HandleGetRecentlyClosedTabs() ->
BaseSessionService::ScheduleGetLastSessionCommands()
- Schedules SessionBackend::ReadLastSessionCommands() on the background thread
- SessionBackend::ReadLastSessionCommands() -> SessionBackend::Init()
-> SessionBackend::MoveCurrentSessionToLastSession() ->
SessionBackend::Init() (already inited, no-op)
- SessionBackend::ReadLastSessionCommands() proceeds reading from
GetLastSessionPath().

So, the first backend operation that is done is
SessionBackend::ReadLastSessionCommand() and not
SessionBackend::MoveCurrentSessionToLastSession(). Copying from the
current session to the last session is done only once.

(Q: Is the Init() that MoveCurrentSessionToLastSession() calls always
a no-op? Will something go wrong if it is not?)

(a) how the change fixes the bug

This is a bit trickier! First, an observation: If I copy my "known to
be good" "Current Tabs" and "Last Tabs" to the profile data dir, both
the buggy version and the trunk version work (i.e., they display the
"Recently closed" menu on the new tab page after the first launch).

So, this is not a startup-time bug but an exit-time bug.

At exit, a BaseSessionService is created, with this BT:
	BaseSessionService::BaseSessionService() [0xee5694]
	TabRestoreService::TabRestoreService() [0xf0705b]
	TabRestoreServiceFactory::BuildServiceInstanceFor() [0x7d134b]
	ProfileKeyedBaseFactory::GetBaseForProfile() [0x78925d]
	ProfileKeyedServiceFactory::GetServiceForProfile() [0x78ae28]
	TabRestoreServiceFactory::GetForProfile() [0x7d1171]
	Browser::~Browser() [0x878ab8]
	scoped_ptr<>::~scoped_ptr() [0x8b8207]
	BrowserWindowGtk::~BrowserWindowGtk() [0x8af59a]

And now, if BaseSessionService schedules SessionBackend::Init(), it is
still ran... and it moves the Current Tabs over the Last Tabs. (In the
non-buggy version, this doesn't happen during the exit.) This is why
the bug happens.

Seems a bit broken that TabRestoreService and BaseSessionService are
created at exit time, just for calling
TabRestoreService::BrowserClosed()... :)

Cheers,
Marja

[Jói, 2012-02-20 16:09:35]

Hi Marja,

Thanks for the analysis!  That is subtle indeed.

I'm not sure what the best fix is to make this code robust to a
similar regression as the one I introduced.  Possibly since the
BaseSessionService gets instantiated more than once there should be a
static to keep track of having already moved the current session to
the old session (one for each type of session I guess).  I'm unsure
whether this is appropriate though, i.e. whether it is expected that
we may want to move the current session to the old session multiple
times during the lifetime of the browser process.

My question on whether MoveCurrentSessionToLastSession should be
called only by SessionBackend::Init() and made private still stands,
although apparently it has nothing to do with the bug :)

Cheers,
Jói



2012/2/20 Marja Hölttä <marja@chromium.org>:
> Hi Joi,
>
> I didn't write the code, so I cannot tell why things are the way they
> are, but I can experimentally try to find out how it works :)
>
> On 20 February 2012 14:39, Jói Sigurðsson <joi@chromium.org> wrote:
>> (b) why things work at all :)
>
> When I start the browser, and no session restore is done ("reopen last
> pages" not selected), the order of the events is:
>
> - TabRestoreService::LoadTabsFromLastSession() ->
> RecentlyClosedTabsHandler::HandleGetRecentlyClosedTabs() ->
> BaseSessionService::ScheduleGetLastSessionCommands()
> - Schedules SessionBackend::ReadLastSessionCommands() on the background thread
> - SessionBackend::ReadLastSessionCommands() -> SessionBackend::Init()
> -> SessionBackend::MoveCurrentSessionToLastSession() ->
> SessionBackend::Init() (already inited, no-op)
> - SessionBackend::ReadLastSessionCommands() proceeds reading from
> GetLastSessionPath().
>
> So, the first backend operation that is done is
> SessionBackend::ReadLastSessionCommand() and not
> SessionBackend::MoveCurrentSessionToLastSession(). Copying from the
> current session to the last session is done only once.
>
> (Q: Is the Init() that MoveCurrentSessionToLastSession() calls always
> a no-op? Will something go wrong if it is not?)
>
> (a) how the change fixes the bug
>
> This is a bit trickier! First, an observation: If I copy my "known to
> be good" "Current Tabs" and "Last Tabs" to the profile data dir, both
> the buggy version and the trunk version work (i.e., they display the
> "Recently closed" menu on the new tab page after the first launch).
>
> So, this is not a startup-time bug but an exit-time bug.
>
> At exit, a BaseSessionService is created, with this BT:
>        BaseSessionService::BaseSessionService() [0xee5694]
>        TabRestoreService::TabRestoreService() [0xf0705b]
>        TabRestoreServiceFactory::BuildServiceInstanceFor() [0x7d134b]
>        ProfileKeyedBaseFactory::GetBaseForProfile() [0x78925d]
>        ProfileKeyedServiceFactory::GetServiceForProfile() [0x78ae28]
>        TabRestoreServiceFactory::GetForProfile() [0x7d1171]
>        Browser::~Browser() [0x878ab8]
>        scoped_ptr<>::~scoped_ptr() [0x8b8207]
>        BrowserWindowGtk::~BrowserWindowGtk() [0x8af59a]
>
> And now, if BaseSessionService schedules SessionBackend::Init(), it is
> still ran... and it moves the Current Tabs over the Last Tabs. (In the
> non-buggy version, this doesn't happen during the exit.) This is why
> the bug happens.
>
> Seems a bit broken that TabRestoreService and BaseSessionService are
> created at exit time, just for calling
> TabRestoreService::BrowserClosed()... :)
>
> Cheers,
> Marja

[sky, 2012-02-22 16:01:58]

Good analysis Marja.

2012/2/20 Jói Sigurðsson <joi@chromium.org>:
> Hi Marja,
>
> Thanks for the analysis!  That is subtle indeed.
>
> I'm not sure what the best fix is to make this code robust to a
> similar regression as the one I introduced.  Possibly since the
> BaseSessionService gets instantiated more than once there should be a
> static to keep track of having already moved the current session to
> the old session (one for each type of session I guess).  I'm unsure
> whether this is appropriate though, i.e. whether it is expected that
> we may want to move the current session to the old session multiple
> times during the lifetime of the browser process.
>
> My question on whether MoveCurrentSessionToLastSession should be
> called only by SessionBackend::Init() and made private still stands,
> although apparently it has nothing to do with the bug :)

MoveCurrentSessionToLastSession can be invoked more than once. From
SessionService:

// SessionService supports restoring from the last session. The last session
// typically corresponds to the last run of the browser, but not always. For
// example, if the user has a tabbed browser and app window running, closes the
// tabbed browser, then creates a new tabbed browser the current session is made
// the last session and the current session reset. This is done to provide the
// illusion that app windows run in separate processes. Similar behavior occurs
// with incognito windows.

If we're to protect against something, it should be that we don't
create the services more than once for a profile. Seems like a bug in
SessionServiceFactory::ShutdownForProfile code.

  -Scott

>
> Cheers,
> Jói
>
>
>
> 2012/2/20 Marja Hölttä <marja@chromium.org>:
>> Hi Joi,
>>
>> I didn't write the code, so I cannot tell why things are the way they
>> are, but I can experimentally try to find out how it works :)
>>
>> On 20 February 2012 14:39, Jói Sigurðsson <joi@chromium.org> wrote:
>>> (b) why things work at all :)
>>
>> When I start the browser, and no session restore is done ("reopen last
>> pages" not selected), the order of the events is:
>>
>> - TabRestoreService::LoadTabsFromLastSession() ->
>> RecentlyClosedTabsHandler::HandleGetRecentlyClosedTabs() ->
>> BaseSessionService::ScheduleGetLastSessionCommands()
>> - Schedules SessionBackend::ReadLastSessionCommands() on the background
thread
>> - SessionBackend::ReadLastSessionCommands() -> SessionBackend::Init()
>> -> SessionBackend::MoveCurrentSessionToLastSession() ->
>> SessionBackend::Init() (already inited, no-op)
>> - SessionBackend::ReadLastSessionCommands() proceeds reading from
>> GetLastSessionPath().
>>
>> So, the first backend operation that is done is
>> SessionBackend::ReadLastSessionCommand() and not
>> SessionBackend::MoveCurrentSessionToLastSession(). Copying from the
>> current session to the last session is done only once.
>>
>> (Q: Is the Init() that MoveCurrentSessionToLastSession() calls always
>> a no-op? Will something go wrong if it is not?)
>>
>> (a) how the change fixes the bug
>>
>> This is a bit trickier! First, an observation: If I copy my "known to
>> be good" "Current Tabs" and "Last Tabs" to the profile data dir, both
>> the buggy version and the trunk version work (i.e., they display the
>> "Recently closed" menu on the new tab page after the first launch).
>>
>> So, this is not a startup-time bug but an exit-time bug.
>>
>> At exit, a BaseSessionService is created, with this BT:
>>        BaseSessionService::BaseSessionService() [0xee5694]
>>        TabRestoreService::TabRestoreService() [0xf0705b]
>>        TabRestoreServiceFactory::BuildServiceInstanceFor() [0x7d134b]
>>        ProfileKeyedBaseFactory::GetBaseForProfile() [0x78925d]
>>        ProfileKeyedServiceFactory::GetServiceForProfile() [0x78ae28]
>>        TabRestoreServiceFactory::GetForProfile() [0x7d1171]
>>        Browser::~Browser() [0x878ab8]
>>        scoped_ptr<>::~scoped_ptr() [0x8b8207]
>>        BrowserWindowGtk::~BrowserWindowGtk() [0x8af59a]
>>
>> And now, if BaseSessionService schedules SessionBackend::Init(), it is
>> still ran... and it moves the Current Tabs over the Last Tabs. (In the
>> non-buggy version, this doesn't happen during the exit.) This is why
>> the bug happens.
>>
>> Seems a bit broken that TabRestoreService and BaseSessionService are
>> created at exit time, just for calling
>> TabRestoreService::BrowserClosed()... :)
>>
>> Cheers,
>> Marja