[SyncFS] Post tasks between SyncEngine and SyncWorker

Remove all method-calls between SyncEngine and SyncWorker,
and make them to post tasks instead.

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

[peria, 2014-04-16 05:56:12]

Just for notes.
This CL is not ready to be reviewed.

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:59: void
RelayToTaskRunner(base::TaskRunner* task_runner,
I have to merge some RelayToTaskRunner().

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:504:
sync_engine_->DidApplyLocalChange(syncer, status);
This method needs more than replacing with RelayToTaskRunner.

[tzik, 2014-04-16 17:21:19]

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:61: const
base::Callback<void(void)>& callback) {
s/base::Callback<void(void)>/base::Closure/

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:69: const T& p1) {
# I'll revisit later.

This probably needs base::internal::CallbackParamTraits to avoid 2^n overload.

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:257:
base::Unretained(sync_engine_.get())),
Pass WeakPtr directly instead of raw pointer, since SyncWorker is going to
outlive SyncEngine.

[tzik, 2014-04-16 17:42:20]

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:257:
base::Unretained(sync_engine_.get())),
BTW, just ui_task_runner->PostTask() seems to work for this case.

[tzik, 2014-04-16 18:26:21]

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:69: const T& p1) {
Whether we should do it or not, we can do it by:
s/const T& p1/typename base::internal::CallbackParamTraits<T>::ForwardType/
s/base::Bind(callback, p1)/base::Bind(callback,
base::internal::CallbackForward(p1))/

[peria, 2014-04-17 01:40:19]

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:61: const
base::Callback<void(void)>& callback) {
On 2014/04/16 17:21:19, tzik wrote:
> s/base::Callback<void(void)>/base::Closure/

Done.

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:69: const T& p1) {
On 2014/04/16 18:26:21, tzik wrote:
> Whether we should do it or not, we can do it by:
> s/const T& p1/typename base::internal::CallbackParamTraits<T>::ForwardType/
> s/base::Bind(callback, p1)/base::Bind(callback,
> base::internal::CallbackForward(p1))/
> 

Done.

https://codereview.chromium.org/236313009/diff/70001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:257:
base::Unretained(sync_engine_.get())),
On 2014/04/16 17:21:19, tzik wrote:
> Pass WeakPtr directly instead of raw pointer, since SyncWorker is going to
> outlive SyncEngine.

Done.

[tzik, 2014-04-17 17:48:31]

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:93:
scoped_refptr<base::SequencedTaskRunner> task_runner(
s/task_runner/file_task_runner/?

Until we complete the migration, we have to use three TaskRunners, instead of
two: UI, Worker, File.
FIle TaskRunner is currently used for blocking task such as file/db related
operations.
Worker TaskRunner points UI TaskRunner for now, and eventually points File
TaskRunner.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:132:
worker_task_runner_.get(),
s/worker/file/?

Could you swap the order of these two task_runner?
I recently added a task runner to MetadataDatabase, and it's order was worker,
file.
I think maintaining the order in ui, worker, file is consistent to
MetadataDatabase case.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:335: const
std::string& account_id =
Can we use value instead of const ref here?
Even though this lifetime expansion is legal, I feel a bit uneasy.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:377:
base::SequencedTaskRunner* worker_task_runner,
s/worker/file/?

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:148:
base::SequencedTaskRunner* task_runner,
s/task_runner/file_task_runner/?

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine_context.h
(right):

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:43:
base::SequencedTaskRunner* GetBlockingTaskRunner();
While you are here, could you rename this GetBlockingTaskRunner to
GetFileTaskRunner?

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:127: FROM_HERE,
callback, SYNC_STATUS_OK);
All existing RelayToTaskRunner usage in this file can be replaced with PostTask.

GetBlockingTaskRunner() looks not the right place to post the callback from
here.
Could you directly post it to GetUiTaskRunner()?

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:135: callback);
This callback is called on the worker thread.
It's not necessary to be done in this CL, but we need to wrap this callback by
RelayToTaskRunner.

[peria, 2014-04-18 05:19:35]

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:93:
scoped_refptr<base::SequencedTaskRunner> task_runner(
On 2014/04/17 17:48:32, tzik wrote:
> s/task_runner/file_task_runner/?
> 
> Until we complete the migration, we have to use three TaskRunners, instead of
> two: UI, Worker, File.
> FIle TaskRunner is currently used for blocking task such as file/db related
> operations.
> Worker TaskRunner points UI TaskRunner for now, and eventually points File
> TaskRunner.

Done.
file_task_runner is passed in SyncEngine::Initialization(), and it is not
directly used by SyncEngine.
worker_task_runner is passed in the constructor, because it is needed to post
tasks.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:132:
worker_task_runner_.get(),
On 2014/04/17 17:48:32, tzik wrote:
> s/worker/file/?
> 
> Could you swap the order of these two task_runner?
> I recently added a task runner to MetadataDatabase, and it's order was worker,
> file.
> I think maintaining the order in ui, worker, file is consistent to
> MetadataDatabase case.

Done.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:335: const
std::string& account_id =
On 2014/04/17 17:48:32, tzik wrote:
> Can we use value instead of const ref here?
> Even though this lifetime expansion is legal, I feel a bit uneasy.

Done.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:377:
base::SequencedTaskRunner* worker_task_runner,
On 2014/04/17 17:48:32, tzik wrote:
> s/worker/file/?
Task runner for file I/O is passed in Initialization().

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:148:
base::SequencedTaskRunner* task_runner,
On 2014/04/17 17:48:32, tzik wrote:
> s/task_runner/file_task_runner/?
rename it worker_task_runner.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine_context.h
(right):

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:43:
base::SequencedTaskRunner* GetBlockingTaskRunner();
On 2014/04/17 17:48:32, tzik wrote:
> While you are here, could you rename this GetBlockingTaskRunner to
> GetFileTaskRunner?

Done.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:127: FROM_HERE,
callback, SYNC_STATUS_OK);
On 2014/04/17 17:48:32, tzik wrote:
> All existing RelayToTaskRunner usage in this file can be replaced with
PostTask.
> 
> GetBlockingTaskRunner() looks not the right place to post the callback from
> here.
> Could you directly post it to GetUiTaskRunner()?

Done.
I may have misunderstood the usage of RelayToTaskRunner.
I remove all RelayToTaskRunner in this CL, and add it again in another CL.

https://codereview.chromium.org/236313009/diff/130001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:135: callback);
On 2014/04/17 17:48:32, tzik wrote:
> This callback is called on the worker thread.
> It's not necessary to be done in this CL, but we need to wrap this callback by
> RelayToTaskRunner.

It may be done in the next CL.
I put TODO comments instead.

[peria, 2014-04-21 02:25:52]

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine_context.cc
(right):

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.cc:23:
base::SequencedTaskRunner* worker_task_runner)
s/worker/file/

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.cc:28:
file_task_runner_(worker_task_runner) {}
ditto.

[tzik, 2014-04-21 04:09:10]

Looks good.

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:99:
base::SequencedTaskRunner*
s/base::SequencedTaskRunner/scoped_refptr<base::SingleThreadTaskRunner>/?
It's for ensuring the TaskRunner is not deleted soon after this statement ends.
Using SingleThreadTaskRunner is just for consistency to other usage.

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:139:
base::MessageLoopProxy::current(),
worker_task_runner_?

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:149:
base::SequencedTaskRunner* work_task_runner,
s/work/worker/?

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (left):

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:471: }
Doesn't this change break anything?

[peria, 2014-04-21 04:50:05]

PTAL

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:99:
base::SequencedTaskRunner*
On 2014/04/21 04:09:10, tzik wrote:
> s/base::SequencedTaskRunner/scoped_refptr<base::SingleThreadTaskRunner>/?
> It's for ensuring the TaskRunner is not deleted soon after this statement
ends.
> Using SingleThreadTaskRunner is just for consistency to other usage.

Done.

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:139:
base::MessageLoopProxy::current(),
On 2014/04/21 04:09:10, tzik wrote:
> worker_task_runner_?
UI task runner should be set here, so base::MessageLoopProxy::current() is
right. Isn't it?

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine_context.cc
(right):

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.cc:23:
base::SequencedTaskRunner* worker_task_runner)
On 2014/04/21 02:25:52, peria wrote:
> s/worker/file/

Done.

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.cc:28:
file_task_runner_(worker_task_runner) {}
On 2014/04/21 02:25:52, peria wrote:
> ditto.

Done.

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (left):

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:471: }
On 2014/04/21 04:09:10, tzik wrote:
> Doesn't this change break anything?

No.
If |status| is SYNC_STATUS_OK, |should_check_conflict_| is set true on #474 and
calls |callback|.
If |status| is SYNC_STATUS_NO_CHANGE_TO_SYNC, only |callback| is called on #476.
Otherwise, |callback| is called on #476, instead of #469.  No status is changed
between those lines.

[tzik, 2014-04-21 05:10:04]

lgtm

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/236313009/diff/150001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:139:
base::MessageLoopProxy::current(),
On 2014/04/21 04:50:06, peria wrote:
> On 2014/04/21 04:09:10, tzik wrote:
> > worker_task_runner_?
> UI task runner should be set here, so base::MessageLoopProxy::current() is
> right. Isn't it?

Ah, right I misread that.

[peria, 2014-04-21 05:10:43]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-04-21 05:10:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peria@chromium.org/236313009/170001

[commit-bot: I haz the power, 2014-04-21 06:36:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-21 06:36:03]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[peria, 2014-04-21 06:50:23]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-04-21 06:50:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peria@chromium.org/236313009/170001

[commit-bot: I haz the power, 2014-04-21 10:43:44]

Change committed as 264976

[tzik, 2014-04-21 11:33:48]

A revert of this CL has been created in
https://codereview.chromium.org/243583005/ by tzik@chromium.org.

The reason for reverting is: This causes UAF on Linux ASan LSan Tests bot.
The error log is:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....