Switch BaseSessionService to use SequencedWorkerPool instead of FILE thread

Switch BaseSessionService to use SequencedWorkerPool instead of FILE thread 

Removed RunningInProduction and changed SessionServiceTestHelper to flush IO tasks when service is deleted.

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

[marja, 2013-08-08 20:16:43]

Some questions (also, I'm on utc+1, I might not have a chance to have another
look today, sorry for the delay):

https://codereview.chromium.org/22363005/diff/8001/chrome/browser/sessions/ba...
File chrome/browser/sessions/base_session_service.cc (right):

https://codereview.chromium.org/22363005/diff/8001/chrome/browser/sessions/ba...
chrome/browser/sessions/base_session_service.cc:321: return profile_ &&
!pool->IsShutdownInProgress();
Why checking IsShutdownInProgress here? Will that somehow be true in tests (like
the function name would hint), or.. is the function name not reflecting what it
does? Will there be "pool" in tests? Do we need the separation between tests and
production code any more?

[oshima, 2013-08-08 20:36:53]

https://codereview.chromium.org/22363005/diff/8001/chrome/browser/sessions/ba...
File chrome/browser/sessions/base_session_service.cc (right):

https://codereview.chromium.org/22363005/diff/8001/chrome/browser/sessions/ba...
chrome/browser/sessions/base_session_service.cc:321: return profile_ &&
!pool->IsShutdownInProgress();
On 2013/08/08 20:16:43, marja wrote:
> Why checking IsShutdownInProgress here? Will that somehow be true in tests
(like
> the function name would hint), or.. is the function name not reflecting what
it
> does? Will there be "pool" in tests? Do we need the separation between tests
and
> production code any more?

This is just replacement of
BrowserThread::IsMessageLoopValid(BrowserThread::FILE) in original code.
This is to run tasks in UI thread during shutdown, otherwise tasks may not
be processed. I believe test case is handled by | profile_ |


I do agree that method name is a bit misleading though.
I'm happy to rename it to something like TaskShouldRunOnMainThread/UIThread or
something you like. Please let me know.

[oshima, 2013-08-08 20:44:29]

On 2013/08/08 20:16:43, marja wrote:
> Some questions (also, I'm on utc+1, I might not have a chance to have another
> look today, sorry for the delay):
> 
>
https://codereview.chromium.org/22363005/diff/8001/chrome/browser/sessions/ba...
> File chrome/browser/sessions/base_session_service.cc (right):
> 
>
https://codereview.chromium.org/22363005/diff/8001/chrome/browser/sessions/ba...
> chrome/browser/sessions/base_session_service.cc:321: return profile_ &&
> !pool->IsShutdownInProgress();
> Why checking IsShutdownInProgress here? Will that somehow be true in tests
(like
> the function name would hint), or.. is the function name not reflecting what
it
> does? Will there be "pool" in tests?

yes

> Do we need the separation between tests and
> production code any more?

 I believe you added original checks in  http://codereview.chromium.org/9751015
and https://chromiumcodereview.appspot.com/10443056, so I assume you
know better than I do? I'm happy to try if I should remove it. Let me know.

[marja, 2013-08-09 07:57:31]

Thanks for your answers and for working on this!

My understanding is that:
- normally we want to delay the initing (see comment here:
https://chromiumcodereview.appspot.com/10443056/diff/1/chrome/browser/session...
)
- but as there was no FILE thread in unit tests, we needed to detect that
situation and run the task immediately instead

However, as there is a worker pool in the unit test, it looks like we could just
always schedule it to run on the pool (maybe some tests then need to wait until
it's really run? but at least the production code would be simpler).

(Plus maybe another case for the situation that the worker pool has already
exited.)

Hmm, this is actually the code you have :) I think it's ok to just check the "is
the pool shut down" in RunTaskOnBackendThread.

Could you also have a look if we can remove the

profile_ &&

from that if clause without tests breaking? Iirc profile_ can only be NULL in
tests, but are those tests still able to run the stuff in the worker pool
successfully?

Conclusion: LGTM with these actions:
- Please remove RunningInProduction, just check "is the pool shut down" in
RunTaskOnBackendThread (i.e., move the if clause there)
- Please investigate if we can remove the "profile_ && from the if clause; if
all tests run, it can be removed.

(I'll be OOO on Monday, if you need quicker feedback, pls try sky@.)

[oshima, 2013-08-09 11:01:06]

On 2013/08/09 07:57:31, marja wrote:
> Thanks for your answers and for working on this!
> 
> My understanding is that:
> - normally we want to delay the initing (see comment here:
>
https://chromiumcodereview.appspot.com/10443056/diff/1/chrome/browser/session...
> )
> - but as there was no FILE thread in unit tests, we needed to detect that
> situation and run the task immediately instead
> 
> However, as there is a worker pool in the unit test, it looks like we could
just
> always schedule it to run on the pool (maybe some tests then need to wait
until
> it's really run? but at least the production code would be simpler).
> 
> (Plus maybe another case for the situation that the worker pool has already
> exited.)
> 
> Hmm, this is actually the code you have :) I think it's ok to just check the
"is
> the pool shut down" in RunTaskOnBackendThread.
> 
> Could you also have a look if we can remove the
> 
> profile_ &&
> 
> from that if clause without tests breaking? Iirc profile_ can only be NULL in
> tests, but are those tests still able to run the stuff in the worker pool
> successfully?
> 
> Conclusion: LGTM with these actions:
> - Please remove RunningInProduction, just check "is the pool shut down" in
> RunTaskOnBackendThread (i.e., move the if clause there)
> - Please investigate if we can remove the "profile_ && from the if clause; if
> all tests run, it can be removed.

Done and uploaded new patch. PTAL.
 
> (I'll be OOO on Monday, if you need quicker feedback, pls try sky@.)

[oshima, 2013-08-09 11:01:53]

On 2013/08/09 11:01:06, oshima wrote:
> On 2013/08/09 07:57:31, marja wrote:
> > Thanks for your answers and for working on this!
> > 
> > My understanding is that:
> > - normally we want to delay the initing (see comment here:
> >
>
https://chromiumcodereview.appspot.com/10443056/diff/1/chrome/browser/session...
> > )
> > - but as there was no FILE thread in unit tests, we needed to detect that
> > situation and run the task immediately instead
> > 
> > However, as there is a worker pool in the unit test, it looks like we could
> just
> > always schedule it to run on the pool (maybe some tests then need to wait
> until
> > it's really run? but at least the production code would be simpler).
> > 
> > (Plus maybe another case for the situation that the worker pool has already
> > exited.)
> > 
> > Hmm, this is actually the code you have :) I think it's ok to just check the
> "is
> > the pool shut down" in RunTaskOnBackendThread.
> > 
> > Could you also have a look if we can remove the
> > 
> > profile_ &&
> > 
> > from that if clause without tests breaking? Iirc profile_ can only be NULL
in
> > tests, but are those tests still able to run the stuff in the worker pool
> > successfully?
> > 
> > Conclusion: LGTM with these actions:
> > - Please remove RunningInProduction, just check "is the pool shut down" in
> > RunTaskOnBackendThread (i.e., move the if clause there)
> > - Please investigate if we can remove the "profile_ && from the if clause;
if
> > all tests run, it can be removed.
> 
> Done and uploaded new patch. PTAL.
>  
> > (I'll be OOO on Monday, if you need quicker feedback, pls try sky@.)

sky is OOO next week, so I'd appreciate if you can review today.

[marja, 2013-08-09 11:38:12]

Still lgtm.

Thanks for doing this; I'm glad that we have one less "if running a test" block
in the production code!

[oshima, 2013-08-09 11:40:05]

jochen -> content/browser/web_contents

[jochen (gone - plz use gerrit), 2013-08-09 11:49:56]

https://chromiumcodereview.appspot.com/22363005/diff/26001/chrome/browser/ses...
File chrome/browser/sessions/base_session_service.cc (right):

https://chromiumcodereview.appspot.com/22363005/diff/26001/chrome/browser/ses...
chrome/browser/sessions/base_session_service.cc:303: task.Run();
that means that the tasks will ran in an random order at this point.

isShutdownInProgress() might return true even if there are still running tasks.

Is that intended?

[oshima, 2013-08-09 15:34:54]

On 2013/08/09 11:49:56, jochen wrote:
>
https://chromiumcodereview.appspot.com/22363005/diff/26001/chrome/browser/ses...
> File chrome/browser/sessions/base_session_service.cc (right):
> 
>
https://chromiumcodereview.appspot.com/22363005/diff/26001/chrome/browser/ses...
> chrome/browser/sessions/base_session_service.cc:303: task.Run();
> that means that the tasks will ran in an random order at this point.
> 
> isShutdownInProgress() might return true even if there are still running
tasks.
> 
> Is that intended?

RunTaskOnBackendThread is called on UI thread, and this SequncedWorkerPool is
shutdown by UI thread
(see ShutdownThreadsAndCleanUp() in browser_main_loop.cc), so it's guaranteed
that there is
no BLOCK_SHUTDOWN task running when IsShutdownInProgress is true.

I added DCHECK just in case.

[jochen (gone - plz use gerrit), 2013-08-09 16:33:16]

lgtm

[commit-bot: I haz the power, 2013-08-09 16:41:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/22363005/35001

[commit-bot: I haz the power, 2013-08-12 05:18:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/22363005/56001

[commit-bot: I haz the power, 2013-08-12 07:33:41]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-12 14:27:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/22363005/72001

[commit-bot: I haz the power, 2013-08-12 15:49:35]

Change committed as 217003