[sql] Move time-machine support from third_party/sqlite to sql/

[sql] Move time-machine support from third_party/sqlite to sql/

Chromium's SQLite was modified to propagate OSX Time-Machine exclusions
from the main database file to any associated journal files.
Re-implement this using a VFS which wraps the default VFS and makes the
check when opening journal files.

BUG=679941
Review-Url: https://codereview.chromium.org/2623083002
Cr-Commit-Position: refs/heads/master@{#445601}
Committed: https://chromium.googlesource.com/chromium/src/+/5f2c344add2637d85bcf0370fe5eb882249d77f3

[Scott Hess - ex-Googler, 2017-01-11 01:27:35]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 01:28:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 01:35:09]

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

[commit-bot: I haz the power, 2017-01-11 01:35:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Scott Hess - ex-Googler, 2017-01-11 08:48:05]

iOS and Windows fixes.

[Scott Hess - ex-Googler, 2017-01-11 08:48:19]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 08:48:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 08:55:07]

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

[commit-bot: I haz the power, 2017-01-11 08:55:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Scott Hess - ex-Googler, 2017-01-11 17:50:47]

yes OSX, no IOS?

[Scott Hess - ex-Googler, 2017-01-11 19:10:41]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 19:11:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 19:19:50]

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

[commit-bot: I haz the power, 2017-01-11 19:19:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Scott Hess - ex-Googler, 2017-01-11 20:08:06]

OMG iOS

[Scott Hess - ex-Googler, 2017-01-11 20:08:16]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 20:08:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 20:26:24]

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

[commit-bot: I haz the power, 2017-01-11 20:26:25]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Scott Hess - ex-Googler, 2017-01-11 20:54:45]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 20:55:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-01-11 20:55:19]

The CQ bit was unchecked by shess@chromium.org

[Scott Hess - ex-Googler, 2017-01-11 20:56:03]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 20:56:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 22:01:28]

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

[commit-bot: I haz the power, 2017-01-11 22:01:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Scott Hess - ex-Googler, 2017-01-17 21:20:53]

disable TimeMachine test, sigh

[Scott Hess - ex-Googler, 2017-01-17 21:21:07]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:21:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2017-01-17 21:28:20]

shess@chromium.org changed reviewers:
+ mek@chromium.org

[Scott Hess - ex-Googler, 2017-01-17 21:28:20]

Reviewer chosen via random CL listings within people under jsbell :-).

The new test is landed disabled because of my comments at
http://crbug.com/410350#c8 and onward.  I still haven't figured out the
mechanism for why the trybots don't work right with this API, but I am satisfied
that the replacement code adequately replicates the existing untested code, so I
don't want to tie this to figuring that out.

[commit-bot: I haz the power, 2017-01-17 23:46:23]

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

[commit-bot: I haz the power, 2017-01-17 23:46:23]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2017-01-20 22:39:14]

Ping?

History:

http://crbug.com/14545 , http://crbug.com/25959, http://crbug.com/74053 ,
others.  The gist of it is that OSX Time Machine really doesn't interact well
with large modify-in-place files.  So we added some code to exclude certain
databases.

This CL is concerned with code that propagated the exclusion setting from the
main database file to the journal file, because there's no point to backing up
the journal in that case.  IMHO this is kind of a hack.  Using a VFS is also a
hack, but it's a more-principled hack, and it also won't have a merge conflict
every time I upgrade SQLite.

[Marijn Kruisselbrink, 2017-01-23 19:20:22]

Sorry for the delay in reviewing this, just some minor nits.
It is rather unfortunate to land a disabled test. Is there any hope/plan that
you'll be able to enable the test at some point?

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc
File sql/vfs_wrapper.cc (right):

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:170: CFURLRef CFURLRefForPath(const char* path){
nit: wouldn't it be safer if this returns a ScopedCFTypeRef rather than a bare
ref?

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:178: int Open(sqlite3_vfs* vfs, const char* file_name,
sqlite3_file* id,
nit: why is the sqlite3_file parameter called "id"?

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:207: char* dash = rindex(file_name, '-');
nit: I think strrchr() is the more common/actually standard C version of
rindex()?
(or base::rfind, but I suppose that doesn't buy you anything since you're
dealing with c style strings anyway)

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:400: memset(wrapper_vfs.get(), '\0', sizeof(sqlite3_vfs));
nit: maybe be consistent with what zero you pass to memset? Above you pass 0,
here you pass '\0'

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:410: wrapper_vfs->pNext = NULL;
nullptr

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:432: (wrapped_vfs->xCurrentTimeInt64 ? &CurrentTimeInt64 :
NULL);
nullptr

[Scott Hess - ex-Googler, 2017-01-23 21:10:50]

review comments.

[Scott Hess - ex-Googler, 2017-01-23 21:18:52]

On 2017/01/23 19:20:22, Marijn Kruisselbrink wrote:
> Sorry for the delay in reviewing this, just some minor nits.

np.

> It is rather unfortunate to land a disabled test. Is there any hope/plan that
> you'll be able to enable the test at some point?

I've been printf-debugging things on https://codereview.chromium.org/2626823008/
using progressively more-intrusive code, and I think the problem comes down to
some issue with the bot setup.  Based on disassembling framework code, I'm about
95% sure it's an unexpected failure to connect to mds (metadata service), which
is part of Spotlight, so it feels likely that it would either end up under
ExternalDependency or something explicitly unsupported by our trybot system for
some good reason.  I can ping some OSX people about whether they think tracking
it down is a good use of time (I generally use disassembly as a trigger for
"Stop, now you're just being silly").

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc
File sql/vfs_wrapper.cc (right):

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:170: CFURLRef CFURLRefForPath(const char* path){
On 2017/01/23 19:20:21, Marijn Kruisselbrink wrote:
> nit: wouldn't it be safer if this returns a ScopedCFTypeRef rather than a bare
> ref?

Done.

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:178: int Open(sqlite3_vfs* vfs, const char* file_name,
sqlite3_file* id,
On 2017/01/23 19:20:21, Marijn Kruisselbrink wrote:
> nit: why is the sqlite3_file parameter called "id"?

Probably historical reasons related to which experimenting-with-vfs local branch
I started with, and what SQLite was using whenever I wrote that branch.  Changed
to |wrapper_file|.

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:207: char* dash = rindex(file_name, '-');
On 2017/01/23 19:20:21, Marijn Kruisselbrink wrote:
> nit: I think strrchr() is the more common/actually standard C version of
> rindex()?

Done.  You might catch me dropping bzero() here and there, too...

> (or base::rfind, but I suppose that doesn't buy you anything since you're
> dealing with c style strings anyway)

Shifted to rfind() because it felt like finding the index was a tiny bit more
the goal than finding the pointer.

Explicitly didn't go on to use a StringPiece proxy for rfind() to ref and to
replace db_name with as_string() because, honestly, it started to look horribly
verbose for what it was doing.

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:400: memset(wrapper_vfs.get(), '\0', sizeof(sqlite3_vfs));
On 2017/01/23 19:20:21, Marijn Kruisselbrink wrote:
> nit: maybe be consistent with what zero you pass to memset? Above you pass 0,
> here you pass '\0'

Done.  That's weird, I've had a decades-long habit of '\0' for these things.

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:410: wrapper_vfs->pNext = NULL;
On 2017/01/23 19:20:21, Marijn Kruisselbrink wrote:
> nullptr

Done.

https://codereview.chromium.org/2623083002/diff/80001/sql/vfs_wrapper.cc#newc...
sql/vfs_wrapper.cc:432: (wrapped_vfs->xCurrentTimeInt64 ? &CurrentTimeInt64 :
NULL);
On 2017/01/23 19:20:21, Marijn Kruisselbrink wrote:
> nullptr

Done.

[Scott Hess - ex-Googler, 2017-01-23 21:18:57]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 21:19:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 22:48:07]

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

[commit-bot: I haz the power, 2017-01-23 22:48:08]

Dry run: This issue passed the CQ dry run.

[Marijn Kruisselbrink, 2017-01-23 23:32:46]

lgtm

https://codereview.chromium.org/2623083002/diff/100001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/2623083002/diff/100001/sql/connection.cc#newc...
sql/connection.cc:1696: const char* vfs_name = (vfs ? vfs->zName : NULL);
nit: this should probably also be nullptr (sorry, missed this my first time
around)

https://codereview.chromium.org/2623083002/diff/100001/sql/vfs_wrapper.cc
File sql/vfs_wrapper.cc (right):

https://codereview.chromium.org/2623083002/diff/100001/sql/vfs_wrapper.cc#new...
sql/vfs_wrapper.cc:169: // Helper to convert a POSIX path into a CoreFoundation
path.  Returns a bare
nit: update the comment now it no longer returns a bare ref.

[Scott Hess - ex-Googler, 2017-01-23 23:45:42]

sigh, should pay more attention next time.

[Scott Hess - ex-Googler, 2017-01-23 23:47:06]

thanks, sorry for being distracted wrt obvious obviousness.

https://codereview.chromium.org/2623083002/diff/100001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/2623083002/diff/100001/sql/connection.cc#newc...
sql/connection.cc:1696: const char* vfs_name = (vfs ? vfs->zName : NULL);
On 2017/01/23 23:32:46, Marijn Kruisselbrink wrote:
> nit: this should probably also be nullptr (sorry, missed this my first time
> around)

Sorry should be at my end - when you mentioned it wrt vfs_wrapper.cc, I searched
only that file and for replacement.

https://codereview.chromium.org/2623083002/diff/100001/sql/vfs_wrapper.cc
File sql/vfs_wrapper.cc (right):

https://codereview.chromium.org/2623083002/diff/100001/sql/vfs_wrapper.cc#new...
sql/vfs_wrapper.cc:169: // Helper to convert a POSIX path into a CoreFoundation
path.  Returns a bare
On 2017/01/23 23:32:46, Marijn Kruisselbrink wrote:
> nit: update the comment now it no longer returns a bare ref.

Done.

[Scott Hess - ex-Googler, 2017-01-23 23:49:05]

maybe I should stopping working on two things at once

[Scott Hess - ex-Googler, 2017-01-23 23:57:30]

scoped-ref in test, too, iwyu pass to remove leftover-from-prototype includes.

[Scott Hess - ex-Googler, 2017-01-23 23:58:39]

The CQ bit was checked by shess@chromium.org

[Scott Hess - ex-Googler, 2017-01-23 23:58:39]

The patchset sent to the CQ was uploaded after l-g-t-m from mek@chromium.org
Link to the patchset: https://codereview.chromium.org/2623083002/#ps160001
(title: "scoped-ref in test, too, iwyu pass to remove leftover-from-prototype
includes.")

[commit-bot: I haz the power, 2017-01-23 23:59:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 02:15:18]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485215919449140,
"parent_rev": "b11b7e7c8ade6d7ac945e772dc5776b2fe663bf8", "commit_rev":
"5f2c344add2637d85bcf0370fe5eb882249d77f3"}

[commit-bot: I haz the power, 2017-01-24 02:16:09]

Description was changed from

==========
[sql] Move time-machine support from third_party/sqlite to sql/

Chromium's SQLite was modified to propagate OSX Time-Machine exclusions
from the main database file to any associated journal files.
Re-implement this using a VFS which wraps the default VFS and makes the
check when opening journal files.

BUG=679941
==========

to

==========
[sql] Move time-machine support from third_party/sqlite to sql/

Chromium's SQLite was modified to propagate OSX Time-Machine exclusions
from the main database file to any associated journal files.
Re-implement this using a VFS which wraps the default VFS and makes the
check when opening journal files.

BUG=679941

Review-Url: https://codereview.chromium.org/2623083002
Cr-Commit-Position: refs/heads/master@{#445601}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f2c344add2637d85bcf0370fe5e...
==========

[commit-bot: I haz the power, 2017-01-24 02:16:11]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/5f2c344add2637d85bcf0370fe5e...