Use SNAPSHOT sync mode for LocalSync

Use SNAPSHOT sync mode for LocalSync

LocalFileSyncService keeps the returned snapshot around until
ApplyLocalChange finishes, so this change must be transparent
to the remote side.

This patch depends on: https://codereview.chromium.org/24106002/

BUG=175693
TEST=LocalFileSyncContextTest.PrepareSync_WriteDuringSync_*
TEST=DriveFileSyncServiceSyncTest.*
TEST=manual

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

[kinuko, 2013-09-13 14:59:09]

This looks to be working ok too (the amount of change is not that big). I'll
test this code some more, but I think this change's ready for review.

[tzik, 2013-09-17 07:48:41]

lgtm

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/syncable_file_system_unittest.cc
(right):

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/syncable_file_system_unittest.cc:44: new
LocalFileSyncContext(base::FilePath(),
maybe, this needs another ScopedTempDir.

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
(right):

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc:127: new
LocalFileSyncContext(base::FilePath(),
ditto

[nhiroki, 2013-09-17 08:18:48]

lgtm

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/local_file_sync_context_unittest.cc
(right):

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/local_file_sync_context_unittest.cc:276:
file_task_runner_.get());
indent-nit

[kinuko, 2013-09-17 11:04:42]

Thx!

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/local_file_sync_context_unittest.cc
(right):

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/local_file_sync_context_unittest.cc:276:
file_task_runner_.get());
On 2013/09/17 08:18:48, nhiroki wrote:
> indent-nit

Done.

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/syncable_file_system_unittest.cc
(right):

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/syncable_file_system_unittest.cc:44: new
LocalFileSyncContext(base::FilePath(),
On 2013/09/17 07:48:41, tzik wrote:
> maybe, this needs another ScopedTempDir.

Done.

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
(right):

https://codereview.chromium.org/23578026/diff/62001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc:127: new
LocalFileSyncContext(base::FilePath(),
On 2013/09/17 07:48:41, tzik wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-09-17 11:06:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/23578026/80001

[earthdok, 2013-09-17 12:24:57]

Please fix the memory leak as well.

[earthdok, 2013-09-17 12:39:26]

https://codereview.chromium.org/23578026/diff/80001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/local_file_sync_context_unittest.cc
(left):

https://codereview.chromium.org/23578026/diff/80001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/local_file_sync_context_unittest.cc:259:
scoped_ptr<base::Thread> file_thread_;
It looks like there's a race condition between the main message loop and one of
these real threads. Is there any reason why we can't replace the whole bunch
with content::TestBrowserThreadBundle? That would both ensure clean teardown and
get rid of a lot of boilerplate code.

[kinuko (google), 2013-09-17 13:36:09]

2013/09/17 21:39 <earthdok@chromium.org>:
>
>
>
https://codereview.chromium.org/23578026/diff/80001/chrome/browser/sync_file_...
> File
> chrome/browser/sync_file_system/local/local_file_sync_context_unittest.cc
> (left):
>
>
https://codereview.chromium.org/23578026/diff/80001/chrome/browser/sync_file_...
>
chrome/browser/sync_file_system/local/local_file_sync_context_unittest.cc:259:
> scoped_ptr<base::Thread> file_thread_;
> It looks like there's a race condition between the main message loop and
> one of these real threads. Is there any reason why we can't replace the
> whole bunch with content::TestBrowserThreadBundle? That would both
> ensure clean teardown and get rid of a lot of boilerplate code.

Because this code has been just moved from webkit/, where we didn't have
access to all those nice content:: utils.  Migrating to ThreadBundle is on
our todo list.

Reg: leak, sorry, which leak error are you refering to?  I'm working on one
leak but might not be the one you're talking about. (I'm on phone right
now, will take a look later anyway)

> https://codereview.chromium.org/23578026/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[earthdok, 2013-09-17 13:41:59]

> Reg: leak, sorry, which leak error are you refering to?  I'm working on one
> leak but might not be the one you're talking about. (I'm on phone right

See the linux_asan failure for patch set 3:
http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/470...

It's almost certainly caused by the race condition I mentioned, so using
ThreadBundle should fix it.

[kinuko, 2013-09-17 13:45:55]

On 2013/09/17 13:41:59, earthdok wrote:
> > Reg: leak, sorry, which leak error are you refering to?  I'm working on one
> > leak but might not be the one you're talking about. (I'm on phone right
> 
> See the linux_asan failure for patch set 3:
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_asan/builds/470...
> 
> It's almost certainly caused by the race condition I mentioned, so using
> ThreadBundle should fix it.

I see, thanks! Will fix it now then.

[commit-bot: I haz the power, 2013-09-18 05:54:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/23578026/80001

[commit-bot: I haz the power, 2013-09-18 05:54:47]

Failed to apply patch for
chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
  Hunk #2 FAILED at 116.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc.rej

Patch:      
chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
Index: chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
diff --git
a/chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
b/chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
index
583bfe86c46dd8d5278d714a1def3e739605ad68..82bcb7f7c7732a663c9cf980acebb2cf8c7e03f9
100644
--- a/chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
+++ b/chrome/browser/sync_file_system/syncable_file_system_util_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/sync_file_system/syncable_file_system_util.h"
 
+#include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
@@ -115,16 +116,20 @@ TEST(SyncableFileSystemUtilTest,
SerializeBeforeOpenFileSystem) {
   ScopedEnableSyncFSDirectoryOperation enable_directory_operation_;
   const std::string serialized = kSyncableFileSystemRootURI +
       CreateNormalizedFilePath(kPath).AsUTF8Unsafe();
+  base::ScopedTempDir dir;
   FileSystemURL deserialized;
   base::MessageLoop message_loop;
 
+  ASSERT_TRUE(dir.CreateUniqueTempDir());
+
   // Setting up a full syncable filesystem environment.
   CannedSyncableFileSystem file_system(GURL(kOrigin),
                                        base::MessageLoopProxy::current().get(),
                                       
base::MessageLoopProxy::current().get());
   file_system.SetUp();
   scoped_refptr<LocalFileSyncContext> sync_context =
-      new LocalFileSyncContext(base::MessageLoopProxy::current().get(),
+      new LocalFileSyncContext(dir.path(),
+                               base::MessageLoopProxy::current().get(),
                                base::MessageLoopProxy::current().get());
 
   // Before calling initialization we would not be able to get a valid

[kinuko, 2013-09-18 07:19:22]

Rebased on top of thread_bundle fix. Will see if test goes happy

[commit-bot: I haz the power, 2013-09-18 12:43:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/23578026/113001

[commit-bot: I haz the power, 2013-09-18 19:39:47]

Change committed as 223913