Fix bug with invalid chrome.sessions.onChanged listener.

Fix bug with invalid chrome.sessions.onChanged listener.

Affected windows and linux builds.
When we have call chains like:
1. Create browser, register chrome.sessions.onChanged listener.
2. Create incognito window.
3. Close previous normal browser window.
4. Create new normal browser window.
5. Close incognito window.
6. Register chrome.sessions.onChanged listener.
Before this bugfix, the 6th step doesn't work: events don't fired.

R=kalman@chromium.org
BUG=

[reat, 2016-01-27 11:16:10]

reat@yandex-team.ru changed reviewers:
+ finnur@chromium.org
- kalman@chromium.org

[Finnur, 2016-01-27 17:23:56]

Missing BUG=

I don't see anything immediately wrong with this. Adding Devlin as FYI.

https://codereview.chromium.org/1642583002/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/sessions/onchanged/onchanged.js
(right):

https://codereview.chromium.org/1642583002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/sessions/onchanged/onchanged.js:10: // 2.
Remove the initial findow.
nit: window

[Finnur, 2016-01-27 17:36:51]

Oops, meant to say: LGTM.

[Devlin, 2016-01-27 17:59:04]

rdevlin.cronin@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-01-27 17:59:04]

https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/sessions/sessions_api.cc:666: new
SessionsEventRouter(Profile::FromBrowserContext(browser_context_),
This doesn't seem like the right solution... why do we create a brand new
SessionsEventRouter when the TabRestoreService is destroyed?  It seems like
we're listening to the wrong signal there.

[reat, 2016-01-28 04:09:59]

https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/sessions/sessions_api.cc:666: new
SessionsEventRouter(Profile::FromBrowserContext(browser_context_),
We do it because TabRestoreService is depends on profile, and while we create
window - create incognito window - close the first window, TabRestoreService is
destroyed, and there is no any possibility to have events on
sessions.onChanged().
Before this fix, when we try to register new event listener for new window, it
doesn't work because here we already remove listener (and do not register it
again), and already created SessionEventRouter doesn't work too, because old
TabRestoreService is destroyed, and SessionEventRouter is not subscribed for the
new one.

[Devlin, 2016-01-28 22:48:05]

rdevlin.cronin@chromium.org changed reviewers:
+ sky@chromium.org

[Devlin, 2016-01-28 22:48:05]

https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/sessions/sessions_api.cc (right):

https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/sessions/sessions_api.cc:666: new
SessionsEventRouter(Profile::FromBrowserContext(browser_context_),
On 2016/01/28 04:09:59, reat wrote:
> We do it because TabRestoreService is depends on profile, and while we create
> window - create incognito window - close the first window, TabRestoreService
is
> destroyed, and there is no any possibility to have events on
> sessions.onChanged().
> Before this fix, when we try to register new event listener for new window, it
> doesn't work because here we already remove listener (and do not register it
> again), and already created SessionEventRouter doesn't work too, because old
> TabRestoreService is destroyed, and SessionEventRouter is not subscribed for
the
> new one.

Interesting.  This stems from TabRestoreService being a
BrowserContextKeyedService, but then modifying the lifetime to be destroyed
before the underling profile is destroyed.  sky@ happens to be an OWNER there -
sky, do you know why we prematurely destroy the TabRestoreService?  Do we need
to?

[Devlin, 2016-01-28 22:48:31]

On 2016/01/28 22:48:05, Devlin wrote:
>
https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
> File chrome/browser/extensions/api/sessions/sessions_api.cc (right):
> 
>
https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
> chrome/browser/extensions/api/sessions/sessions_api.cc:666: new
> SessionsEventRouter(Profile::FromBrowserContext(browser_context_),
> On 2016/01/28 04:09:59, reat wrote:
> > We do it because TabRestoreService is depends on profile, and while we
create
> > window - create incognito window - close the first window, TabRestoreService
> is
> > destroyed, and there is no any possibility to have events on
> > sessions.onChanged().
> > Before this fix, when we try to register new event listener for new window,
it
> > doesn't work because here we already remove listener (and do not register it
> > again), and already created SessionEventRouter doesn't work too, because old
> > TabRestoreService is destroyed, and SessionEventRouter is not subscribed for
> the
> > new one.
> 
> Interesting.  This stems from TabRestoreService being a
> BrowserContextKeyedService, but then modifying the lifetime to be destroyed
> before the underling profile is destroyed.  sky@ happens to be an OWNER there
-
> sky, do you know why we prematurely destroy the TabRestoreService?  Do we need
> to?

D'oh - hit publish and mail too soon.  Meant to add "+sky@ for the question in
sessions_api.cc".

[sky, 2016-01-29 01:06:26]

On 2016/01/28 22:48:31, Devlin wrote:
> On 2016/01/28 22:48:05, Devlin wrote:
> >
>
https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
> > File chrome/browser/extensions/api/sessions/sessions_api.cc (right):
> > 
> >
>
https://codereview.chromium.org/1642583002/diff/1/chrome/browser/extensions/a...
> > chrome/browser/extensions/api/sessions/sessions_api.cc:666: new
> > SessionsEventRouter(Profile::FromBrowserContext(browser_context_),
> > On 2016/01/28 04:09:59, reat wrote:
> > > We do it because TabRestoreService is depends on profile, and while we
> create
> > > window - create incognito window - close the first window,
TabRestoreService
> > is
> > > destroyed, and there is no any possibility to have events on
> > > sessions.onChanged().
> > > Before this fix, when we try to register new event listener for new
window,
> it
> > > doesn't work because here we already remove listener (and do not register
it
> > > again), and already created SessionEventRouter doesn't work too, because
old
> > > TabRestoreService is destroyed, and SessionEventRouter is not subscribed
for
> > the
> > > new one.
> > 
> > Interesting.  This stems from TabRestoreService being a
> > BrowserContextKeyedService, but then modifying the lifetime to be destroyed
> > before the underling profile is destroyed.  sky@ happens to be an OWNER
there
> -
> > sky, do you know why we prematurely destroy the TabRestoreService?  Do we
need
> > to?
> 
> D'oh - hit publish and mail too soon.  Meant to add "+sky@ for the question in
> sessions_api.cc".

Where does TRS do that?

[reat, 2016-01-29 05:45:53]

On 2016/01/29 01:06:26, sky wrote:
> 
> Where does TRS do that?

All begin from here:
https://chromium.googlesource.com/chromium/src.git/+/master/chrome/browser/ui...
We close the last window with profile, and TabRestoreService destroyed.

[sky, 2016-01-29 17:06:17]

Early versions of TRS took ownership of the NavigationController,
hence destruction issues. That hasn't been the case for a long time.
So, I think the code in browser can be removed, which I believe would
mean you don't need this patch.

  -Scott

On Thu, Jan 28, 2016 at 9:45 PM,  <reat@yandex-team.ru> wrote:
> On 2016/01/29 01:06:26, sky wrote:
>>
>> Where does TRS do that?
>
> All begin from here:
>
https://chromium.googlesource.com/chromium/src.git/+/master/chrome/browser/ui...
> We close the last window with profile, and TabRestoreService destroyed.
>
>
> https://codereview.chromium.org/1642583002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2016-01-29 17:41:02]

On 2016/01/29 17:06:17, sky wrote:
> Early versions of TRS took ownership of the NavigationController,
> hence destruction issues. That hasn't been the case for a long time.
> So, I think the code in browser can be removed, which I believe would
> mean you don't need this patch.
> 
>   -Scott

That sounds much better to me. :)  I think that having a manually-enforced
lifetime for a BCKS is likely to trip us up in more places.  reat@, can you try
changing that behavior and seeing if that solves the problem?

[reat, 2016-02-02 09:23:53]

I remove this code which calls
TabRestoreServiceFactory::ResetForProfile(profile_); and all works fine.
Need I to make new request with removed code?

[sky, 2016-02-02 16:04:17]

On 2016/02/02 09:23:53, reat wrote:
> I remove this code which calls
> TabRestoreServiceFactory::ResetForProfile(profile_); and all works fine.
> Need I to make new request with removed code?

You haven't updated this patch, so I'm not sure what you're asking. Either morph
this patch into the fix (updating description and all that), or create a new
patch. Either one if fine (but I would likely side with a new patch as the fix
is completely different than what you have here as is the description).

[reat, 2016-02-03 10:58:01]

I create the new one.
https://codereview.chromium.org/1667683002/