Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581213002:
Force HostContentSettingsMap to be destroyed on its owning thread. (Closed)
DescriptionForce HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH comparison.
2) Nothing about RefcountedKeyedService's use of its |task_runner| is thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this deletion is
racy in theory (deleting without owning the sequence means potentially deleting
things from the same SequenceToken in parallel... any good test would fail TSAN and
I also don't want to address this issue given it's long standing and SWP is being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)
Committed: https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170
Cr-Commit-Position: refs/heads/master@{#439534}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix CookieSettingsTest #Patch Set 3 : Use ScopedMockTimeMessageLoopTaskRunner in NotificationPermissionContextTest to ensure old MessageL… #Patch Set 4 : fix compile #
Total comments: 2
Patch Set 5 : s/STRH comparison/RunsTasksOnCurrentThread/ #
Messages
Total messages: 58 (37 generated)
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
BUG=674946
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@https://codereview.chromium.org/2491613004/#ps160001
BUG=674946
==========
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gab@chromium.org changed reviewers: + bauerb@chromium.org, erg@chromium.org, vmpstr@chromium.org
@vmpstr for sequenced_worker_pool.cc (copied from https://codereview.chromium.org/2491613004/#ps160001). @erg for components/keyed_service/* @bauerb for components/content_settings/core/browser/host_content_settings_map.cc Thanks, Gab
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@https://codereview.chromium.org/2491613004/#ps160001
BUG=674946
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@https://codereview.chromium.org/2491613004/#ps160001
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
keyed service lgtm
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2581213002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2581213002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:831: DCHECK(thread_checker_.CalledOnValidThread()); Should this now be a SequenceChecker? And/or have RefcountedKeyedService expose its SequencedTaskRunner to subclasses and check RunsTasksOnCurrentThread()?
https://codereview.chromium.org/2581213002/diff/1/components/content_settings... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2581213002/diff/1/components/content_settings... components/content_settings/core/browser/host_content_settings_map.cc:831: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/12/16 19:28:45, Bernhard Bauer wrote: > Should this now be a SequenceChecker? And/or have RefcountedKeyedService expose > its SequencedTaskRunner to subclasses and check RunsTasksOnCurrentThread()? The API of its constructor says it should run on UI thread so I figured keeping the checks thread-affine for now was fine (even if RefCountedKeyedService moved to be sequence-affine -- thread-affine is a subset of sequence-affine so it's okay). I welcome a refactoring to make this class sequence-friendly, but that's beyond the scope of this CL.
gab@chromium.org changed reviewers: + dcheng@chromium.org
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@https://codereview.chromium.org/2491613004/#ps160001
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=vmpstr/dcheng (base/), bauerb (content_settings)
==========
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=vmpstr/dcheng (base/), bauerb (content_settings)
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=vmpstr/dcheng (base/), bauerb (content_settings)
==========
TBR vmpstr (already reviewed @ https://codereview.chromium.org/2491613004/#ps160001 on behalf of danakj) and dcheng as R-S. TBR bauerb for content_settings/ Sorry for the rush, I want this to land tonight so I can land https://codereview.chromium.org/2576243003/ ahead of the Finch freeze and not waste two weeks of user data.
The CQ bit was unchecked by gab@chromium.org
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org Link to the patchset: https://codereview.chromium.org/2581213002/#ps40001 (title: "fix CookieSettingsTest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
OK, I guess it makes sense that the HCSM knows that it has a stronger thread guarantee internally than what its superclass requires, so it can check that. IOW, LGTM :) On Fri, Dec 16, 2016, 20:42 <gab@chromium.org> wrote: > > > https://codereview.chromium.org/2581213002/diff/1/components/content_settings... > File > components/content_settings/core/browser/host_content_settings_map.cc > (right): > > > https://codereview.chromium.org/2581213002/diff/1/components/content_settings... > components/content_settings/core/browser/host_content_settings_map.cc:831: > DCHECK(thread_checker_.CalledOnValidThread()); > On 2016/12/16 19:28:45, Bernhard Bauer wrote: > > Should this now be a SequenceChecker? And/or have > RefcountedKeyedService expose > > its SequencedTaskRunner to subclasses and check > RunsTasksOnCurrentThread()? > > The API of its constructor says it should run on UI thread so I figured > keeping the checks thread-affine for now was fine (even if > RefCountedKeyedService moved to be sequence-affine -- thread-affine is a > subset of sequence-affine so it's okay). > > I welcome a refactoring to make this class sequence-friendly, but that's > beyond the scope of this CL. > > https://codereview.chromium.org/2581213002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
base LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=vmpstr/dcheng (base/), bauerb (content_settings)
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.\
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=vmpstr/dcheng (base/), bauerb (content_settings)
==========
gab@chromium.org changed reviewers: + mukai@chromium.org
TBR mukai for side-effects on NotificationPermissionContextTest, thanks!
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2581213002/#ps60001 (title: "Use ScopedMockTimeMessageLoopTaskRunner in NotificationPermissionContextTest to ensure old MessageL…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2581213002/#ps70001 (title: "fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.\
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=vmpstr/dcheng (base/), bauerb (content_settings)
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=vmpstr/dcheng (base/), bauerb (content_settings)
==========
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=vmpstr/dcheng (base/), bauerb (content_settings)
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)
==========
https://codereview.chromium.org/2581213002/diff/70001/components/keyed_servic... File components/keyed_service/core/refcounted_keyed_service.cc (right): https://codereview.chromium.org/2581213002/diff/70001/components/keyed_servic... components/keyed_service/core/refcounted_keyed_service.cc:17: obj->task_runner_.get() != base::SequencedTaskRunnerHandle::Get()) { Do we have a guarantee that STRH's can be compared by address?
The CQ bit was checked by gab@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
@erg: PTAL at latest change to refcounted_keyed_service.cc https://codereview.chromium.org/2581213002/diff/70001/components/keyed_servic... File components/keyed_service/core/refcounted_keyed_service.cc (right): https://codereview.chromium.org/2581213002/diff/70001/components/keyed_servic... components/keyed_service/core/refcounted_keyed_service.cc:17: obj->task_runner_.get() != base::SequencedTaskRunnerHandle::Get()) { On 2016/12/19 17:07:29, Bernhard Bauer wrote: > Do we have a guarantee that STRH's can be compared by address? No we don't (in practice it's true for MessageLoop and TaskScheduler, but not for SequencedWorkerPool). But it's fine as worst case is a false negative where we post to self. You make me think though, the right thing here is to just use obj->task_runner_->RunsTasksOnCurrentThread().
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH
comparison.
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/19 18:25:56, gab wrote: > @erg: PTAL at latest change to refcounted_keyed_service.cc > > https://codereview.chromium.org/2581213002/diff/70001/components/keyed_servic... > File components/keyed_service/core/refcounted_keyed_service.cc (right): > > https://codereview.chromium.org/2581213002/diff/70001/components/keyed_servic... > components/keyed_service/core/refcounted_keyed_service.cc:17: > obj->task_runner_.get() != base::SequencedTaskRunnerHandle::Get()) { > On 2016/12/19 17:07:29, Bernhard Bauer wrote: > > Do we have a guarantee that STRH's can be compared by address? > > No we don't (in practice it's true for MessageLoop and TaskScheduler, but not > for SequencedWorkerPool). But it's fine as worst case is a false negative where > we post to self. > > You make me think though, the right thing here is to just use > obj->task_runner_->RunsTasksOnCurrentThread(). This paradigm is used in 45 places around the codebase today: https://cs.chromium.org/search/?q=if%5C+%5C(.*RunsTasksOnCurrentThread&sq=pac... TBR erg@ for paradigm change in refcounted_keyed_service.cc
The CQ bit was checked by gab@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erg@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2581213002/#ps90001 (title: "s/STRH comparison/RunsTasksOnCurrentThread/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 90001, "attempt_start_ts": 1482177658816650,
"parent_rev": "30b8804fb69c982be42bb872ed0e4b8f9ffec9d7", "commit_rev":
"55e9437837f6587656cc2e7b276d7dcb0cbb7566"}
Message was sent while issue was closed.
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH
comparison.
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH
comparison.
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)
Review-Url: https://codereview.chromium.org/2581213002
==========
Message was sent while issue was closed.
Committed patchset #5 (id:90001)
Message was sent while issue was closed.
Description was changed from
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH
comparison.
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)
Review-Url: https://codereview.chromium.org/2581213002
==========
to
==========
Force HostContentSettingsMap to be destroyed on its owning thread.
It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).
The rest of the changes can then be inferred from the stack trace on the bug:
1) RefcountedKeyedServiceTraits::Destruct() must check for
ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH
comparison.
2) Nothing about RefcountedKeyedService's use of its |task_runner| is
thread-affine
=> using SequencedTaskRunner instead is inline with our desire to make all
top-level API use sequence-affinity instead of thread-affinity for
thread-safety.
3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor
requires
the SequencedWorkerPool's task info to be set in that scope
=> brought changes from same requirements in another CL
@ https://codereview.chromium.org/2491613004/#ps160001
Note: intentionally not adding tests as this change highlights that this
deletion is
racy in theory (deleting without owning the sequence means potentially
deleting
things from the same SequenceToken in parallel... any good test would fail
TSAN and
I also don't want to address this issue given it's long standing and SWP is
being
replaced by TaskScheduler shortly).
4) Switch to ScopedMockTimeMessageLoopTaskRunner in
NotificationPermissionContextTest
=> otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap
is
sent for deletion on the wrong task runner and the task is never flushed.
(ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before
replacing
it, both ways).
Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.
BUG=674946, 665588, 622400
TBR=mukai (NotificationPermissionContextTest side-effects)
Committed: https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170
Cr-Commit-Position: refs/heads/master@{#439534}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170 Cr-Commit-Position: refs/heads/master@{#439534}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:90001) has been created in https://codereview.chromium.org/2587883003/ by yoichio@chromium.org. The reason for reverting is: This might cause flakiness: https://bugs.chromium.org/p/chromium/issues/detail?id=675722. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
