Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2591643004:
Make TypedUrlSyncableService sequence-affine. (Closed)
DescriptionMake TypedUrlSyncableService sequence-affine.
This class is thread-unsafe not thread-affine. It requires
external synchronization between tasks; which a sequence brings
implicitly. It does not need to be used from a single thread.
Note: Classes that do not depend on a constant thread id (implies that
they do not use thread-local storage) are usually not thread-affine.
BUG=675800, 675631
Committed: https://crrev.com/6fce596dd212d98fba4603f1806653809fb85c30
Cr-Commit-Position: refs/heads/master@{#440115}
Patch Set 1 #Patch Set 2 : remove dependency #
Messages
Total messages: 18 (10 generated)
fdoray@chromium.org changed reviewers: + sdefresne@chromium.org
PTAL
I'm not sure this is correct. TypedUrlSyncableService does call directly to HistoryBackend. HistoryBackend explicitly says: // Internal history implementation which does most of the work of the history // system. This runs on a background thread (to not block the browser when we // do expensive operations) and is NOT threadsafe, so it must only be called // from message handlers on the background thread. Invoking on another thread // requires threadsafe refcounting. This obviously depends on whether HistoryService creates a Thread or use a base::CreateSequencedTaskRunner which is controlled by a variation as far as I can tell from reading HistoryService constructor.
On 2016/12/21 12:34:24, sdefresne ooo till 2nd Jan 17 wrote: > I'm not sure this is correct. TypedUrlSyncableService does call directly to > HistoryBackend. HistoryBackend explicitly says: > > // Internal history implementation which does most of the work of the history > // system. This runs on a background thread (to not block the browser when we > // do expensive operations) and is NOT threadsafe, so it must only be called > // from message handlers on the background thread. Invoking on another thread > // requires threadsafe refcounting. > > This obviously depends on whether HistoryService creates a Thread or use a > base::CreateSequencedTaskRunner which is controlled by a variation as far as I > can tell from reading HistoryService constructor. Currently, a DCHECK is triggered when the experiment is enabled because ThreadChecker::CalledOnValidThread fails when called from sequenced tasks. SequenceChecker::CalledOnValidSequence is equivalent to ThreadChecker::CalledOnValidThread when used from tasks running on a base::Thread, but it can also be used from sequenced tasks. It was discussed here https://crbug.com/661143 that HistoryBackend can be used from a sequence (its method can't be called concurrently and external synchronization is required between calls on different threads, but they don't need to always be called from the same thread). I'll send you a CL that updates the documentation of HistoryBackend very soon.
On 2016/12/21 13:28:58, fdoray wrote: > On 2016/12/21 12:34:24, sdefresne ooo till 2nd Jan 17 wrote: > > I'm not sure this is correct. TypedUrlSyncableService does call directly to > > HistoryBackend. HistoryBackend explicitly says: > > > > // Internal history implementation which does most of the work of the history > > // system. This runs on a background thread (to not block the browser when we > > // do expensive operations) and is NOT threadsafe, so it must only be called > > // from message handlers on the background thread. Invoking on another thread > > // requires threadsafe refcounting. > > > > This obviously depends on whether HistoryService creates a Thread or use a > > base::CreateSequencedTaskRunner which is controlled by a variation as far as I > > can tell from reading HistoryService constructor. > > Currently, a DCHECK is triggered when the experiment is enabled because > ThreadChecker::CalledOnValidThread fails when called from sequenced tasks. > SequenceChecker::CalledOnValidSequence is equivalent to > ThreadChecker::CalledOnValidThread when used from tasks running on a > base::Thread, but it can also be used from sequenced tasks. It was discussed > here https://crbug.com/661143 that HistoryBackend can be used from a sequence > (its method can't be called concurrently and external synchronization is > required between calls on different threads, but they don't need to always be > called from the same thread). > > I'll send you a CL that updates the documentation of HistoryBackend very soon. Ah, I missed the fact that SequencedChecked behave like a ThreadChecker too. lgtm then
The CQ bit was checked by fdoray@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2593803002 Patch 20001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2591643004/#ps20001 (title: "remove dependency")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make TypedUrlSyncableService sequence-affine. This class is thread-unsafe not thread-affine. It requires external synchronization between tasks; which a sequence brings implicitly. It does not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 ========== to ========== Make TypedUrlSyncableService sequence-affine. This class is thread-unsafe not thread-affine. It requires external synchronization between tasks; which a sequence brings implicitly. It does not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482332790044270,
"parent_rev": "34787dcddb25873cc4a764a34467f8dbc3304969", "commit_rev":
"b8be1bdcff23493268faecd7cd9e34739efcc846"}
Message was sent while issue was closed.
Description was changed from ========== Make TypedUrlSyncableService sequence-affine. This class is thread-unsafe not thread-affine. It requires external synchronization between tasks; which a sequence brings implicitly. It does not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 ========== to ========== Make TypedUrlSyncableService sequence-affine. This class is thread-unsafe not thread-affine. It requires external synchronization between tasks; which a sequence brings implicitly. It does not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 Review-Url: https://codereview.chromium.org/2591643004 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Make TypedUrlSyncableService sequence-affine. This class is thread-unsafe not thread-affine. It requires external synchronization between tasks; which a sequence brings implicitly. It does not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 Review-Url: https://codereview.chromium.org/2591643004 ========== to ========== Make TypedUrlSyncableService sequence-affine. This class is thread-unsafe not thread-affine. It requires external synchronization between tasks; which a sequence brings implicitly. It does not need to be used from a single thread. Note: Classes that do not depend on a constant thread id (implies that they do not use thread-local storage) are usually not thread-affine. BUG=675800, 675631 Committed: https://crrev.com/6fce596dd212d98fba4603f1806653809fb85c30 Cr-Commit-Position: refs/heads/master@{#440115} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6fce596dd212d98fba4603f1806653809fb85c30 Cr-Commit-Position: refs/heads/master@{#440115} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
