New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 739945 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Convert DatabaseTracker and DatabaseMessageFilter to use TaskScheduler

Project Member Reported by jsb...@chromium.org, Jul 6 2017

Issue description

Currently, the DatabaseTracker (browser-side support for WebSQL) uses the FILE thread. It should use TaskScheduler.

* Have DatabaseTracker mint a sequenced task runner.
* Convert DBMF over to use it
* ... 
* Profit!

---------------

Current status (2017-09-05)

* DatabaseTracker and DatabaseMessageFilter are moved to a dedicated sequence minted in the DatabaseTracker constructor - that part is done.
* Unit tests inject a task runner which overrides this, via DatabaseTracker:: set_task_runner_for_testing() - this should change to the tests using the normal runner and doing work on the appropriate sequence instead.
 

Comment 1 by jsb...@chromium.org, Jul 12 2017

Cc: michaeln@chromium.org gab@chromium.org pwnall@chromium.org
When using CreateSequencedTaskRunnerWithTraits(), running into SQLite issues on Android only:

[ RUN      ] BrowsingDataRemoverImplTest.MultipleTasks
[ERROR:connection.cc(1963)] DatabaseTracker sqlite error 1802, errno 0: disk I/O error, sql: CREATE TABLE meta(key LONGVARCHAR NOT NULL UNIQUE PRIMARY KEY, value LONGVARCHAR)
[FATAL:connection.cc(1978)] disk I/O error
#00 0x0000007f950b44af /data/app/org.chromium.native_test-1/lib/arm64/lib_content_unittests__library.so+0x00000000019b24af
...

Problem goes away if I use base::CreateSingleThreadTaskRunnerWithTraits() instead, indicating that there's some thread affinity lurking somewhere.

Likely going to tackle this in three stages:

(1) Migrate off of BrowserThread::FILE to a SingleThreadTaskRunner minted by the DatabaseTracker (but still injected by tests)

(2) Update tests to use the DatabaseTracker's task runner

(3) Figure out what's up with the Android/SQLite failure and move to TaskScheduler

The unittest problem sounds familiar, this could be a matter of a missing base::test::ScopedTaskEnvironment? See the following CL/bug for example.
https://chromium-review.googlesource.com/c/567396/5

Comment 3 by jsb...@chromium.org, Jul 12 2017

It's got a TestBrowserThreadBundle which (per gab@) gives you a ScopedTaskEnvironment "for free"

Comment 4 by jsb...@chromium.org, Jul 12 2017

(looking at the bug/CL...) Interesting. Again, per gab@ I wouldn't expect you'd need both. I'll keep an eye on that, maybe give it a try.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 24 2017

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

commit 607cb1475d625041f23200d3002c448e06239570
Author: Joshua Bell <jsbell@chromium.org>
Date: Mon Jul 24 19:17:16 2017

Move DatabaseTracker and DatabaseMessageFilter off of FILE thread

As part of the Great TaskScheduler Migration, move 'Database' (i.e.
WebSQL) operations in the browser off of the FILE thread to a
dedicated task runner.

Unit tests still override the task runner; that will be tackled in
a follow-up.

Bug: 739945
Change-Id: I7308c18fa6ff60686a3cffa62d35eb9719935477
Reviewed-on: https://chromium-review.googlesource.com/563649
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Reviewed-by: Michael Nordman <michaeln@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489043}
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/chrome/browser/browsing_data/browsing_data_database_helper.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/chrome/browser/browsing_data/browsing_data_database_helper.h
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/chrome/browser/browsing_data/mock_browsing_data_database_helper.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/chrome/browser/browsing_data/mock_browsing_data_database_helper.h
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/chrome/browser/extensions/extension_service_unittest.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/content/browser/browser_context.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/content/browser/renderer_host/database_message_filter.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/content/browser/renderer_host/database_message_filter.h
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/content/public/test/browsing_data_remover_test_util.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/content/shell/browser/layout_test/layout_test_message_filter.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/content/shell/browser/layout_test/layout_test_message_filter.h
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/sql/connection.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/storage/browser/database/database_quota_client.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/storage/browser/database/database_quota_client.h
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/storage/browser/database/database_quota_client_unittest.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/storage/browser/database/database_tracker.cc
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/storage/browser/database/database_tracker.h
[modify] https://crrev.com/607cb1475d625041f23200d3002c448e06239570/storage/browser/database/database_tracker_unittest.cc

Comment 6 by jsb...@chromium.org, Jul 25 2017

Not closing yet since tests still inject a task runner.
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 25 2017

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

commit bdfd33d478b95ae1bb831ad089c3698b0645beaa
Author: Joshua Bell <jsbell@chromium.org>
Date: Tue Jul 25 21:49:44 2017

WebSQL: Move database deletion retries to correct sequence

Follow up to https://chromium-review.googlesource.com/c/563649/

DatabaseMessageFilter and DatabaseTracker were moved off the FILE
thread to a dedicated sequence, but a case involving retries on
I/O error was missed. Fix it.

Bug: 739945
Change-Id: I2703680e1797142766f9fa8cc1a5cc8b19cb24c1
Reviewed-on: https://chromium-review.googlesource.com/585369
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489450}
[modify] https://crrev.com/bdfd33d478b95ae1bb831ad089c3698b0645beaa/content/browser/renderer_host/database_message_filter.cc

Cc: jsb...@chromium.org
Owner: ----
Status: Available (was: Started)
Moving back to available for the test updates, since I'm not working on it at the moment.
#8: While this is still on your mind, can you please update the bug description to reflect what was done / what needs to be done?
Description: Show this description
Project Member

Comment 11 by sheriffbot@chromium.org, Sep 6

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Available (was: Untriaged)

Sign in to add a comment