sdefresne@ or sky@: Can we replace the history thread with a TaskScheduler sequence? Tasks that currently run on the history thread would run one at a time in posting order in TaskScheduler (not necessarily on a single thread).

I don't know if sqlite has anything that requires access only from a single thread. shess or brettw would likely know.

It seems like this would impact shutdown ordering, is that right? I could easily see that causing problems.

I'm pretty sure TaskScheduler provides sufficient sequencing guarantees to keep SQLite happy as we've used it.

What are good metrics to look at to know if the experiment somehow causes more corrupted databases or something?

Re. shutdown: the HistoryService can release its backend's SequencedTaskRunner and even block on the last task having been executed by the TaskScheduler if that's an issue.

I think thread-safety issues in SQLite are much more likely to show up as weird new crashes than as any sort of corruption.

https://www.sqlite.org/threadsafe.html No threading mode requires an SQLite object to always be accessed from the same thread. "Single-thread" and "Multi-thread" simply put restrictions on the number of threads from which an object can be used simultaneously.

And thus my #3.  SingleThreadTaskRunner and SequencedTaskRunner both provide the appropriate guarantees.  I'd just recommend against simultaneous access within a single connection, because none of our code works that way so you'd be on your own in terms of failure cases.

Also, our sql:: wrapper objects are not written to be thread-safe when used from multiple threads simultaneously.  Again, sequential posting is sufficient.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0cab87a48e26580170d502dc54a195cecd5cf1ca

commit 0cab87a48e26580170d502dc54a195cecd5cf1ca
Author: fdoray <fdoray@chromium.org>
Date: Tue Nov 22 14:17:58 2016

Redirect HistoryService thread to TaskScheduler via a field trial.

When the "RedirectHistoryService" variation param of the BrowserScheduler
experiment is set, HistoryService doesn't create a dedicated thread to
run HistoryBackend operations. Instead, it forwards these operations to
a TaskScheduler sequence.

BUG=661143

Review-Url: https://codereview.chromium.org/2486603003
Cr-Commit-Position: refs/heads/master@{#433855}

[modify] https://crrev.com/0cab87a48e26580170d502dc54a195cecd5cf1ca/components/history/core/browser/BUILD.gn
[add] https://crrev.com/0cab87a48e26580170d502dc54a195cecd5cf1ca/components/history/core/browser/DEPS
[modify] https://crrev.com/0cab87a48e26580170d502dc54a195cecd5cf1ca/components/history/core/browser/history_service.cc
[modify] https://crrev.com/0cab87a48e26580170d502dc54a195cecd5cf1ca/components/history/core/browser/history_service.h
[modify] https://crrev.com/0cab87a48e26580170d502dc54a195cecd5cf1ca/testing/variations/fieldtrial_testing_config.json

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4aa30591e0953da7e0c54ffdb91856e496d16b5f

commit 4aa30591e0953da7e0c54ffdb91856e496d16b5f
Author: gab <gab@chromium.org>
Date: Thu Dec 22 15:04:29 2016

Enable BrowserScheduler.RedirectHistoryService experiment on trunk.

Add .WithSyncPrimitives() trait to history service's backend
TaskRunner. Required by ChromeHistoryBackendClient::IsBookmarked().

BUG=661143
TBR=sky@chromium.org (history_service.cc tweak to experiment)

Review-Url: https://codereview.chromium.org/2592813002
Cr-Commit-Position: refs/heads/master@{#440418}

[modify] https://crrev.com/4aa30591e0953da7e0c54ffdb91856e496d16b5f/components/history/core/browser/history_service.cc
[modify] https://crrev.com/4aa30591e0953da7e0c54ffdb91856e496d16b5f/testing/variations/fieldtrial_testing_config.json

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2885403f92c9ab0a8769c2c41e859502da5a7e6f

commit 2885403f92c9ab0a8769c2c41e859502da5a7e6f
Author: gab <gab@chromium.org>
Date: Thu Dec 22 19:54:32 2016

Revert of Enable BrowserScheduler.RedirectHistoryService experiment on trunk. (patchset #6 id:120001 of https://codereview.chromium.org/2592813002/ )

Revert adapted from https://codereview.chromium.org/2597013005/ to apply on trunk.

Reason for revert:
Suspecting this CL of causing test failures:
  ProfileBrowserTest.CreateNewProfileSynchronous
  ProfileBrowserTest.OffTheRecordURLRequestContextIsolation
  ProfileBrowserTest.CreateOldProfileSynchronous
  ProfileBrowserTest.ExitType

b/c error messages mention HistoryBackend

https://uberchromegw.corp.google.com/i/chromium.memory/builders/Mac%20ASan%2064%20Tests%20%281%29/builds/25845

Original issue's description:
> Enable BrowserScheduler.RedirectHistoryService experiment on trunk.
>
> Add .WithSyncPrimitives() trait to history service's backend
> TaskRunner. Required by ChromeHistoryBackendClient::IsBookmarked().
>
> BUG=661143
> TBR=sky@chromium.org (history_service.cc tweak to experiment)
>
> Committed: https://crrev.com/4aa30591e0953da7e0c54ffdb91856e496d16b5f
> Cr-Commit-Position: refs/heads/master@{#440418}

TBR=rkaplow@chromium.org,sky@chromium.org,gab@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=661143

Review-Url: https://codereview.chromium.org/2598033003
Cr-Commit-Position: refs/heads/master@{#440479}

[modify] https://crrev.com/2885403f92c9ab0a8769c2c41e859502da5a7e6f/testing/variations/fieldtrial_testing_config.json

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/584e16efe96fd3b746987ac7afc357798f512921

commit 584e16efe96fd3b746987ac7afc357798f512921
Author: fdoray <fdoray@chromium.org>
Date: Fri Jan 06 16:53:38 2017

Flush TaskScheduler in ProfileBrowserTest.*.

When the BrowserScheduler/RedirectHistoryService experiment is disabled,
HistoryService uses a dedicated thread for backend operations. This thread
is joined when the HistoryService is deleted.

When the BrowserScheduler/RedirectHistoryService experiment is enabled,
HistoryService uses a TaskScheduler sequence for backend operations.
These tasks can run after the HistoryService has been deleted.

History backend tasks access databases in the profile directory. A crash
occurs if these accesses occur after the profile directory has been deleted.
To prevent this from happening in ProfileBrowserTest.*., this CL adds code to
flush TaskScheduler before deleting the profile directory.

BUG=661143,  676714 

Review-Url: https://codereview.chromium.org/2611053003
Cr-Commit-Position: refs/heads/master@{#441951}

[modify] https://crrev.com/584e16efe96fd3b746987ac7afc357798f512921/chrome/browser/profiles/profile_browsertest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/344517411399fc467e5a6938e4793650729b3506

commit 344517411399fc467e5a6938e4793650729b3506
Author: gab <gab@chromium.org>
Date: Fri Jan 06 18:58:25 2017

Enable BrowserScheduler.RedirectHistoryService experiment on trunk.

BUG=661143
Committed: https://crrev.com/4aa30591e0953da7e0c54ffdb91856e496d16b5f
Cr-Commit-Position: refs/heads/master@{#440418}

Reverted: https://crrev.com/2885403f92c9ab0a8769c2c41e859502da5a7e6f
Cr-Commit-Position: refs/heads/master@{#440479}
Cause of revert fixed in https://codereview.chromium.org/2611053003/.

Review-Url: https://codereview.chromium.org/2592813002
Cr-Commit-Position: refs/heads/master@{#441997}

[modify] https://crrev.com/344517411399fc467e5a6938e4793650729b3506/testing/variations/fieldtrial_testing_config.json

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f3596ad8588a28907ccfca0f9c9af984e3c5681

commit 8f3596ad8588a28907ccfca0f9c9af984e3c5681
Author: gab <gab@chromium.org>
Date: Fri Jan 20 01:56:12 2017

Revert of Enable BrowserScheduler.RedirectHistoryService experiment on trunk. (patchset #7 id:140001 of https://codereview.chromium.org/2592813002/ )

Reason for revert:
Suspected culprit  http://crbug.com/680698  and  http://crbug.com/682219  per HistoryService tasks no longer being guaranteed to be flushed in ~HistoryService -- which BookmarksModel depends on.

Original issue's description:
> Enable BrowserScheduler.RedirectHistoryService experiment on trunk.
>
> BUG=661143
> Committed: https://crrev.com/4aa30591e0953da7e0c54ffdb91856e496d16b5f
> Cr-Commit-Position: refs/heads/master@{#440418}
>
> Reverted: https://crrev.com/2885403f92c9ab0a8769c2c41e859502da5a7e6f
> Cr-Commit-Position: refs/heads/master@{#440479}
> Cause of revert fixed in https://codereview.chromium.org/2611053003/.
>
> Review-Url: https://codereview.chromium.org/2592813002
> Cr-Commit-Position: refs/heads/master@{#441997}
> Committed: https://chromium.googlesource.com/chromium/src/+/344517411399fc467e5a6938e4793650729b3506

TBR=rkaplow@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=661143, 680698 , 682219 

Review-Url: https://codereview.chromium.org/2642253002
Cr-Commit-Position: refs/heads/master@{#444942}

[modify] https://crrev.com/8f3596ad8588a28907ccfca0f9c9af984e3c5681/testing/variations/fieldtrial_testing_config.json

Looks like gab@ already completed this, with https://chromium-review.googlesource.com/c/chromium/src/+/533920 ?

No, HistoryService still uses its own thread... Sadly.