Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=606772
Committed: https://crrev.com/dbbc7ddf2e5c2f3f72718bd5e807fa3e6ccad641
Cr-Commit-Position: refs/heads/master@{#393813}

[the_jk, 2016-04-20 14:52:11]

Description was changed from

==========
Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=
==========

to

==========
Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=
==========

[the_jk, 2016-04-20 14:52:11]

the_jk@opera.com changed reviewers:
+ asanka@chromium.org, sky@chromium.org

[sky, 2016-04-20 19:29:24]

sky@chromium.org changed reviewers:
+ shess@chromium.org
- sky@chromium.org

[sky, 2016-04-20 19:29:24]

sky->shess

[Scott Hess - ex-Googler, 2016-04-21 02:32:56]

lgtm.  Suggestions are just that, it's the kind of problem where you can only
expect to tweak the complexity.

https://codereview.chromium.org/1897153005/diff/1/components/history/core/bro...
File components/history/core/browser/download_database.cc (right):

https://codereview.chromium.org/1897153005/diff/1/components/history/core/bro...
components/history/core/browser/download_database.cc:243: "substr(hex(char(
id>>28)    ),2) || substr(hex(char((id>>24)&15)),2) || "
Since SQLite integers are 64-bit, I'd throw in the &15 just for the heck of it.

https://codereview.chromium.org/1897153005/diff/1/components/history/core/bro...
components/history/core/browser/download_database.cc:251: "'-' ||
hex(randomblob(6))";
Wow.  I assume you have golden tests for this so you'd notice it breaking?

Since generally the majority of the real work is in reading and writing the file
itself, not the processing, and this is a migration anyhow, I wonder if this
wouldn't be more easily done with something like:
  sql::Statement s(GetCached("SELECT id FROM downloads"));
  sql::Statement u(GetCached("UPDATE downloads SET guid = ? WHERE id = ?"));
  while (s.Step()) {
    u.BindBlob(0, MakeGuid(s.ColumnInt(0)));
    u.BindInteger(1, s.ColumnInt(0));
  }

[the_jk, 2016-04-21 11:05:28]

On 2016/04/21 02:32:56, Scott Hess wrote:
>
https://codereview.chromium.org/1897153005/diff/1/components/history/core/bro...
> components/history/core/browser/download_database.cc:243: "substr(hex(char(
> id>>28)    ),2) || substr(hex(char((id>>24)&15)),2) || "
> Since SQLite integers are 64-bit, I'd throw in the &15 just for the heck of
it.
Agreed.

> Wow.  I assume you have golden tests for this so you'd notice it breaking?
I'm rather trusting the original tests for the migration.

[Scott Hess - ex-Googler, 2016-04-21 20:57:20]

On 2016/04/21 11:05:28, the_jk wrote:
> On 2016/04/21 02:32:56, Scott Hess wrote:
> >
>
https://codereview.chromium.org/1897153005/diff/1/components/history/core/bro...
> > components/history/core/browser/download_database.cc:243: "substr(hex(char(
> > id>>28)    ),2) || substr(hex(char((id>>24)&15)),2) || "
> > Since SQLite integers are 64-bit, I'd throw in the &15 just for the heck of
> it.
> Agreed.
> 
> > Wow.  I assume you have golden tests for this so you'd notice it breaking?
> I'm rather trusting the original tests for the migration.

git ls-files | egrep download_database
components/history/core/browser/download_database.cc
components/history/core/browser/download_database.h

That doesn't seem super promising.  Does anything break if you introduce a minor
flaw in the original code?

Looks like I can make this change to the original code:
   const char kMigrateGuidsQuery[] =
       "UPDATE downloads SET guid = printf"
       "(\"%08X-%s-4%s-%01X%s-%s\","
-      " id,"
+      " 0,"
       " hex(randomblob(2)),"
       " substr(hex(randomblob(2)),2),"
       " (8 | (random() & 3)),"
and nothing breaks for components_unittests.  Things break if I add garbage to
the format string.  So this code is being exercised, but nobody is checking that
the relevant comment is actually happening.

Mostly asking because you're changing somewhat convoluted code for
more-convoluted code.  It's certainly not something that you can trivially see
is correct.

I think the code is correct, though, so LGTM.  If you value the point about
setting time_low to the download ID, you may want to circle back and add a test
for that.

[the_jk, 2016-04-26 08:31:07]

The CQ bit was checked by the_jk@opera.com

[the_jk, 2016-04-26 08:31:07]

The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org
Link to the patchset: https://codereview.chromium.org/1897153005/#ps40001
(title: "Expand unittest")

[commit-bot: I haz the power, 2016-04-26 08:31:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1897153005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1897153005/40001

[commit-bot: I haz the power, 2016-04-26 08:38:25]

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

[commit-bot: I haz the power, 2016-04-26 08:38:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[asanka, 2016-04-26 13:54:05]

On 2016/04/21 at 20:57:20, shess wrote:
> On 2016/04/21 11:05:28, the_jk wrote:
> > On 2016/04/21 02:32:56, Scott Hess wrote:
> > >
> >
https://codereview.chromium.org/1897153005/diff/1/components/history/core/bro...
> > > components/history/core/browser/download_database.cc:243:
"substr(hex(char(
> > > id>>28)    ),2) || substr(hex(char((id>>24)&15)),2) || "
> > > Since SQLite integers are 64-bit, I'd throw in the &15 just for the heck
of
> > it.
> > Agreed.
> > 
> > > Wow.  I assume you have golden tests for this so you'd notice it breaking?
> > I'm rather trusting the original tests for the migration.
> 
> git ls-files | egrep download_database
> components/history/core/browser/download_database.cc
> components/history/core/browser/download_database.h
> 
> That doesn't seem super promising.  Does anything break if you introduce a
minor flaw in the original code?
> 
> Looks like I can make this change to the original code:
>    const char kMigrateGuidsQuery[] =
>        "UPDATE downloads SET guid = printf"
>        "(\"%08X-%s-4%s-%01X%s-%s\","
> -      " id,"
> +      " 0,"
>        " hex(randomblob(2)),"
>        " substr(hex(randomblob(2)),2),"
>        " (8 | (random() & 3)),"
> and nothing breaks for components_unittests.  Things break if I add garbage to
the format string.  So this code is being exercised, but nobody is checking that
the relevant comment is actually happening.
> 
> Mostly asking because you're changing somewhat convoluted code for
more-convoluted code.  It's certainly not something that you can trivially see
is correct.
> 
> I think the code is correct, though, so LGTM.  If you value the point about
setting time_low to the download ID, you may want to circle back and add a test
for that.

The relevant test is HistoryBackendDBTest.MigrateHashHttpMethodAndGenerateGuids
in components_unittests. The test verifies that the resulting GUIDs are
well-formed and that they have sufficient entropy (to the extent verifiable in a
unittest). The bug you introduce doesn't significantly reduce the entropy to
make a difference and the resulting GUID is still well-formed. The ID is used as
a backup in case we collide in the remaining 90 bits (perhaps too paranoid).

the_jk: Would you mind filing a bug and updating the BUG= field? That would be
useful if some other distribution runs into this.

[the_jk, 2016-04-26 14:14:03]

Description was changed from

==========
Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=
==========

to

==========
Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=606772
==========

[the_jk, 2016-04-26 14:14:50]

On 2016/04/26 13:54:05, asanka (OOO until April 25) wrote:
> the_jk: Would you mind filing a bug and updating the BUG= field? That would be
> useful if some other distribution runs into this.

Done

[the_jk, 2016-04-29 14:27:26]

On 2016/04/26 14:14:50, the_jk wrote:
> On 2016/04/26 13:54:05, asanka (OOO until April 25) wrote:
> > the_jk: Would you mind filing a bug and updating the BUG= field? That would
be
> > useful if some other distribution runs into this.
> 
> Done

Ping? I assume I'm still going to get:
Missing LGTM from an OWNER for these files:
    components/history/core/browser/download_database.cc
    components/history/core/browser/history_backend_db_unittest.cc
from chromium_presubmit

[asanka, 2016-04-29 14:59:33]

On 2016/04/29 at 14:27:26, the_jk wrote:
> On 2016/04/26 14:14:50, the_jk wrote:
> > On 2016/04/26 13:54:05, asanka (OOO until April 25) wrote:
> > > the_jk: Would you mind filing a bug and updating the BUG= field? That
would be
> > > useful if some other distribution runs into this.
> > 
> > Done
> 
> Ping? I assume I'm still going to get:
> Missing LGTM from an OWNER for these files:
>     components/history/core/browser/download_database.cc
>     components/history/core/browser/history_backend_db_unittest.cc
> from chromium_presubmit

Yup. You'll need a review/stamp from an OWNER. Looks like that'll have to be
either sky@ or sdefresne.

[the_jk, 2016-05-02 13:52:28]

the_jk@opera.com changed reviewers:
+ sdefresne@chromium.org, sky@chromium.org

[the_jk, 2016-05-12 08:07:41]

Ping

[asanka, 2016-05-13 15:20:47]

On 2016/05/12 at 08:07:41, the_jk wrote:
> Ping

sky: This CL needs your rubberstamp.

[sky, 2016-05-13 15:58:55]

rubber stamp LGTM

[the_jk, 2016-05-16 08:44:35]

The CQ bit was checked by the_jk@opera.com

[the_jk, 2016-05-16 08:44:35]

The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org
Link to the patchset: https://codereview.chromium.org/1897153005/#ps80001
(title: "char() was added by 3.7.16")

[commit-bot: I haz the power, 2016-05-16 08:44:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1897153005/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1897153005/80001

[commit-bot: I haz the power, 2016-05-16 09:43:51]

Description was changed from

==========
Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=606772
==========

to

==========
Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=606772
==========

[commit-bot: I haz the power, 2016-05-16 09:43:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-16 09:45:23]

Description was changed from

==========
Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=606772
==========

to

==========
Make SQL in DownloadDatabase SQLite pre 3.8.3 compatible

printf() was added in SQLite 3.8.3 so need to use other older
methods to generate the GUID.
third_party/sqlite/sqlite.gyp lists 3.6.1 as required version at the
time of writing

BUG=606772

Committed: https://crrev.com/dbbc7ddf2e5c2f3f72718bd5e807fa3e6ccad641
Cr-Commit-Position: refs/heads/master@{#393813}
==========

[commit-bot: I haz the power, 2016-05-16 09:45:24]

Patchset 5 (id:??) landed as
https://crrev.com/dbbc7ddf2e5c2f3f72718bd5e807fa3e6ccad641
Cr-Commit-Position: refs/heads/master@{#393813}