Let IndexedDBContextImpl create its own task runner

Let IndexedDBContextImpl create its own task runner

Have the IndexedDBContextImpl create its own sequenced task runner
(via the task scheduler), rather than having it injected on creation.

Unit tests needed updating to ensure objects (factory, backing store,
transactions, etc) are created on the appropriate sequences and that
operations (e.g. blob writes) do correct thread hopping. This
makes the tests more correct, but also more complicated.

BUG=552552
Review-Url: https://codereview.chromium.org/2930183002
Cr-Commit-Position: refs/heads/master@{#486588}
Committed: https://chromium.googlesource.com/chromium/src/+/bd2caa0d663427a19b674a8f88ad36cadcb5275e

[jsbell, 2017-06-12 17:51:12]

jsbell@chromium.org changed reviewers:
- dmurph@chromium.org, jam@chromium.org

[jsbell, 2017-06-12 17:51:13]

gab@ - can you take an initial look at this?

[jsbell, 2017-06-12 17:51:13]

jsbell@chromium.org changed reviewers:
- dmurph@chromium.org, jam@chromium.org

[jsbell, 2017-06-12 17:51:14]

gab@ - can you take an initial look at this?

[jsbell, 2017-06-12 17:56:43]

On 2017/06/12 17:51:14, jsbell wrote:
> gab@ - can you take an initial look at this?

Ugh, sorry for the double mail and also I wanted to provide more context...

* The use of ScopedTaskEnvironment and TestBrowserThreadBundle together is
pretty cargo-cult. Advice welcome (desired?)

* There are still too many SetTaskRunnerForTesting calls; advice welcome on
quick fixes w/o reworking the tests.

The uses in indexed_db_backing_store_unittest.cc,
indexed_db_factory_unittest.cc, and indexed_db_unittest.cc are annotated - the
test bodies call directly into methods that check sequence affinity. It feels
like I'm doing something wrong here.

The threading contortions in indexed_db_dispatcher_host_unittest.cc are just
more complicated than I wanted to wrap my head around at the moment. Some looks
extraneous, some might be for mojo. I'll consult with dmurph and do a follow-up.

[gab, 2017-06-13 15:31:23]

inline comments on your main questions

https://codereview.chromium.org/2930183002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/2930183002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_context_impl.h:80: const
scoped_refptr<base::SequencedTaskRunner>& task_runner) override;
ditto (prefer by value)

https://codereview.chromium.org/2930183002/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_unittest.cc (right):

https://codereview.chromium.org/2930183002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_unittest.cc:60: TestBrowserThreadBundle
thread_bundle_;
TestBrowserThreadBundle includes a base::test::ScopedTaskEnvironment, you don't
need to mix both.

(and TestBrowserThreadBundle provides a main thread of type UI by default as
well)

TestBrowserThreadBundle/ScopedTaskEnvironment typically need to be the first
member though so that might be why you were hitting issues before, just move it
to 55 instead of ScopedTaskEnvironment and it should work.

Regarding running-until-idle. 

ScopedTaskEnvironment -> ScopedTaskEnvironment::RunUntilIdle()
TestBrowserThreadBundle -> RunAllBlockingPoolTasksUntilIdle()

you shouldn't need to mix both within a given file, they're equivalent (and we
should eventually just have TestBrowserThreadBundle::RunUntilIdle() to coalesce
the APIs but RunAllBlockingPoolTasksUntilIdle() is old stuff so need to live
with it for now...)

https://codereview.chromium.org/2930183002/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_unittest.cc:178:
base::SequencedTaskRunnerHandle::Get());
A cleaner alternative to doing this would be to post your entire test body which
needs to run on |task_runner| as a lambda and then run until idle.

That way you properly create a multi-threaded test where things run as expected
(and highlight races for TSAN if there are any in the product).

i.e.:

instead of 

TEST(...) {
  idb_context->Foo();
  idb_context->Bar();
}

you could:

TEST(...) {
  idb_context->TaskRunner()->PostTask(
      base::Bind([](IndexedDBContextImpl* idb_context) {
          idb_context->Foo();
          idb_context->Bar();
      }, base::Unretained(idb_context)));

  run until idle...
}

https://codereview.chromium.org/2930183002/diff/20001/content/public/browser/...
File content/public/browser/indexed_db_context.h (right):

https://codereview.chromium.org/2930183002/diff/20001/content/public/browser/...
content/public/browser/indexed_db_context.h:49: const
scoped_refptr<base::SequencedTaskRunner>& task_runner) = 0;
Taking scoped_refptr by value and std::move() in impl is the prefered style
since C++11

[jsbell, 2017-06-28 18:44:27]

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

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

Dry run: CQ is trying da patch.

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

[jsbell, 2017-06-28 18:56:24]

Latest PS is just a work in progress. 

3 tests still need updating:
* IndexedDBFactoryTest.DatabaseFailedOpen - still uses SetTaskRunnerForTesting
* IndexedDBFactoryTest.DataFormatVersion - still uses SetTaskRunnerForTesting
* IndexedDBBackingStoreTest.LiveBlobJournal - commented out

All three pump the loop in the middle of steps, so the "post the entire test
body" approach doesn't trivially work. There's probably a way to allow nested
loops but I haven't stumbled on the right magic yet. Still need to ponder what
the cleanest approach is.

The IndexedDBBackingStoreTests are also a bit weird in that most work occurs on
the "main" thread and just the transaction commits get posted, which satisfies
various assertions but does not reflect normal threading.

[commit-bot: I haz the power, 2017-06-28 19:22:46]

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

[commit-bot: I haz the power, 2017-06-28 19:22:47]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jsbell, 2017-06-28 20:30:45]

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

[commit-bot: I haz the power, 2017-06-28 20:31:19]

Dry run: CQ is trying da patch.

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

[jsbell, 2017-06-28 23:40:47]

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

[commit-bot: I haz the power, 2017-06-28 23:41:10]

Dry run: CQ is trying da patch.

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

[jsbell, 2017-06-29 00:48:19]

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

[commit-bot: I haz the power, 2017-06-29 00:48:40]

Dry run: CQ is trying da patch.

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

[jsbell, 2017-06-29 00:49:35]

On 2017/06/28 18:56:24, jsbell wrote:
> Latest PS is just a work in progress. 

Still true.
 
> 3 tests still need updating:

Down to just 1:

> * IndexedDBBackingStoreTest.LiveBlobJournal - commented out

I'll tackle that next. 

> The IndexedDBBackingStoreTests are also a bit weird in that most work occurs
on
> the "main" thread and just the transaction commits get posted, which satisfies
> various assertions but does not reflect normal threading.

Also still true.

[commit-bot: I haz the power, 2017-06-29 03:02:19]

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

[commit-bot: I haz the power, 2017-06-29 03:02:20]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jsbell, 2017-06-30 18:25:38]

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

[commit-bot: I haz the power, 2017-06-30 18:26:00]

Dry run: CQ is trying da patch.

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

[jsbell, 2017-06-30 19:08:27]

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

[commit-bot: I haz the power, 2017-06-30 19:08:52]

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-06-30 20:22:25]

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

[commit-bot: I haz the power, 2017-06-30 20:22:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jsbell, 2017-06-30 23:22:46]

Description was changed from

==========
Let IndexedDBContextImpl create its own task runner

Apart from tests which need control of the task runner and can use
SetTaskRunnerForTesting(), let the Indexed DB context object create
its own sequenced task runner (by asking the task scheduler), rather
than having it injected on creation.

R=dmurph,gab,jam
BUG=552552
==========

to

==========
Let IndexedDBContextImpl create its own task runner

Have the IndexedDBContextImpl create its own sequenced task runner
(via the task scheduler), rather than having it injected on creation.

Unit tests needed updating to ensure objects (factory, backing store,
transactions, etc) are created on the appropriate sequences and that
operations (e.g. blob writes) do correct thread hopping. This
makes the tests more correct, but also more complicated.

BUG=552552
==========

[jsbell, 2017-06-30 23:22:46]

jsbell@chromium.org changed reviewers:
+ dmurph@chromium.org
- gab@chromium.org

[jsbell, 2017-06-30 23:25:09]

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

[commit-bot: I haz the power, 2017-06-30 23:25:21]

Dry run: CQ is trying da patch.

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

[jsbell, 2017-06-30 23:26:30]

dmurph@ - can you take an initial look?

I made several questionable style choices in the tests (e.g. posting the test
fixture itself, introducing a TestState object in some places rather than
subclassing the fixture, etc) and I'd like your opinions on what will leave the
code in the cleanest state.

[commit-bot: I haz the power, 2017-07-01 01:33:50]

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

[commit-bot: I haz the power, 2017-07-01 01:33:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jsbell, 2017-07-05 17:33:32]

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

[commit-bot: I haz the power, 2017-07-05 17:33:53]

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-07-05 17:45:12]

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

[commit-bot: I haz the power, 2017-07-05 17:45:14]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jsbell, 2017-07-05 22:01:22]

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

[commit-bot: I haz the power, 2017-07-05 22:02:02]

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-07-06 00:03:01]

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

[commit-bot: I haz the power, 2017-07-06 00:03:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not
started yet; builder either lacks capacity or does not exist (misspelled?))
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[dmurph, 2017-07-07 18:29:00]

https://codereview.chromium.org/2930183002/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/2930183002/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:497: struct
TestState {
I like this approach.

https://codereview.chromium.org/2930183002/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_factory_unittest.cc (right):

https://codereview.chromium.org/2930183002/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_factory_unittest.cc:652:
RunAllBlockingPoolTasksUntilIdle();
I'd be a little more comfortable if you do something like:

base::RunLoop loop;
context()->TaskRunner()->PostTask(FROM_HERE, loop.QuitClosure());
loop.Run();

This is a little more constrained & direct, where we will run past tasks and
stop when we reach that quit closure. This might be a lot of work to change
though, and if the tests aren't flaky then it's "ok" to do the rununtilidle. But
I'd like to encourage moving towards the above method.  RunUntilIdle is
apparently a recipe for flakiness.

This is up to you.

[jsbell, 2017-07-07 21:35:43]

jsbell@chromium.org changed reviewers:
+ bauerb@google.com, jam@chromium.org, rdevlin.cronin@chromium.org

[jsbell, 2017-07-07 21:35:44]

rdevlin@ - can you please review chrome/browser/extensions?

bauerb@ - can you please review chrome/browser/browsing_data?

jam@ - can you please review content/browser/storage_partition* and
content/public/browser/* ?

dmurph@ - let's chat offline re: QuitClosure; maybe we can come up with a simple
helper...

https://codereview.chromium.org/2930183002/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_factory_unittest.cc (right):

https://codereview.chromium.org/2930183002/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_factory_unittest.cc:652:
RunAllBlockingPoolTasksUntilIdle();
On 2017/07/07 18:28:59, dmurph wrote:
> I'd be a little more comfortable if you do something like:
> 
> base::RunLoop loop;
> context()->TaskRunner()->PostTask(FROM_HERE, loop.QuitClosure());
> loop.Run();

Per gab@ (email thread to scheduler-dev@) the scheduler doesn't have a message
loop so using WaitableEvents would be necessary... and I think that would make
the tests even more complex. So... would prefer to start this way. I haven't
seen any flakiness in local runs or the bots (just in the DispatcherHostTest
which was because I removed too much cleanup code)

[Devlin, 2017-07-07 23:01:55]

c/b/extensions lgtm

[jam, 2017-07-08 01:01:39]

lgtm

[jsbell, 2017-07-11 19:20:38]

jsbell@chromium.org changed reviewers:
+ bauerb@chromium.org
- bauerb@google.com

[jsbell, 2017-07-11 19:21:19]

bauerb@ - can you please review chrome/browser/browsing_data?

(sorry, used the wrong email before)

dmurph@ - anything else?

[dmurph, 2017-07-11 20:43:23]

lgtm

[Bernhard Bauer, 2017-07-12 15:29:00]

lgtm

[jsbell, 2017-07-12 16:56:20]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2017-07-12 16:56:30]

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-07-12 16:59:50]

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

[commit-bot: I haz the power, 2017-07-12 16:59:52]

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...)

[jsbell, 2017-07-12 20:58:22]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2017-07-12 20:58:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
rdevlin.cronin@chromium.org, jam@chromium.org, dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/2930183002/#ps240001
(title: "rebased")

[commit-bot: I haz the power, 2017-07-12 20:58:44]

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-07-12 23:27:14]

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

[commit-bot: I haz the power, 2017-07-12 23:27:16]

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...)

[jsbell, 2017-07-13 22:57:32]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2017-07-13 22:57:59]

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-07-14 01:13:10]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1499986652888980,
"parent_rev": "0cdabaee801fff51aa2d90258c58da52a4e8b118", "commit_rev":
"d778808fd1f91b8f33fca61481038e815fca2e98"}

[commit-bot: I haz the power, 2017-07-14 01:13:36]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1499986652888980,
"parent_rev": "917a15f28de33bbe32a2a3d08c08afbb300b8891", "commit_rev":
"bd2caa0d663427a19b674a8f88ad36cadcb5275e"}

[commit-bot: I haz the power, 2017-07-14 01:13:49]

Description was changed from

==========
Let IndexedDBContextImpl create its own task runner

Have the IndexedDBContextImpl create its own sequenced task runner
(via the task scheduler), rather than having it injected on creation.

Unit tests needed updating to ensure objects (factory, backing store,
transactions, etc) are created on the appropriate sequences and that
operations (e.g. blob writes) do correct thread hopping. This
makes the tests more correct, but also more complicated.

BUG=552552
==========

to

==========
Let IndexedDBContextImpl create its own task runner

Have the IndexedDBContextImpl create its own sequenced task runner
(via the task scheduler), rather than having it injected on creation.

Unit tests needed updating to ensure objects (factory, backing store,
transactions, etc) are created on the appropriate sequences and that
operations (e.g. blob writes) do correct thread hopping. This
makes the tests more correct, but also more complicated.

BUG=552552

Review-Url: https://codereview.chromium.org/2930183002
Cr-Commit-Position: refs/heads/master@{#486588}
Committed:
https://chromium.googlesource.com/chromium/src/+/bd2caa0d663427a19b674a8f88ad...
==========

[commit-bot: I haz the power, 2017-07-14 01:13:52]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/bd2caa0d663427a19b674a8f88ad...