Unsafe usage of sql::Statement in //components/history/core/browser/android and //chrome/browser/history/android/ |
||
Issue descriptionsql::Statement is thread-unsafe, and must be used from the same sequence. Adding sequence checkers in https://crrev.com/c/1137851/4 produced the following test failures: unit_tests: SQLiteCursorTest.Run AndroidHistoryProviderServiceTest.TestHistoryAndBookmark AndroidHistoryProviderServiceTest.TestSearchTerm chrome_public_test_apk: org.chromium.chrome.browser.provider.ProviderSearchesUriTest#testAddSearchTerm org.chromium.chrome.browser.provider.ProviderSearchesUriTest#testDeleteSearchTerm org.chromium.chrome.browser.provider.ProviderSearchesUriTest#testSearchesTable org.chromium.chrome.browser.provider.ProviderBookmarksUriTest#testAddBookmark org.chromium.chrome.browser.provider.ProviderBookmarksUriTest#testUpdateBookmark org.chromium.chrome.browser.provider.ProviderSearchesUriTest#testUpdateSearchTerm org.chromium.chrome.browser.provider.ProviderBookmarksUriTest#testBookmarksTable org.chromium.chrome.browser.provider.ProviderBookmarksUriTest#testQueryBookmark Examples of full test runs on Android: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-kitkat-arm-rel/40839 https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/44631
,
Jul 23
+michaelbai for historical knowledge of these tests +yfriedman for yet another reason I want these to be deleted. At least for the ProviderSearchesUriTest, it looks like the test is primarily executed from the test thread and not Android's UI thread. I don't know the expectation of what thread it "should" be on though.
,
Jul 23
Those tests use the content provide client API, IIRC, it doesn't matter which thread they are running.
,
Jul 24
#3: it may not matter which thread is servicing the requesting (i believe the content providers use a threadpool and thus need to respond on multiple) but we still need to be consistent with a given thread's usage of sql::Statement. It requires further investigation because it may be a test-only issue though as we're somewhat sloppy with testing threading (instrumentation/test thread vs ui thread) If we have a nested message loop, would that satisfy the SequenceChecker or would they be different sequences? That might be what's tripping up the native tests. Haven't looked at the instrumentation failures |
||
►
Sign in to add a comment |
||
Comment 1 by sky@chromium.org
, Jul 21