Reject ownerless receivers on base::Bind |
|||
Issue description
There's a common thread race bug pattern around PostTask and base::Bind usage in a constructor of ref-counted objects:
In the example below, `new Foo` may return a stale pointer if PostTask failed or the posted task ran soon before Foo::Foo is finished.
Foo::Foo() {
PostTask(FROM_HERE, base::BindOnce(&Foo::Bar, this));
}
To catch most of the failing case, we can/should check if the ref count is more than zero, so that the instance keeps alive even if the asynchronous task released the reference earlier.
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a1d9b6678ace05ad0a8a92a22fc628aea6af65f9 commit a1d9b6678ace05ad0a8a92a22fc628aea6af65f9 Author: tzik <tzik@chromium.org> Date: Tue Jul 24 01:40:58 2018 Remove fake ref count from ViewEventTestBase ViewEventTestBase has no-op AddRef and Release for a historical reason, but they are no longer needed for years. Bug: 866456 Change-Id: Iea74ee9c87667f033fc90d47f2d331a202ab61d8 Reviewed-on: https://chromium-review.googlesource.com/1146538 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577400} [modify] https://crrev.com/a1d9b6678ace05ad0a8a92a22fc628aea6af65f9/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc [modify] https://crrev.com/a1d9b6678ace05ad0a8a92a22fc628aea6af65f9/chrome/test/base/view_event_test_base.cc [modify] https://crrev.com/a1d9b6678ace05ad0a8a92a22fc628aea6af65f9/chrome/test/base/view_event_test_base.h
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/570f47a2227f9a1aee0e3cbb821c61a77f95fc42 commit 570f47a2227f9a1aee0e3cbb821c61a77f95fc42 Author: tzik <tzik@chromium.org> Date: Tue Jul 24 09:16:57 2018 Avoid touching dbus::ObjectManager ref count before it's fully cosntructed dbus::ObjectManager's reference count is zero at the beginning of its constructor, and used to be incremented at base::Bind there implicitly. However, if PostTaskAndReplyWithResult there failed, the reference is gone and the reference count gets to 0 again. And as the result, `new ObjectManager` may return a stale pointer. This CL adds a static constructor to ObjectManager, and moves the ref count manipulation part out from the constructor. Bug: 866456 Change-Id: I5dd8947a5087359005cbcb75e1bb2a0a42ebeda8 Reviewed-on: https://chromium-review.googlesource.com/1148037 Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#577465} [modify] https://crrev.com/570f47a2227f9a1aee0e3cbb821c61a77f95fc42/dbus/BUILD.gn [modify] https://crrev.com/570f47a2227f9a1aee0e3cbb821c61a77f95fc42/dbus/bus.cc [delete] https://crrev.com/96e7e0b037345fc2b76d8a7f1cf039c64074c67e/dbus/mock_object_manager.cc [delete] https://crrev.com/96e7e0b037345fc2b76d8a7f1cf039c64074c67e/dbus/mock_object_manager.h [modify] https://crrev.com/570f47a2227f9a1aee0e3cbb821c61a77f95fc42/dbus/object_manager.cc [modify] https://crrev.com/570f47a2227f9a1aee0e3cbb821c61a77f95fc42/dbus/object_manager.h
,
Jul 24
This is awesome, great work Taiju, thanks!
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/45bf149390f1b6047b035566a438504b47f3bd06 commit 45bf149390f1b6047b035566a438504b47f3bd06 Author: tzik <tzik@chromium.org> Date: Tue Jul 24 18:18:35 2018 Hold a reference to ShaderDiskCache while its Init() is called ShaderDiskCache's reference count used to be 0 at the beginning of Init(), and be incremented implicitly by base::Bind there. As the reference count is decremented to 0 if CreateCacheBackend finished immediately and the passed callback is destroyed. That causes unexpected destruction of the SharedDiskCache instance. After this CL, the caller of Init() keeps an reference while Init() is running to avoid destroying the ShaderDiskCache instance. Bug: 866456 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: Iddf9daaf09119aed279ffb81f0af1a74efb9dc54 Reviewed-on: https://chromium-review.googlesource.com/1148090 Reviewed-by: Jonathan Backer <backer@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577613} [modify] https://crrev.com/45bf149390f1b6047b035566a438504b47f3bd06/gpu/ipc/host/shader_disk_cache.cc
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f994a90a78bce4d3b4a2109b713ae3be0110395c commit f994a90a78bce4d3b4a2109b713ae3be0110395c Author: tzik <tzik@chromium.org> Date: Tue Jul 24 18:46:48 2018 Avoid binding ref counted objects in ScreenObserverDelegate constructor This CL adds a static constructor to ScreenObserverDelegate to avoid a racy ref count operation. As ScreenObserverDelegate is a ref counted class, base::Bind implicitly increments the reference count, and releases it on the callback instance destruction. If PostTask in the SOD ctor fails, or the posted task ran soon before the ctor has completed, the ScreenObserverDelegate instance may be destroyed immediately, and `new ScreenObserverDelegate` may returns a stale pointer. Bug: 866456 Change-Id: I7c64915529786b2f0738731c323aaf2f293dbc20 Reviewed-on: https://chromium-review.googlesource.com/1147896 Reviewed-by: Christian Fremerey <chfremer@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577624} [modify] https://crrev.com/f994a90a78bce4d3b4a2109b713ae3be0110395c/media/capture/video/chromeos/display_rotation_observer.cc [modify] https://crrev.com/f994a90a78bce4d3b4a2109b713ae3be0110395c/media/capture/video/chromeos/display_rotation_observer.h [modify] https://crrev.com/f994a90a78bce4d3b4a2109b713ae3be0110395c/media/capture/video/chromeos/video_capture_device_chromeos_halv3.cc [modify] https://crrev.com/f994a90a78bce4d3b4a2109b713ae3be0110395c/media/capture/video/linux/video_capture_device_chromeos.cc
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/782ece688e3c05119b34dad597b90beca5d0d7b6 commit 782ece688e3c05119b34dad597b90beca5d0d7b6 Author: tzik <tzik@chromium.org> Date: Tue Jul 24 19:36:43 2018 Avoid touching ChainedBlobWriterImpl's ref count before it's fully constructed ChainedBlobWriterImpl is a ref-counted object, and its first reference used to be taken by base::BindOnce implicitly in its constructor. The reference is posted to another thread, and released after the task has run. However, if the task finished before the constructor finished, the object is destroyed immediately, and `new ChainedBlobWriterImpl` may return a stale pointer. This CL adds a static constructor to ChainedBlobWriterImpl to keep a reference while the task is posted. Bug: 866456 Change-Id: If8fee2e8944ce550e766b5269d47878f357db6c5 Reviewed-on: https://chromium-review.googlesource.com/1148098 Reviewed-by: Daniel Murphy <dmurph@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577638} [modify] https://crrev.com/782ece688e3c05119b34dad597b90beca5d0d7b6/content/browser/indexed_db/indexed_db_backing_store.cc
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3bc7dce54257dc862bd8515972ac95d8f78b088c commit 3bc7dce54257dc862bd8515972ac95d8f78b088c Author: tzik <tzik@chromium.org> Date: Wed Jul 25 01:56:01 2018 Avoid touching ExtendedAuthenticatorImpl's ref count before its fully constructed ExtendedAuthenticatorImpl is a ref counted object. Its first reference is taken by base::Bind in its constructor, and the reference is passed to GetSystemSalt to do an asynchronous task. However, if GetSystemSalt impl drops the passed callback soon, (e.g. by PostTask failure), the reference to ExtendedAuthenticatorImpl is gone and the instance is destroyed, even before the construction has completed. That is, `new ExtendedAuthenticatorImpl` might return a stale pointer on the previous code. This CL adds a static constructor to ExtendedAuthenticatorImpl, and moves the problematic part out of the constructor. Bug: 866456 Change-Id: I377acc8a753c01a79a4cf09617bfa3b0f6aa923f Reviewed-on: https://chromium-review.googlesource.com/1148164 Reviewed-by: Antoine Labour <piman@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577766} [modify] https://crrev.com/3bc7dce54257dc862bd8515972ac95d8f78b088c/chromeos/login/auth/extended_authenticator.cc [modify] https://crrev.com/3bc7dce54257dc862bd8515972ac95d8f78b088c/chromeos/login/auth/extended_authenticator_impl.cc [modify] https://crrev.com/3bc7dce54257dc862bd8515972ac95d8f78b088c/chromeos/login/auth/extended_authenticator_impl.h
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/53f933e87f1c0f284d0ad640ebb4b085640dbd30 commit 53f933e87f1c0f284d0ad640ebb4b085640dbd30 Author: tzik <tzik@chromium.org> Date: Wed Jul 25 01:56:04 2018 Avoid touching AddressSorterWin::Job's ref count before it's fully constructed AddressSorterWin::Job's reference count is zero at the beginning of its constructor, and used to be incremented at base::Bind there implicitly. However, if PostTaskAndReply there failed, the reference is gone and the reference count gets to 0 again. And as the result, `new Job` may return a stale pointer. This CL adds a static constructor to , and moves the ref count manipulation part out from the constructor, in order to avoid the unexpected destruction. Bug: 866456 Change-Id: I1e735d6d0bf427988ba7380569628d4264c755f5 Reviewed-on: https://chromium-review.googlesource.com/1147898 Reviewed-by: Matt Menke <mmenke@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577767} [modify] https://crrev.com/53f933e87f1c0f284d0ad640ebb4b085640dbd30/net/dns/address_sorter_win.cc
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6639fc0aec33344ce8159b8b463037117479510c commit 6639fc0aec33344ce8159b8b463037117479510c Author: tzik <tzik@chromium.org> Date: Wed Jul 25 14:14:05 2018 Avoid touching ConnectivityCheckerImpl's ref count before it's fully constructed The first reference to ConnectivityCheckerImpl used to made by base::Bind implicitly in the constructor. The reference is moved to a task runner, and released when the resulting callback object is destroyed. However, the posted task may be immediately destroyed if the PostTask failed, or the posted task may run soon before the constructor finished. That implies, `new ConnectivityCheckerImpl` may return a stale pointer. This CL adds a static constructor to ConnectivityCheckerImpl, and moves the setup code that touches ref count out of the ctor. Bug: 866456 Change-Id: I5ea6f7cf80175abb63f6318e454075f2678ff45d Reviewed-on: https://chromium-review.googlesource.com/1149437 Reviewed-by: Luke Halliwell <halliwell@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577873} [modify] https://crrev.com/6639fc0aec33344ce8159b8b463037117479510c/chromecast/net/connectivity_checker.cc [modify] https://crrev.com/6639fc0aec33344ce8159b8b463037117479510c/chromecast/net/connectivity_checker_impl.cc [modify] https://crrev.com/6639fc0aec33344ce8159b8b463037117479510c/chromecast/net/connectivity_checker_impl.h
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b1dda104f92db54ab30dfb2912d9d552bc5e0073 commit b1dda104f92db54ab30dfb2912d9d552bc5e0073 Author: tzik <tzik@chromium.org> Date: Wed Jul 25 16:40:57 2018 Avoid touching NestedSubscription's ref count before it's fully constructed NestedSubscription is a ref-counted object, and its first reference used to be made by base::BindOnce in its constructor. The reference is passed to another thread, and released when the callback instance is destroyed. However, if the PostTask failed or the posted task ran soon before the constructor finished to construct the NestedSubscription instance, the ref count is decremented to 0, and `new NestedSubscription` may return a stale pointer. This CL adds a static constructor to avoid that by splitting the ref-count related set up out of the constructor. Bug: 866456 Change-Id: Idf03b31b95b4a7ddee81fdebff78a594e52a62f8 Reviewed-on: https://chromium-review.googlesource.com/1149762 Reviewed-by: Richard Coles <torne@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577933} [modify] https://crrev.com/b1dda104f92db54ab30dfb2912d9d552bc5e0073/android_webview/browser/net/aw_cookie_change_dispatcher_wrapper.cc
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9861bc62651d47e5ede4deea4324e3a0391631e commit b9861bc62651d47e5ede4deea4324e3a0391631e Author: tzik <tzik@chromium.org> Date: Wed Jul 25 17:35:13 2018 Avoid touching ResourceRequestDetectorClient's ref count before it's fully constructed The first reference to ResourceRequestDetectorClient instance used to be made by base::Bind in its constructor, and posted to another thread. If the PostTask call failed, or the posted task ran immediately after the other thread, the RRDC instance may destroyed before the construction is completed. Though this does not actually causes a crash, as no one use the result of `new ResourceRequestDetectorClient`, this hits an assertion that being added as a false positive case. This CL adds ResourceRequestDetectorClient::Start, and makes the constructor private to ensure no one touches the fragile way. Also, this converts legacy base::Callback to base::OnceCallback, as a drive-by. Bug: 866456 Change-Id: Ifb3d8136f03e2e233cf4b0b2b0ee070db6b3093d Reviewed-on: https://chromium-review.googlesource.com/1149779 Reviewed-by: Jialiu Lin <jialiul@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577961} [modify] https://crrev.com/b9861bc62651d47e5ede4deea4324e3a0391631e/chrome/browser/safe_browsing/incident_reporting/resource_request_detector.cc
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ed031ddf9a7693760399450dfa8631de505ee49d commit ed031ddf9a7693760399450dfa8631de505ee49d Author: tzik <tzik@chromium.org> Date: Wed Jul 25 17:35:16 2018 Avoid touching ThreatDetails's ref-count before it's fully constructed ThreatDetails is a ref counted object, and its first reference can be made by base::BindRepeating through ThreadDetails::StartCollection. The ref count is released when the callback instance is destroyed. As the RepeatingCallback instance created in StartCollection is destroyed after the scope out, `new ThreatDetails` may have returned a stale pointer. This CL moves the problematic StartCollection() call out of the constructor to avoid touching the ref count before its first proper reference is made. Bug: 866456 Change-Id: I82b83b9bb30cdc95cff60e3e22e79f0af17645a2 Reviewed-on: https://chromium-review.googlesource.com/1149777 Reviewed-by: Jialiu Lin <jialiul@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#577962} [modify] https://crrev.com/ed031ddf9a7693760399450dfa8631de505ee49d/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc [modify] https://crrev.com/ed031ddf9a7693760399450dfa8631de505ee49d/chrome/browser/safe_browsing/threat_details_unittest.cc [modify] https://crrev.com/ed031ddf9a7693760399450dfa8631de505ee49d/components/safe_browsing/browser/threat_details.cc [modify] https://crrev.com/ed031ddf9a7693760399450dfa8631de505ee49d/components/safe_browsing/browser/threat_details.h [modify] https://crrev.com/ed031ddf9a7693760399450dfa8631de505ee49d/components/safe_browsing/triggers/trigger_manager_unittest.cc
,
Jul 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0f58446e515be296942ffc627c8ed060205c7721 commit 0f58446e515be296942ffc627c8ed060205c7721 Author: tzik <tzik@chromium.org> Date: Thu Jul 26 00:53:03 2018 Use OAuth2 FailCaller after its construction is completed `new FailCaller` in oauth2_access_token_fetcher_immediate_error.cc may return a stale pointer if PostTask in its ctor failed, since base::Bind there increments the ref count, and the callback destruction releases it. This CL moves the PostTask after the completion of the ctor call. Bug: 866456 Change-Id: Id185e46292238955b3da9015903dfa6a8c94e7e9 Reviewed-on: https://chromium-review.googlesource.com/1146345 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#578150} [modify] https://crrev.com/0f58446e515be296942ffc627c8ed060205c7721/google_apis/gaia/oauth2_access_token_fetcher_immediate_error.cc
,
Jul 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/56d606947d05764a2effb644561263f8c77372dd commit 56d606947d05764a2effb644561263f8c77372dd Author: tzik <tzik@chromium.org> Date: Thu Jul 26 03:41:48 2018 Avoid a reference cycle between SSLPrivateKeyInternal and mojom::SSLPrivateKey SSLPrivateKeyInternal owns mojom::SSLPrivateKey, and mojom::SSLPrivateKey has a reference to SSLPrivateKeyInternal. That keeps both of them alive until a connection error happens. This CL removes the reference from mojom::SSLPK to SSLPKI to remove the cycle. Bug: 866456 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: Iab0a8bcd16663520873b6b7b29d9326755d93600 Reviewed-on: https://chromium-review.googlesource.com/1148095 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#578186} [modify] https://crrev.com/56d606947d05764a2effb644561263f8c77372dd/services/network/url_loader.cc
,
Jul 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fbd27cfaeee3b95ac5d374310eb3639c8acae054 commit fbd27cfaeee3b95ac5d374310eb3639c8acae054 Author: tzik <tzik@chromium.org> Date: Thu Jul 26 16:20:22 2018 Avoid touching TimeZoneMonitor's ref count before it's fully constructed TimeZoneMonitorLinuxImpl is a ref counted type, and its first reference used to be made by base::Bind implicitly. The reference was passed to |file_task_runner_| and released on the task destruction. However, if the PostTask failed or the posted task ran soon before the ctor of TimeZoneMonitorLinuxImpl has finished, the instance is destroyed immediately, and `new TimeZoneMonitorLinuxImpl` returns a stale pointer. This CL adds a static constructor to move the problematic ref count manipulation out of the constructor. Bug: 866456 Change-Id: I5000cfa493cabe8858f20517328fe1d6e27795ff Reviewed-on: https://chromium-review.googlesource.com/1150959 Reviewed-by: Mark Mentovai <mark@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#578327} [modify] https://crrev.com/fbd27cfaeee3b95ac5d374310eb3639c8acae054/services/device/time_zone_monitor/time_zone_monitor_linux.cc
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b commit ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b Author: tzik <tzik@chromium.org> Date: Fri Jul 27 00:59:12 2018 Avoid storing rtc::RefCountInterface to base::scoped_refptr Some WebRTC's ref-counted objects are implicitly stored into base::scoped_refptr instead of rtc::scoped_refptr through base::Bind. And an upcoming base::Bind update disables this case of auto wrapping. This CL replaces it to rtc::scoped_refptr by wrapping the pointer with rtc::scoped_refptr explicitly. Bug: 866456 Change-Id: Id8a9dd622f5aa5b9c843ec7ec43e0d390a0777db Reviewed-on: https://chromium-review.googlesource.com/1145147 Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Henrik Boström <hbos@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#578498} [modify] https://crrev.com/ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b/content/renderer/media/stream/media_stream_audio_processor.cc [modify] https://crrev.com/ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b/content/renderer/media/webrtc/rtc_peer_connection_handler.cc [modify] https://crrev.com/ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b/content/renderer/media/webrtc/rtc_stats.cc [modify] https://crrev.com/ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b/content/renderer/media/webrtc/webrtc_audio_renderer.cc [modify] https://crrev.com/ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b/remoting/host/win/rdp_client_window.cc [modify] https://crrev.com/ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b/remoting/protocol/webrtc_audio_module.cc [modify] https://crrev.com/ecb28473dbb74e8df15e48788b4a2ce55ab1cc1b/third_party/webrtc_overrides/rtc_base/task_queue.cc
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1d5d38a7b752ad2e7db100e6fbe6784ec281f830 commit 1d5d38a7b752ad2e7db100e6fbe6784ec281f830 Author: tzik <tzik@chromium.org> Date: Fri Jul 27 04:40:10 2018 Avoid touching bookmarks::ModelLoader's ref count before it's fully constructed ModelLoader is a ref counted type, and its first reference used to be taken in its constructor through base::BindOnce. The reference was passed to a task runner, and released after the task has run. However, if the PostTask failed or the posted task ran soon before the construction had completed, the ModelLoader instance can be destroyed before another reference is made on the original sequence. So, `new ModelLoader` can return a stale pointer. This CL adds a static constructor to ModelLoader, and makes the first reference on the original sequence before passing a reference to the other sequence. Bug: 866456 Change-Id: I4d3c954ca39b7187fbd651c498e17273024c9968 Reviewed-on: https://chromium-review.googlesource.com/1151173 Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#578537} [modify] https://crrev.com/1d5d38a7b752ad2e7db100e6fbe6784ec281f830/components/bookmarks/browser/bookmark_model.cc [modify] https://crrev.com/1d5d38a7b752ad2e7db100e6fbe6784ec281f830/components/bookmarks/browser/model_loader.cc [modify] https://crrev.com/1d5d38a7b752ad2e7db100e6fbe6784ec281f830/components/bookmarks/browser/model_loader.h
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d110229385af383d23bcdb2f16f9c391b4541b1 commit 4d110229385af383d23bcdb2f16f9c391b4541b1 Author: tzik <tzik@chromium.org> Date: Fri Jul 27 04:56:51 2018 Keep reference to DOMStorageNamespace while it's being cloned While DOMStorageNamespace::Clone constructs an instance, it binds it to a callback, post it to a task runner and returns the instance as a raw pointer. Note that base::BindOnce here retains a reference to |clone| and releases the reference when the callback instance is destroyed. However, if PostTaskAndReply there failed, the callback instance is destroyed immediately and DOMStorageNamespace loses the last reference. Then, DOMStorageNamespace::Clone may return a stale pointer. This CL converts the return value to scoped_refptr, and has Clone() to keep the reference to the resulting instance. Bug: 866456 , 867306 Change-Id: Ic3a5a02e266bf55f8ad3c4f901eb1eebc2ea9d8e Reviewed-on: https://chromium-review.googlesource.com/1146409 Reviewed-by: Daniel Murphy <dmurph@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577377}(cherry picked from commit e17c5244514c97df94a26f17ecf25af135ef6c49) Reviewed-on: https://chromium-review.googlesource.com/1152588 Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#140} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/4d110229385af383d23bcdb2f16f9c391b4541b1/content/browser/dom_storage/dom_storage_namespace.cc [modify] https://crrev.com/4d110229385af383d23bcdb2f16f9c391b4541b1/content/browser/dom_storage/dom_storage_namespace.h
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f9813f399244b7dcb38caff3e6eb898942425d1 commit 1f9813f399244b7dcb38caff3e6eb898942425d1 Author: tzik <tzik@chromium.org> Date: Mon Jul 30 20:09:00 2018 Avoid passing ownerless RefCounted type to base::Bind An upcoming change to base::Bind implementation rejects ref counted objects with zero ref count, as this likely causes an error. This CL updates false positive cases of the check. They are safe, but will hit the check, as they passes newly created ref counted object as the method receiver. Bug: 866456 Change-Id: Ib5a2ca3b4d7d745a68452fc391b7a51c9f4a28f1 Reviewed-on: https://chromium-review.googlesource.com/1154849 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#579141} [modify] https://crrev.com/1f9813f399244b7dcb38caff3e6eb898942425d1/base/message_loop/message_loop_unittest.cc [modify] https://crrev.com/1f9813f399244b7dcb38caff3e6eb898942425d1/chrome/browser/bookmarks/bookmark_html_writer.cc [modify] https://crrev.com/1f9813f399244b7dcb38caff3e6eb898942425d1/chrome/browser/ui/webui/welcome_win10_handler.cc [modify] https://crrev.com/1f9813f399244b7dcb38caff3e6eb898942425d1/media/base/test_helpers.cc [modify] https://crrev.com/1f9813f399244b7dcb38caff3e6eb898942425d1/remoting/protocol/fake_desktop_capturer.cc
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2d2b9225a67898e8ea0a12587fac41e2d350c377 commit 2d2b9225a67898e8ea0a12587fac41e2d350c377 Author: tzik <tzik@chromium.org> Date: Tue Jul 31 04:50:36 2018 Keep shared web::test::HttpServer instance alive The web::test::HttpServer instance returned by GetSharedInstance has zero reference count initially, HttpServer::StartOrDie implicitly increments the reference count through base::Bind, and its embedded_test_server_ destruction decrements the reference count. That is, once the server stopped, the shared instance is destroyed, and GetSharedInstance() starts returning a stale pointer. After this CL, the resulting instance of GetSharedInstance has non-zero reference count, so that it keeps alive after server stop. Bug: 866456 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I1f13dff1e86d08efb28b5805ae73426b874a4f80 Reviewed-on: https://chromium-review.googlesource.com/1146881 Reviewed-by: Eugene But <eugenebut@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#579314} [modify] https://crrev.com/2d2b9225a67898e8ea0a12587fac41e2d350c377/ios/web/public/test/http_server/http_server.mm
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e3e1ca1ea1f2dcd205ed89816eba5c01584ebbad commit e3e1ca1ea1f2dcd205ed89816eba5c01584ebbad Author: tzik <tzik@chromium.org> Date: Tue Jul 31 09:26:55 2018 Avoid touching ScopedFileOpener::Runner's ref count before it's fully constructed ScopedFileOpener::Runner is a ref counted object, and its first reference used to be made in its constructor through base::Bind. The reference is passed to ProvidedFileSystem::OpenFile, and released when the callback object is destroyed. However, if OpenFile failed, the reference is released before the Runner construction has done. Then `new Runner` returns a stale pointer, instead of newly created object. This CL adds a static consntructor and moves the implicit ref count manipulation out of the real constructor. Bug: 866456 Change-Id: I92d68b3383d8fecd900be080cc23551cddb5f12b Reviewed-on: https://chromium-review.googlesource.com/1156325 Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#579359} [modify] https://crrev.com/e3e1ca1ea1f2dcd205ed89816eba5c01584ebbad/chrome/browser/chromeos/file_system_provider/scoped_file_opener.cc
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ebe5a8172c154497a40fe2a7ab6a35412ba4d35d commit ebe5a8172c154497a40fe2a7ab6a35412ba4d35d Author: tzik <tzik@chromium.org> Date: Tue Jul 31 16:39:08 2018 Avoid touching SafeBrowsingClientImpl's ref count before it's fully constructed SafeBrowsingContextImpl is ref counted, and its first reference used to be made in its constructor through base::Bind. The reference is passed to the IO thread, and released when the task is destroyed. However, if PostTask failed or the posted task ran soon even before the constructor has finished, the SafeBrowsingContextImpl instance may be destroyed before the construction has completed. Though the SafeBrowsingContextImpl case is safe, even if `new SafeBrowsingContextImpl` returns a stale pointer, this hits an sanity check that is being added to base::Bind. This CL moves the PostTask() out of the constructor, and holds an reference to the instance while base::Bind is called to avoid the check failure. Bug: 866456 Change-Id: Iee58de1e4cd807ff858985f9831e48f4a903bbd4 Reviewed-on: https://chromium-review.googlesource.com/1156332 Reviewed-by: Reilly Grant <reillyg@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#579433} [modify] https://crrev.com/ebe5a8172c154497a40fe2a7ab6a35412ba4d35d/chrome/browser/extensions/blacklist.cc
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c92c98d1e6fa95a0ea268eae23d003fc279acb7d commit c92c98d1e6fa95a0ea268eae23d003fc279acb7d Author: tzik <tzik@chromium.org> Date: Tue Jul 31 18:08:20 2018 Avoid touching DevTool DiscoveryRequest's ref count before it's fully constructed An upcoming change to base::Bind rejects binding ref counted objects if their ref count is zero, as this is an error prone pattern. And DevToolsDeviceDiscovery::DiscoveryRequest hits the check, as its first reference is made in its constructor through base::Bind, though its usage is valid and the check failure is a false positive case. To avoid the check failure, this CL moves base::Bind out from the constructor, and keeps a reference to the instance separately. Bug: 866456 Change-Id: Ic90d78981ac03accbb98e5536b64b576f8147f09 Reviewed-on: https://chromium-review.googlesource.com/1156328 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#579480} [modify] https://crrev.com/c92c98d1e6fa95a0ea268eae23d003fc279acb7d/chrome/browser/devtools/device/devtools_device_discovery.cc
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9efb2216a07f625f86eaff914003d0f696ec694 commit b9efb2216a07f625f86eaff914003d0f696ec694 Author: tzik <tzik@chromium.org> Date: Tue Jul 31 20:23:44 2018 Avoid touching TrackedPreferencesMigrator's ref count before it's fully constructed The first reference to a TrackedPreferencesMigrator instance used to be made in its constructor implicitly. Though this case is safe as the reference keeps alive until the other reference is made, it's error prone to make the first reference in the ctor, and an upcoming change to base::Bind rejects to make the first reference. This CL moves the implicit ref count manipulation through base::Bind() out of the constructor, so that we can avoid making the implicit first reference creation. Bug: 866456 Change-Id: I3563296c0f972d57ad8cc0c2f6e32658b9d6293d Reviewed-on: https://chromium-review.googlesource.com/1156706 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#579550} [modify] https://crrev.com/b9efb2216a07f625f86eaff914003d0f696ec694/services/preferences/tracked/tracked_preferences_migration.cc
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50039cd3c20a92d21d199dbeaa6b51ac470bb6b2 commit 50039cd3c20a92d21d199dbeaa6b51ac470bb6b2 Author: tzik <tzik@chromium.org> Date: Wed Aug 01 01:14:25 2018 Avoid touching extensions::ContentVerifyJob's ref count before it's fully constructed extensions::ContentVerifyJob is ref counted, and its first reference may be implicitly created in ContentVerifyJob::Start through base::BindOnce. However, after an upcoming change to base::Bind, it starts rejecting to make the implicit first reference to a ref-counted object, as this is an error prone pattern. This CL updates callers of ContentVerifyJob::Start to hold to hold the first reference, before Start() is called. Bug: 866456 Change-Id: Ib3a70973ac789f60a75e636db03f4e4d66e12603 Reviewed-on: https://chromium-review.googlesource.com/1156704 Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#579653} [modify] https://crrev.com/50039cd3c20a92d21d199dbeaa6b51ac470bb6b2/extensions/browser/extension_protocols.cc
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/38f920aa1b6e8ffcc4cb7756313f2c118422c0f8 commit 38f920aa1b6e8ffcc4cb7756313f2c118422c0f8 Author: tzik <tzik@chromium.org> Date: Wed Aug 01 04:41:20 2018 Avoid Bind()ing AsyncDelegateLogger when it has no owner AsyncDelegateLogger is a ref counted class, and its instance is destroyed when all of its reference is released. So, an unexpected first reference creation may cause an unexpected early destruction. AsyncDelegateLogger::Start() used to run before the first reference is made, and the first reference used to be made by base::Bind in Start() implicitly. The reference is released when the callback instance is destroyed, so the instance may be destroyed in Start(). Though this case is safe, as no one touch the instance after Start(), an upcoming change to base::Bind() disables this pattern of implicit first reference creation as it's error prone. After this CL, AsyncDelegateLogger::Run() holds the reference to avoid implicit first reference creation, and to avoid the check failure. Bug: 866456 Change-Id: I719dd6f398f2d3ce4f5d3243a326b4f2d792381e Reviewed-on: https://chromium-review.googlesource.com/1156705 Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#579701} [modify] https://crrev.com/38f920aa1b6e8ffcc4cb7756313f2c118422c0f8/net/url_request/url_request_unittest.cc
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/58faa7485fe712d0b4c483e07652c4084e23a773 commit 58faa7485fe712d0b4c483e07652c4084e23a773 Author: tzik <tzik@chromium.org> Date: Wed Aug 01 10:54:18 2018 Stop binding ownerless ref-count objects in //content An upcoming change to base::Bind rejects binding ref counted objects when their ref count is zero, as that pattern of usage is error prone. This CL updates that pattern in //content. For DOMStorageContextWrapper, the instance and a MemoryPressureListener used to own each other implicitly. And, after this CL, DOMStorageContextWrapper owns MemoryPressureListener. For PlatformNotificationContextImpl, its Initialize() makes a reference to the instance, and drops it soon. So, without keeping a reference in caller, Initialize() may destroy the instance. For WebContentsImpl::RequestAXTreeSnapshot, this should also keep a reference to the instance, as AXTreeSnapshotCombiner::AddFrame called by RecursiveRequestAXTreeSnapshotOnFrame makes a reference to AXTreeSnapshotCombiner, and releases the reference after the method call. So, that destroys AXTreeSnapshotCombiner instance implicitly. SW's StoppedObserever and TestFileErrorInjector is the false positive cases of the check. Bug: 866456 Change-Id: Ie99c996fdcf2413b170de2a246d63c34433a24ac Reviewed-on: https://chromium-review.googlesource.com/1156200 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#579754} [modify] https://crrev.com/58faa7485fe712d0b4c483e07652c4084e23a773/content/browser/dom_storage/dom_storage_context_wrapper.cc [modify] https://crrev.com/58faa7485fe712d0b4c483e07652c4084e23a773/content/browser/notifications/platform_notification_context_unittest.cc [modify] https://crrev.com/58faa7485fe712d0b4c483e07652c4084e23a773/content/browser/web_contents/web_contents_impl.cc [modify] https://crrev.com/58faa7485fe712d0b4c483e07652c4084e23a773/content/public/test/service_worker_test_helpers.cc [modify] https://crrev.com/58faa7485fe712d0b4c483e07652c4084e23a773/content/public/test/test_file_error_injector.cc
,
Aug 1
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a50a34bc08a2af326911f746979bd3f98c8875fa commit a50a34bc08a2af326911f746979bd3f98c8875fa Author: tzik <tzik@chromium.org> Date: Wed Aug 01 18:42:36 2018 Avoid implicit first reference retention around AwQuotaManager GetOriginsTask is ref counted, and its first reference is implicitly retained by base::Bind() in GetOriginsTask::Run(). Though this case is safe, an upcoming base::Bind change rejects this pattern of the first reference retention. After this CL, the caller of GetOriginsTask::Run() keeps the first reference to the GetOriginsTask instance to avoid the check failure. Bug: 866456 Change-Id: Ia011ccc3b815a626200403b22c63b8133c83fbb0 Reviewed-on: https://chromium-review.googlesource.com/1158664 Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Tao Bai <michaelbai@chromium.org> Cr-Commit-Position: refs/heads/master@{#579873} [modify] https://crrev.com/a50a34bc08a2af326911f746979bd3f98c8875fa/android_webview/browser/aw_quota_manager_bridge.cc
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d79a374ab5943eaf9bafb1e9d5dc9559f821431c commit d79a374ab5943eaf9bafb1e9d5dc9559f821431c Author: tzik <tzik@chromium.org> Date: Thu Aug 02 03:43:49 2018 Avoid touching VEAEncoder's ref count before it's fully constructed This contains a similar fix to the reverted previous attempt at http://crrev.com/556813c93fa177cd VEAEncoder is ref counted, and its first reference used to be retained in its constructor through base::Bind(). The reference is passed to the other sequence, and released when the callback instance is destroyed. If the PostTask there failed or the posted task ran soon, the VEAEncoder instance can be destroyed before another reference is retained on the original sequence. That is, `new VEAEncoder` may return a stale pointer. This CL adds a static cosntructor to VEAEncoder, and keeps the first reference before calling PostTask. Bug: 866456 Change-Id: Icbb430bebbfa5c800f3c0248b5afd10f2c2de271 Reviewed-on: https://chromium-review.googlesource.com/1158316 Reviewed-by: Emircan Uysaler <emircan@chromium.org> Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Cr-Commit-Position: refs/heads/master@{#580050} [modify] https://crrev.com/d79a374ab5943eaf9bafb1e9d5dc9559f821431c/content/renderer/media_recorder/vea_encoder.cc [modify] https://crrev.com/d79a374ab5943eaf9bafb1e9d5dc9559f821431c/content/renderer/media_recorder/vea_encoder.h [modify] https://crrev.com/d79a374ab5943eaf9bafb1e9d5dc9559f821431c/content/renderer/media_recorder/video_track_recorder.cc
,
Aug 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/efea4f5d9e773cbe917ca28ea2e30d9bdffc80a9 commit efea4f5d9e773cbe917ca28ea2e30d9bdffc80a9 Author: tzik <tzik@chromium.org> Date: Thu Aug 02 15:20:46 2018 Ban ownerless RefCounted receiver on base::Bind After this CL, base::BindOnce and base::BindRepeating rejecting pointers to ref-counted objects that have zero ref count. So that we can avoid a pattern of racy case like below in most cases: If a ref-counted class |Foo| posted a task that contains |this| as a receiver like this: Foo::Foo() { base::PostTask(FROM_HERE, base::BindOnce(&Foo::Bar, this)); } base::BindOnce retains a reference, and PostTask may release the reference before the construction of Foo completes, that happen when PostTask fails or the task runs soon. As the result, `new Foo()` may return stale pointer. Bug: 866456 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:ios-simulator-full-configs;luci.chromium.try:linux_mojo;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I2c7e8efcbd74fda49d183dbb782f94b0d7cf6af3 Reviewed-on: https://chromium-review.googlesource.com/1144813 Commit-Queue: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#580182} [modify] https://crrev.com/efea4f5d9e773cbe917ca28ea2e30d9bdffc80a9/base/bind.h [modify] https://crrev.com/efea4f5d9e773cbe917ca28ea2e30d9bdffc80a9/base/bind_internal.h [modify] https://crrev.com/efea4f5d9e773cbe917ca28ea2e30d9bdffc80a9/base/bind_unittest.cc [modify] https://crrev.com/efea4f5d9e773cbe917ca28ea2e30d9bdffc80a9/base/callback_unittest.cc [modify] https://crrev.com/efea4f5d9e773cbe917ca28ea2e30d9bdffc80a9/base/memory/ref_counted.cc [modify] https://crrev.com/efea4f5d9e773cbe917ca28ea2e30d9bdffc80a9/base/memory/ref_counted.h
,
Aug 3
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a4d8c399d911da54d3faa23ca55c67897f094cfc commit a4d8c399d911da54d3faa23ca55c67897f094cfc Author: Boris Yusupov <boriay@yandex-team.ru> Date: Thu Sep 06 08:21:57 2018 Avoid Bind() without reference to ref-counted Bug: 866456 R=tzik@chromium.org Change-Id: I54b1192a65e9646b5177ce5fcfd15e5fdb735058 Reviewed-on: https://chromium-review.googlesource.com/1206335 Reviewed-by: Taiju Tsuiki <tzik@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Reviewed-by: Jinho Bang <jinho.bang@samsung.com> Commit-Queue: Boris Yusupov <boriay@yandex-team.ru> Cr-Commit-Position: refs/heads/master@{#589111} [modify] https://crrev.com/a4d8c399d911da54d3faa23ca55c67897f094cfc/content/browser/payments/payment_app_installer.cc [modify] https://crrev.com/a4d8c399d911da54d3faa23ca55c67897f094cfc/content/shell/browser/shell_content_browser_client.cc [modify] https://crrev.com/a4d8c399d911da54d3faa23ca55c67897f094cfc/content/shell/browser/shell_login_dialog.cc [modify] https://crrev.com/a4d8c399d911da54d3faa23ca55c67897f094cfc/content/shell/browser/shell_login_dialog.h |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Jul 24