0
Commit Graph

643 Commits

Author SHA1 Message Date
Will Harris
711a5ec858 Reland "Add ability to open cookie database file with exclusive flag."
This is a reland of commit 4e37d8b3fb

The CL uses a NOTREACHED_NORETURN which behaves like a CHECK
and eats stream params in official builds. This means that
instead of EXPECT_DEATH_IF_SUPPORTED, the correct macro to use for
a death test for NOTREACHED_NORETURN is EXPECT_CHECK_DEATH_WITH.

Patchset 2 updates the CL to this expectation.

BUG=1429117,1430341

Original change's description:
> Add ability to open cookie database file with exclusive flag.
>
> This CL adds the ability to specify that the sqlite file containing
> cookies be opened with the `exclusive` uri parameter. On Windows,
> this instructs sqlite to open it without the FILE_SHARE_READ and
> FILE_SHARE_WRITE sharing dispositions, meaning that handles can
> no longer be obtained to the file while the database is open.
>
> The flag is wired up through the net/extras/sqlite layer and into
> the network service, where it is exposed as an optional parameter
> on network_context.
>
> In //chrome, the flag is set on Windows for profile based cookie
> stores, based on the feature LockProfileCookieDatabase, which is
> disabled by default.
>
> Tests are added at each layer to verify the behavior:
>  - sql_unittests ->
>     that the new parameter enable_exclusive_database_file_lock is
>     respected.
>  - net_unittests ->
>     that the new parameter to SQLitePersistentCookieStore is
>     respected.
>  - browser_tests ->
>     that the feature LockProfileCookieDatabase successfully
>     toggles the feature for the profile cookie store.
>
> BUG=1429117
>
> Change-Id: I2beaa0eab330535eecf733ecc8fd6a93f748227a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4378424
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Austin Sullivan <asully@chromium.org>
> Reviewed-by: Rohit Rao <rohitrao@chromium.org>
> Commit-Queue: Will Harris <wfh@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1125803}

Bug: 1429117
Change-Id: I3f8fa57527c6c15bccb5ef1b28d0f315165b883d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4396113
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1126252}
2023-04-04 21:43:37 +00:00
Rune Lillesveen
66afbf37b2 Revert "Add ability to open cookie database file with exclusive flag."
This reverts commit 4e37d8b3fb.

Reason for revert: Suspected culprit for sql_unittests failures on
ChromeOS

See e.g.:

https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/31726/overview

Original change's description:
> Add ability to open cookie database file with exclusive flag.
>
> This CL adds the ability to specify that the sqlite file containing
> cookies be opened with the `exclusive` uri parameter. On Windows,
> this instructs sqlite to open it without the FILE_SHARE_READ and
> FILE_SHARE_WRITE sharing dispositions, meaning that handles can
> no longer be obtained to the file while the database is open.
>
> The flag is wired up through the net/extras/sqlite layer and into
> the network service, where it is exposed as an optional parameter
> on network_context.
>
> In //chrome, the flag is set on Windows for profile based cookie
> stores, based on the feature LockProfileCookieDatabase, which is
> disabled by default.
>
> Tests are added at each layer to verify the behavior:
>  - sql_unittests ->
>     that the new parameter enable_exclusive_database_file_lock is
>     respected.
>  - net_unittests ->
>     that the new parameter to SQLitePersistentCookieStore is
>     respected.
>  - browser_tests ->
>     that the feature LockProfileCookieDatabase successfully
>     toggles the feature for the profile cookie store.
>
> BUG=1429117
>
> Change-Id: I2beaa0eab330535eecf733ecc8fd6a93f748227a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4378424
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Austin Sullivan <asully@chromium.org>
> Reviewed-by: Rohit Rao <rohitrao@chromium.org>
> Commit-Queue: Will Harris <wfh@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1125803}

Bug: 1429117
Change-Id: I84b563c91ddfafecc28a10cfbe16a370854cc827
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4396349
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Auto-Submit: Rune Lillesveen <futhark@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1125865}
2023-04-04 08:49:53 +00:00
Will Harris
4e37d8b3fb Add ability to open cookie database file with exclusive flag.
This CL adds the ability to specify that the sqlite file containing
cookies be opened with the `exclusive` uri parameter. On Windows,
this instructs sqlite to open it without the FILE_SHARE_READ and
FILE_SHARE_WRITE sharing dispositions, meaning that handles can
no longer be obtained to the file while the database is open.

The flag is wired up through the net/extras/sqlite layer and into
the network service, where it is exposed as an optional parameter
on network_context.

In //chrome, the flag is set on Windows for profile based cookie
stores, based on the feature LockProfileCookieDatabase, which is
disabled by default.

Tests are added at each layer to verify the behavior:
 - sql_unittests ->
    that the new parameter enable_exclusive_database_file_lock is
    respected.
 - net_unittests ->
    that the new parameter to SQLitePersistentCookieStore is
    respected.
 - browser_tests ->
    that the feature LockProfileCookieDatabase successfully
    toggles the feature for the profile cookie store.

BUG=1429117

Change-Id: I2beaa0eab330535eecf733ecc8fd6a93f748227a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4378424
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1125803}
2023-04-04 04:17:52 +00:00
Will Harris
563a50c492 sql: remove enable_foreign_keys_discouraged
enable_foreign_keys_discouraged is not used anywhere, so it can
be safely removed.

BUG=910955

Change-Id: Iabac5c33fa778e355b949aa605b0df8f942777fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4379442
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1123858}
2023-03-29 21:40:35 +00:00
Andrew Paseltiner
d309fec220 Remove deprecated sql::Database::RazeAndClose method
It has been replaced by the identically behaving albeit more precisely
named RazeAndPoison.

Bug: 1428036
Change-Id: I949b411bd4fcc6b7ae06ed3c11f6f519d7cc4175
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4377358
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1123105}
2023-03-28 16:59:55 +00:00
Andrew Paseltiner
b03bebe6f1 Deprecate sql::Database::RazeAndClose and rename to RazeAndPoison
Callers will be migrated separately.

Bug: 1428036
Change-Id: I84efe87ef159cec56d35aa7a08ce9a62b09f8736
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4370719
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Quick-Run: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122361}
2023-03-27 13:08:24 +00:00
Keishi Hattori
130ae0a599 [BRP] Add RAW_PTR_EXCLUSION for manual-field-to-ignore.txt entries
clang plugin cannot read external files when used with goma so
this change transitions manual-field-to-ignore.txt entries to
RAW_PTR_EXCLUSION macros.

This change only covers files used in the linux build.

Change-Id: Idea1c8c7ab1aa1dc88ba4031e02fc81228f9f6d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335650
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Owners-Override: Keishi Hattori <keishi@chromium.org>
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122242}
2023-03-27 03:48:47 +00:00
Keishi Hattori
de4c4aede1 Add RAW_PTR_EXCLUSION to files in ui/ etc.
Add RAW_PTR_EXCLUSION to files in ui/ etc. where the rewriter could not automatically rewrite.

Change was generated by processing the output of the rewriter.

AX-Relnotes: n/a.
Bug: 1273182
Change-Id: I5f3a9e2a59629545b915d5344d54623fcd34fe94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4168832
Owners-Override: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1121616}
2023-03-24 11:14:45 +00:00
Austin Sullivan
53e516eb9e sqlite: Compile with SQLITE_ENABLE_DBPAGE_VTAB
Compiling with this flag is required to support the new built-in
corruption recovery module. This was undocumented until recently,
but the documentation now exists here:
https://www.sqlite.org/recovery.html#source_code_files

The SQLITE_DBPAGE virtual table is a powerful feature that should
be enabled with caution. See https://www.sqlite.org/dbpage.html

Its documentation suggests that use of this table "can very easily
cause unrecoverable database corruption" and to "not allow untrusted
components to access the SQLITE_DBPAGE table."

This CL therefore disables use of the table in WebSQL and suggests in
numerous places that the table is not to be used in Chrome code.

Bug: 1385500
Change-Id: Ida69f4854dcd69449e99b2f50ca6e516a58944f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4298063
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Auto-Submit: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1113643}
2023-03-06 23:13:31 +00:00
Elly Fong-Jones
7d1394bd63 sql: relax constraints on VirtualCursor layout
VirtualCursor::FromSqliteCursor required that VirtualCursor had a
standard layout, but in fact VirtualCursor shouldn't have a standard
layout, and the fact that it does with libc++ is a deviation from the
C++ standard. This change:

1. Relaxes the requirement that VirtualCursor has a standard layout, and
2. Relaxes the requirement that the sqlite_cursor_ field has to be at
   offset 0

by use of offsetof() and pointer subtraction. This change both improves
standards compliance and makes this code build with libstdc++.

Bug: 1380656
Change-Id: I9c47abd9197b187da0360ca5619ccf7dadab4f33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4292313
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1111925}
2023-03-02 00:15:11 +00:00
Andrew Paseltiner
ee6c86046c Mark sql::MetaTable version-number setters [[nodiscard]]
To help prevent erroneous usage that could lead to partial schema
initialization and therefore subsequent crashes.

Bug: 1414092
Change-Id: I612e4dec62352aa7ad4ce1d4960944866b7c5e07
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4281858
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108901}
2023-02-23 13:45:45 +00:00
Joe Mason
a0ba58b051 Remove browser_watcher (3/3): Remove activity_tracker.h and all refs
Also fix IWYU errors in files that transitively included
activity_tracker.h.

This does not remove the Try() call in LockImpl::Lock() because
base/allocator/partition_allocator/spinning_mutex.h is implemented
using the try-and-then-acquire pattern so its performance is well
known. Also removes the LIKELY annotation to match the
spinning_mutex.h implementation.

Bug: 1415328
Change-Id: Ia68184c931d8b13ae3cee46048338fdfc4242e0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4273206
Reviewed-by: Will Harris <wfh@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/main@{#1108550}
2023-02-22 20:46:29 +00:00
Andrew Paseltiner
839fc23a00 Note inability to recover data from WITHOUT ROWID tables
Bug: 1418026
Change-Id: Ibb50b39be443ac396103dfc43e6baabead407ec3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4272707
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107864}
2023-02-21 19:10:40 +00:00
Austin Sullivan
142c449dcf sql: Fix divide-by-zero in PayloadReader
RecordReader always assumed PayloadReader was initialized properly so I
thought that may have been the issue, but it turns out that the
PayloadReader _was_ being initialized, but max_overflow_payload_size_
was not being set in some cases where it should be. I don't fully
understand this code or if the test case is valid, but failing the read
by returning early should at least be safe (and makes the fuzzer happy)

The extra initialization checks ended up not being needed, but it's
a nice change so I've kept it in the CL

Bug: 1417151
Change-Id: I16bbe4dafc5ddc4c688ca59922c176be36025cc0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4264791
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106988}
2023-02-17 21:13:17 +00:00
Bruce Dawson
ca1fb51824 Fixing #if commands using OS_WIN
An accidental search for OS_WIN led to me finding various obsolete uses
of OS_WIN. This upgrades them to BUILDFLAG(IS_WIN), along with upgrading
other nearby build flags.

See also crrev.com/c/4226756.

Bug: 1234043
Change-Id: I7fc581d5989eb946b0fca1df6046736f09b279d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4229854
Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Katie Dektar <katie@chromium.org>
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1104064}
2023-02-10 21:39:16 +00:00
Andrew Paseltiner
ef652c91a1 Mark sql::MetaTable::Init [[nodiscard]]
This will help prevent erroneous usage that could lead to partial
schema initialization and therefore subsequent crashes or unintended
behavior.

All callers have been updated in crbug.com/1414055.

Bug: 1414055
Change-Id: Ia25051c34c239f23d12dbd598a895d0924fee54c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4227535
Quick-Run: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1102885}
2023-02-08 21:26:32 +00:00
Andrew Paseltiner
423a131e9b Fail early in sql::MetaTable::Init if setting versions fails
An incorrect version number can cause issues with migrations, including
crashes, so Init should function atomically and only commit its
transaction if all meta table operations succeeded.

We accomplish this by returning success/failure from the two version-
setter methods, which also allows callers to return early from their
own migration code.

Change-Id: I2ed6ad38623ff46a53711e3d1041f8878a546cc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4228100
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Quick-Run: Andrew Paseltiner <apaseltiner@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1102399}
2023-02-07 21:59:05 +00:00
Will Harris
16c11cc26d Pass unsigned value into base::make_span in sql
base::span will shortly require that length is unsigned.

This CL adds a checked_cast for `result_size` as it is a
value returned from `sqlite3_column_bytes` which cannot
be guaranteed to always be unsigned.

This is part of a larger CL https://crrev.com/c/4211460.

BUG=1385166

Change-Id: Ic1fb9a3b3f2ecd4ae86b85810892bc0fdb642e77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4214451
Commit-Queue: Will Harris <wfh@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1100265}
2023-02-02 03:59:45 +00:00
Avi Drissman
2e85357a8b Update header includes for /base/functional in /s*
bind.h, callback.h, callback_forward.h, and callback_helpers.h
moved into /base/functional/. Update the include paths to
directly include them in their new location.

Bug: 1364441
Change-Id: I9576f9ae63e93053a6ce2c8cc64ebf6c85c40b1a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4159511
Auto-Submit: Avi Drissman <avi@chromium.org>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Owners-Override: Avi Drissman <avi@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1092060}
2023-01-12 21:57:32 +00:00
Kirubel Aklilu
97f422cc60 sql: Add BindTimeDelta() and ColumnTimeDelta() to sql::Statement
These helpers use the recommended approach to serialize/deserialize a
base::TimeDelta to/from int64.

These methods are added to encourage usage of built-in ways to insert
and retrieve a TimeDelta from a sql database, and discourage usage of
the `statement.BindInt64(delta.ToInternalValue())` pattern since TimeDelta::ToInternalValue is deprecated.

Bug: 1402777, 1195962

Change-Id: Ia6db6b8ff862be25d60b6ffa67f4e7e56204330d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4118621
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Kirubel Aklilu <kaklilu@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085968}
2022-12-21 17:38:56 +00:00
Jean-Philippe Gravel
01687619f3 Add missing ostream includes.
These compilation units had a transitive dependency on ostream via
base/numerics/safe_conversions.h. The ostream include in
safe_conversions.h is however unused and should be removed.

This CL was uploaded by git cl split.

R=asully@chromium.org

Bug: 1270812, 1372522
Change-Id: Id420de6fc4290a9d0bb52637d7571fb0f14b6010
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4057546
Auto-Submit: Jean-Philippe Gravel <jpgravel@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1076367}
2022-11-28 20:09:29 +00:00
Ali Hijazi
5517919606 Rewrite T& into raw_ref<T> under multiple directories [1]
The changes were generated by running
tools/clang/rewrite_raw_ref_fields/rewrite-multiple-platforms.sh with
tool-arg=--enable_raw_ref_rewrite

`raw_ref` is a smart pointer for a pointer which can not be null, and
which provides Use-after-Free protection in the same ways as raw_ptr.
This class acts like a combination of std::reference_wrapper and
raw_ptr.

See raw_ptr and //base/memory/raw_ptr.md for more details on the
Use-after-Free protection.

Bug: 1357022
Change-Id: Ibcd714bd5a8e408aa31f07a04c5fc67f7eff4e3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4001524
Owners-Override: danakj <danakj@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1069181}
2022-11-09 16:28:51 +00:00
Tommy C. Li
8ee3d2636e [sql] Consider SQLITE_ERROR a catastrophic error that razes DBs again
This CL is a partial revert of:
https://chromium-review.googlesource.com/c/chromium/src/+/3727775

Earlier this year, I changed SQLITE_ERROR to be non-catastrophic,
under the hypothesis that we had a wrong SQL statement in the code
somewhere.

We never found that wrong statement, and now I think it doesn't exist.

What we did find is:
 1. A lot of failed transaction commits or begins.
 2. A few database schemas that didn't match the reported version.

Both of those suggest genuine database corruption, so going back to
razing the database in those cases makes sense.

I DID implement a RAII singleton for the History long-running
transaction though, so we should have eliminated unintentional
transaction nesting at least.

Bug: 1321483, 1367850, 1354173
Change-Id: I7954da0add89e43644de2afc0e78d5f21314650a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4006396
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1067772}
2022-11-05 00:30:01 +00:00
Maksim Ivanov
f37a06f450 Fuzz sql::Recovery
Implement fuzzer for checking data parsing logic in
//sql/recovery.cc.

Bug: none
Change-Id: I1cd9c623fefac23a2ce69dd975a3bc952ba0ea4b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3990833
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1065734}
2022-11-01 00:16:11 +00:00
Ivan Murashov
3c7a0d50d5 sql: Add missing terminating " character in '#error' directive
In the CL https://crrev.com/c/3538180 was missed terminating " character
in the '#error' directive.
Added missing character and updated the error comment.

Bug: 1308290
Change-Id: Ie63d12588cdc16c2c4be5b142cc32d87c4a4e624
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3986423
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1064546}
2022-10-27 21:21:47 +00:00
Peter Kasting
f82bdb8866 Convert std::count*() to base:: functions: sql/
Bug: 1368812
Change-Id: I0c285862526fea0d2176faf42fc5b0f14f634aa1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3964798
Commit-Queue: Austin Sullivan <asully@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1060872}
2022-10-19 05:29:49 +00:00
Daniel Cheng
4f45ff9316 Use helper macros to define base::Features in //sql
This allows:
- features to be defined with a consistent set of qualifiers, and for
  that set of qualifiers to be updated over time as appropriate.
- better PRESUBMIT checks to ensure that base::Features are not defined
  in headers.
- simplifies things for scripts trying to extract feature definitions
  out of C++ code.

The primary CL was generated using a script that automatically rewrites
base::Feature declarations and definitions to the macro form. Changes to
any files with known incompatibilities with the macros (base::Features
without static storage duration and base::Features declared as static
class members) were then fully reverted; those changes will be manually
handled in followups.

This CL was uploaded by git cl split.

R=asully@chromium.org

Bug: 1364289
Change-Id: Ib72314440a729540f22a58022fab85cab5dc6a19
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3923023
Auto-Submit: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1052139}
2022-09-28 00:47:35 +00:00
Andrew Paseltiner
088afc4ebc Mark sql::Transaction::Begin [[nodiscard]]
Assuming that transactions are primarily used for correctness with
respect to write atomicity, callers should be expected to check the
return value of Begin and retry at a higher level if necessary, rather
than silently proceed with non-transactional writes.

Note that we make a slight behavioral change to
chrome/browser/android/explore_sites/get_catalog_task.cc to account for
this, since it has an obvious implementation.

Change-Id: Ifebe9fc56ea9a26c9bcc054cfe1240de4ff79c7e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3895361
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Quick-Run: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1048231}
2022-09-16 22:10:49 +00:00
Andrew Paseltiner
7d512ddd5b Explicitly prevent moves of immovable sql types
To make their movability clear to readers.

Change-Id: I3f43481e74468ed07642dea747141f512830c515
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3894745
Commit-Queue: Andrew Paseltiner <apaseltiner@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1047969}
2022-09-16 13:36:52 +00:00
Avi Drissman
69b874fee0 Update copyright headers in sql/, storage/
The methodology used to generate this CL is documented in
https://crbug.com/1098010#c95.

No-Try: true
Bug: 1098010
Change-Id: I68bb81a4dcae37f944f4d8cd39d82ed540364615
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3899461
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Auto-Submit: Avi Drissman <avi@chromium.org>
Owners-Override: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1047607}
2022-09-15 19:11:14 +00:00
Tommy C. Li
400c0f150b [sql] Add Error message to DatabaseDiagnostics
Adds the result of `Database::GetErrorMessage()` to the diagnostics
struct uploaded to Chrometto.

This is necessary because the existing data didn't allow us to
diagnose the error which occurs inside a transaction.

Google3 side CL that adds this field:
cl/473102826

Bug: 1321483
Change-Id: I67a2e5aa012cb95f001a50402248ab7f5f7c6936
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3885062
Reviewed-by: Stephen Nusko <nuskos@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1046610}
2022-09-13 22:39:28 +00:00
Ayu Ishii
3817c561c8 Add estade@ to sqlite OWNER
Change-Id: I11a4b46aceee24bbef2d399ca8ec63ad153d03a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3792912
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Ayu Ishii <ayui@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1029559}
2022-07-29 01:00:34 +00:00
Tommy C. Li
7803636c89 [history] Collect a trace of the diagnostics on history SQLITE_ERROR
Uses TRACE_EVENT_INSTANT1 to copy db_diagnostics_ into a Perfetto
trace once we hit an SQLITE_ERROR for History.

Follows strategy laid out here:
https://groups.google.com/a/chromium.org/g/chromium-dev/c/9zcB3WHD9R8

I intend to add an entry to the Variations config to collect the trace
of the previous 30 seconds when we log
History.DatabaseSqliteError == kError.

I'm defining a new category "history" too within tracing that I
specifically plan to capture.

Bug: 1321483
Change-Id: I01d87bba7fbd11858f76c8ab1c27e8de1fc797c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3739048
Reviewed-by: Christopher Lam <calamity@chromium.org>
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Stephen Nusko <nuskos@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1028936}
2022-07-27 21:47:22 +00:00
Tommy C. Li
596190cdf0 [sql] Don't consider SQLITE_ERROR a catastrophic error that razes the DB
Partial revert of:
https://chromium-review.googlesource.com/c/chromium/src/+/3520971/

Basically I think razing the whole DB because of a wrong SQL statement
is overkill.

Bug: 1321483
Change-Id: I7a470d34ad97fc94a7687af84f188adccf51f824
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3727775
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1019784}
2022-06-30 19:55:34 +00:00
a20.singh
b0ca112f83 sqlite: Add test case for bad db file path
This CL adds the test case for bad db file path in reference to previous
patch - https://chromium-review.googlesource.com/c/chromium/src/+/3308486

Bug: 1275513
Change-Id: I32a009e913c179d40db615877c48652c0daae522
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3655610
Reviewed-by: Chris Mumford <cmumford@google.com>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Aditi Singh <a20.singh@samsung.com>
Cr-Commit-Position: refs/heads/main@{#1007709}
2022-05-26 03:32:45 +00:00
Peter Kasting
5acb62083b Fix for C++20 support.
u8"" no longer produces const char*.  Use "" instead, since that also
accepts UTF-8 literals.

Bug: 1284275
Change-Id: Iabd04dad961ef8461d25af46286762c61a11eff1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3630693
Commit-Queue: Austin Sullivan <asully@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1000552}
2022-05-06 20:54:13 +00:00
Keishi Hattori
488b760ceb Add RAW_PTR_EXCLUSION annotations to fields
Bug: 1273182
Change-Id: I235ab43fc32e2a313197a4b535a93991581dcdfa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3607476
Owners-Override: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Bartek Nowierski <bartekn@chromium.org>
Cr-Commit-Position: refs/heads/main@{#998328}
2022-05-02 13:09:31 +00:00
ssid
583f55e474 Add readonly statement support in sql::Database
Adds support for verifying whether an SQL atatement is readonly, by
using sqlite3_stmt_readonly().

BUG=1310450

Change-Id: I0cb90157f106e199cb429960fd0a29c7ca65d8e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3597753
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Siddhartha S <ssid@chromium.org>
Cr-Commit-Position: refs/heads/main@{#994862}
2022-04-21 19:59:00 +00:00
David Sanders
f84c9f41c6 Reland "Cleanup includes of //base/check_op.h in //base"
This is a reland of commit a0719a864c

Original change's description:
> Cleanup includes of //base/check_op.h in //base
>
> Also adds some includes to other files which were
> getting them transitively, to fix the build.
>
> Bug: 242216
> Change-Id: I25e5c6012ffa3bd1bad2e21bb1622ca4d440b286
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3584279
> Commit-Queue: David Sanders <dsanders11@ucsbalum.com>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Commit-Queue: Lei Zhang <thestig@chromium.org>
> Owners-Override: Lei Zhang <thestig@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#992825}

Bug: 242216
Change-Id: Ia9fcbcfec002ed27b36324e743f7de9637d36568
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3588029
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Owners-Override: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#992915}
2022-04-15 13:42:32 +00:00
Yuki Shiino
d18d64a950 Revert "Cleanup includes of //base/check_op.h in //base"
This reverts commit a0719a864c.

Reason for revert: Suspicious about causing a tree closure.

Original change's description:
> Cleanup includes of //base/check_op.h in //base
>
> Also adds some includes to other files which were
> getting them transitively, to fix the build.
>
> Bug: 242216
> Change-Id: I25e5c6012ffa3bd1bad2e21bb1622ca4d440b286
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3584279
> Commit-Queue: David Sanders <dsanders11@ucsbalum.com>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Commit-Queue: Lei Zhang <thestig@chromium.org>
> Owners-Override: Lei Zhang <thestig@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#992825}

Bug: 242216
Change-Id: I73ebab2078c2cf515c336b37b506a3c9681ab675
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3586951
Auto-Submit: Yuki Shiino <yukishiino@chromium.org>
Owners-Override: Yuki Shiino <yukishiino@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#992839}
2022-04-15 04:37:28 +00:00
David Sanders
a0719a864c Cleanup includes of //base/check_op.h in //base
Also adds some includes to other files which were
getting them transitively, to fix the build.

Bug: 242216
Change-Id: I25e5c6012ffa3bd1bad2e21bb1622ca4d440b286
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3584279
Commit-Queue: David Sanders <dsanders11@ucsbalum.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Owners-Override: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#992825}
2022-04-15 02:50:59 +00:00
Victor Costan
62df799cca Update OWNERS around Storage.
Change-Id: Ic2fb2c500aee1f0432923ac6f5d8c46333b846a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3566213
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/main@{#988220}
2022-04-02 01:49:13 +00:00
Gabriel Charette
d87f10f467 [base] IWYU for base::Time
This CL is a no-op.

Only fixing instances where base::Time (and Ticks/Delta) is used in
a statement (i.e. not as a parameter to avoid adding includes in mere
overrides). Skipping pointer and reference qualified instances.

i.e. matches this regex:
'(\n *[^/\n][^/\n][^/\n]*base::(Time|Thread)(Ticks|Delta)?\b[^*&][^)]*;)'

and skipping files that have any existing fwd-decl for any of the
variants.

This is a prereq to remove unused base/task/post_task.h includes in
https://chromium-review.googlesource.com/c/chromium/src/+/3555247

Bug: 1026641
Change-Id: I87b43a8dc92bdceb67f4bd59b327b54813aa72a6
AX-Relnotes: n/a.
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3557354
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#987283}
2022-03-31 00:44:22 +00:00
Victor Costan
e7944b4ea6 sql: Clean up sql::Database unit tests.
This CL performs the following cleanups.

* SQLDatabaseTest.Rollback is deleted, because it's redundant with
  SQLTransactionTest.ExplicitRollback and
  SQLTransactionTest.NestedRollback.

* SQLDatabaseTest.RazeLock is broken into
  SQLDatabaseTest.Raze_OtherConnectionHas{Read,Write}lock.

* SQLDatabaseTest.RazeEmptyDB is renamed to EmptyDatabaseFile

* SQLDatabaseTest.RazeAndClose and RazeAndClose_Diagostics are broken
  into RazeAndClose, RazeAndClose_{DeletesData, IsOpen,
  Reopen_NoChanges, OpenTransaction, Preload_NoCrash, DoesTableExist,
  IsSQLValid, Execute, GetUniqueStatement, GetCachedStatement,
  TransactionBegin} and Close_IsSQLValid.

* RazeAndClose_Invalidates{Unique,Cached}Statement are added, based on
  the tests involving `valid_statement` in SQLDatabaseTest.Poison.

* SQLDatabaseTest.Poison is broken into sub-tests that parallel the
  RazeAndClose_* tests.

* SQLDatabaseTest.AttachDatabase{WithOpenTransaction} are cleaned up and
  ported away from sql::Database's direct transaction APIs.

* The modified tests have their SQL statements updated to follow the SQL
  style guide.

This CL is intended to give us better coverage around
Database::RazeAndClose() and Database::Poison(), and start migrating
away from Database::{Begin,Rollback,Commit}Transaction() APIs.

Bug: 1310577, 1311718
Change-Id: I1c970988851dd098c0deddb297242e37f4654340
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3558074
Reviewed-by: Austin Sullivan <asully@chromium.org>
Auto-Submit: Victor Costan <pwnall@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/main@{#987134}
2022-03-30 20:06:22 +00:00
Victor Costan
e49e904da7 sql: Update OWNERS.
Change-Id: I2aff17144a7f64c9ca75bdaac8a4fda1eeae9577
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3558336
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/main@{#986719}
2022-03-29 21:58:35 +00:00
Victor Costan
e2e042f4c3 sql: Clean up MetaTable.
This CL performs the following cleanups on sql::MetaTable.

* Replace all SELECT variants with PrepareGetStatement() and all INSERT
  variants with PrepareSetStatement(). Asides from reducing code
  duplication, this will reduce the number of prepared statements in the
  database's cache.

* Move Prepare{Get,Set}Statement() from private static methods to
  functions in the implementation file's anonymous namespace.

* Replace const char* arguments with base::StringPiece.

* Replace pointers with references for out arguments. This change is
  currently scoped to internal APIs, because changing external APIs
  would require updates across the whole codebase.

* Remove the SetValue() override that takes an int value. The C++
  compiler's override selection will implicitly migrate callsites to the
  SetValue() override that takes an int64_t.

* Migrate the GetValue() implementation from ColumnInt() to
  ColumnInt64().

* Tighten SQL query syntax by removing unnecessary extra spaces.

* Add DCHECKs documenting (and checking) preconditions.

* IWYU fixes in both the header and the implementation file.

This CL does not change the meta table's schema. While that is
suboptimal, fixing it will require migration code, which would be best
done in a separate CL.

Bug: 1228463, 1299863
Change-Id: I1f8bfed7aa021470d5ae47cd94bfa0acc9f11ecf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3556693
Auto-Submit: Victor Costan <pwnall@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#986697}
2022-03-29 21:11:34 +00:00
Victor Costan
cdef384420 sql: Handle sqlite3_backup_init() failures.
https://crrev.com/c/3520185 removed error checks in the code calling
sqlite3_backup_init(), based on a DCHECK that suggested the function
should not error out given our usage.

Crash data from the field proved this assumption was incorrect. This
CL adds a test demonstrating one failure case, and adds error handling
back in the code calling sqlite3_backup_init().

Bug: 1308325
Change-Id: I6c54a521faeb7812d01f7c9645646aba4ccb693f
Tested: New test hit the deleted DCHECK (before it was deleted).
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3554638
Auto-Submit: Victor Costan <pwnall@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#986071}
2022-03-28 18:51:08 +00:00
Victor Costan
eab708b057 sql: Have sql::Statement::GetSQLStatement() return std::string.
sqlite3_sql() returns a short-lived buffer. Copying the buffer contents
into a std::string immediately spares us from having to reason about
that buffer's lifetime.

Change-Id: I934d459aeef54f846804373c51c9b1ee629b8891
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3544494
Auto-Submit: Victor Costan <pwnall@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984309}
2022-03-23 14:02:53 +00:00
Victor Costan
51a84f7147 sql: Integrate SqliteResultCode into sql::Database.
This CL converts over most sql::Database methods that do not expose or
accept SQLite result codes as integers.

Bug: 1308290
Change-Id: Ibc74785ed30bd23aff5605ac9436149be99c0d79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3540769
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Auto-Submit: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/main@{#983724}
2022-03-22 08:39:40 +00:00
Victor Costan
335b822ecc sql: Check for on-disk page size in DatabaseOptions test.
This CL moves ReadPageSize() from an anonymous namespace to

to ensure that SQLite is indeed using the indicated page size.

sql: :test:ReadDatabasePageSize() and uses it in DatabaseOptions's test
Change-Id: If037ed19e96a88d7a199bb98685e7bd6b561730a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3541048
Auto-Submit: Victor Costan <pwnall@chromium.org>
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Austin Sullivan <asully@chromium.org>
Cr-Commit-Position: refs/heads/main@{#983717}
2022-03-22 08:15:11 +00:00