New issue
Advanced search Search tips

Issue 688072 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Detect and fix unsafe usages of non-threadsafe base::RefCounted

Project Member Reported by tzik@chromium.org, Feb 2 2017

Issue description

base::RefCounted is not thread-safe, that is, the user is responsible to protect AddRef() and Release() calls do not cause the data race on the refcount, which is error prone to achieve manually.
IMO, we should have an assertion to detect common unsafe case, and probably we need an opt-out option for that.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 16 2017

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

commit 704e5153f057ce179dc44ff6b4f6a0d565d886e4
Author: tzik <tzik@chromium.org>
Date: Thu Feb 16 09:42:02 2017

Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted

disk_cache::TraceObject has non-thread-safe ref count. Its AddRef() is
called on a background thread in disk_cache::BackendImpl::SyncInit(),
and Release() is called on the IO thread on disk_cache::Backend destruction.
So, if a Backend instance is initialized while another Backend instance is
being destroyed, these ref count manipulations cause data race.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2672983004
Cr-Commit-Position: refs/heads/master@{#450917}

[modify] https://crrev.com/704e5153f057ce179dc44ff6b4f6a0d565d886e4/net/disk_cache/blockfile/trace.h

Comment 2 by tzik@chromium.org, Feb 16 2017

Just to record:

I'm adding a DCHECK to trap the unsafe RefCount manipulation:
https://codereview.chromium.org/2666423002/

And found and fixed a bunch of data race:

Clear BlobReader in SW browser_tests on the right thread
https://codereview.chromium.org/2681073003/

Make FakeBookmarkDatabase RefCountedThreadSafe
https://codereview.chromium.org/2680353002/

Make nested classes of DevToolsDeviceDiscovery RefCountedThreadSafe
https://codereview.chromium.org/2686803002/

Make MessageLoopRunner be RefCountedThreadSafe
https://codereview.chromium.org/2686873003/

Avoid AddRef'ing non-thread-safe ref count on PageCaptureSaveAsMHTML
https://codereview.chromium.org/2684843004/

Protect RefCount manipulation of InterfaceEndpoint with MultiplexRouter lock
https://codereview.chromium.org/2684213002/

Make cast_channel::Logger RefCountedThreadSafe
https://codereview.chromium.org/2689483003/

Make DisplayLinkMac RefCountedThreadSafe
https://codereview.chromium.org/2684753004/

Release gpu::ContextGroup in gpu::InProcessCommandBuffer on GPU thread
https://codereview.chromium.org/2692063004/

Make disk_cache::TraceObject RefCountedThreadSafe instead of RefCounted
https://codereview.chromium.org/2672983004/


And, there are 2 pendings:

Make FontListImpl RefCountedThreadSafe
https://codereview.chromium.org/2695393003/

Fix a data race on the ref count of AddToHomescreenDataFetcher
https://codereview.chromium.org/2691943010/
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 17 2017

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

commit 99f46d6b1a2e559bf0f4f60fe59c2c0690e94ba8
Author: tzik <tzik@chromium.org>
Date: Fri Feb 17 06:16:36 2017

Fix a data race on the ref count of AddToHomescreenDataFetcher

AddToHomescreenDataFetcher has a non-thread-safe ref count. It posts
itself to background threads, and posts itself back to the original
thread. In the post-back phase of the previous code, the ref count
is manipulated without any protection, and that causes a data race.

This CL replaces the PostTask round trip with PostTaskAndReply, so
that we touch the ref count only on the original thread.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2691943010
Cr-Commit-Position: refs/heads/master@{#451241}

[modify] https://crrev.com/99f46d6b1a2e559bf0f4f60fe59c2c0690e94ba8/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.cc
[modify] https://crrev.com/99f46d6b1a2e559bf0f4f60fe59c2c0690e94ba8/chrome/browser/android/webapps/add_to_homescreen_data_fetcher.h

Project Member

Comment 4 by bugdroid1@chromium.org, Feb 20 2017

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

commit d8359a676ba9f7c6446f5aa2449466ff517efc9c
Author: tzik <tzik@chromium.org>
Date: Mon Feb 20 05:32:25 2017

Make MockImageCaptureClient RefCountedThreadSafe

MockImageCaptureClient has non-thread-safe ref count, and touched from
multiple threads. Though the access ordering to the ref count is well
controlled and there's no data race, the usage causes a false positive
DCHECK failure on an upcoming strict execution sequence check by:
https://codereview.chromium.org/2666423002/

This CL converts MockImageCaptureClient to RefCountedThreadSafe to
avoid the failure.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2699073002
Cr-Commit-Position: refs/heads/master@{#451562}

[modify] https://crrev.com/d8359a676ba9f7c6446f5aa2449466ff517efc9c/media/capture/video/video_capture_device_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Feb 21 2017

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

commit 3b2b2e23c656e16592fb6e2d4c75b5e4c353d061
Author: tzik <tzik@chromium.org>
Date: Tue Feb 21 05:01:25 2017

Avoid touching fonts in ResourceBundle while reloading locale

gfx::Font is not thread safe due to non-thread-safe ref counts in
gfx::PlatformFont and gfx::FontListImpl, and ui::ResouceBundle doesn't
protect them from data race. So, we should not touch them only on UI
thread to avoid the race on the ref count manipulation.

BUG= 688072 ,  468010 

Review-Url: https://codereview.chromium.org/2699403003
Cr-Commit-Position: refs/heads/master@{#451704}

[modify] https://crrev.com/3b2b2e23c656e16592fb6e2d4c75b5e4c353d061/chrome/browser/chromeos/base/locale_util.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 22 2017

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

commit 65452e0405106b58dd554d134679ace6acc26d00
Author: tzik <tzik@chromium.org>
Date: Wed Feb 22 22:53:16 2017

Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.

BUG= 688072 ,  468010 

Review-Url: https://codereview.chromium.org/2707753004
Cr-Commit-Position: refs/heads/master@{#452250}

[modify] https://crrev.com/65452e0405106b58dd554d134679ace6acc26d00/chrome/browser/icon_loader_chromeos.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 23 2017

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

commit bd171b13a4e04434cf2ee4d3b79ce28f0aeddd0f
Author: tzik <tzik@chromium.org>
Date: Thu Feb 23 04:24:04 2017

Avoid touching ResourceBundle fonts from non-UI thread

SharedResourcesDataSource touches shared fonts in ResourceBundle from
IO thread. However, gfx::Font is not thread safe due to non-thread-safe
ref counts in gfx::PlatformFont and gfx::FontListImpl. That causes a
data race on the ref count.
So, we should touch fonts in ResourceBundle only from UI thread.

BUG= 688072 ,  468010 

Review-Url: https://codereview.chromium.org/2701183002
Cr-Commit-Position: refs/heads/master@{#452390}

[modify] https://crrev.com/bd171b13a4e04434cf2ee4d3b79ce28f0aeddd0f/content/browser/webui/shared_resources_data_source.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 23 2017

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

commit 645ad78c4d429cda9591f9be1486b911d7076f59
Author: tzik <tzik@chromium.org>
Date: Thu Feb 23 05:32:51 2017

Restrict cross-thread access to gfx::Image and gfx::Font in ResourceBundle

Since gfx::Image and gfx::Font are not thread safe, cached images and
fonts in ResourceBundle can be used only on UI thread, regardless of
the protection by ResourceBundle::images_and_fonts_lock_.

This CL removes the lock and adds DCHECK to limit the access to UI thread.

BUG= 688072 ,  468010 

Review-Url: https://codereview.chromium.org/2699323002
Cr-Commit-Position: refs/heads/master@{#452407}

[modify] https://crrev.com/645ad78c4d429cda9591f9be1486b911d7076f59/ui/base/resource/resource_bundle.cc
[modify] https://crrev.com/645ad78c4d429cda9591f9be1486b911d7076f59/ui/base/resource/resource_bundle.h
[modify] https://crrev.com/645ad78c4d429cda9591f9be1486b911d7076f59/ui/base/resource/resource_bundle_ios.mm
[modify] https://crrev.com/645ad78c4d429cda9591f9be1486b911d7076f59/ui/base/resource/resource_bundle_mac.mm

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 24 2017

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

commit 06203bafdd105bf358fd8f216fd77ae0053f4ec7
Author: tzik <tzik@chromium.org>
Date: Fri Feb 24 03:35:25 2017

Avoid returning a shallow copy of gfx::Image from gfx::ImageFamily::CreateExact

gfx::ImageFamily::CreateExact() conditionally returns a shallow copy of
an internal gfx::Image. In that case, the resulting gfx::Image shares
gfx::internal::ImageStorage with an gfx::Image held by the gfx::ImageFamily.
Where gfx::internal::ImageStorage has non-thread-safe ref count.

That implies, if the gfx::ImageFamily is created on UI thread and
ImageFamily::CreateExact() is called on another thread, the copy operation
of gfx::Image causes a data race on the ref count.
E.g. web_apps::internals::UpdatePlatformShortcuts() on Windows calls
gfx::ImageFamily::CreateExact() via IconUtil::CreateIconFileFromImageFamily()
on FILE thread.

BUG= 688072 ,  468010 

Review-Url: https://codereview.chromium.org/2707993005
Cr-Commit-Position: refs/heads/master@{#452740}

[modify] https://crrev.com/06203bafdd105bf358fd8f216fd77ae0053f4ec7/ui/gfx/image/image_family.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 27 2017

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

commit 393f08e143d2d7c6d84744221db39516474fb159
Author: tzik <tzik@chromium.org>
Date: Mon Feb 27 17:51:42 2017

Make PnaclTranslationCacheEntry RefCountedThreadSafe

PnaclTranslationCacheEntry has a non-thread-safe ref count, and usually
bound to IO thread. However, disk_cache::BackendIO may be destroyed on
CACHE thread, and may drop a reference on it. That causes a data race
on the unprotected ref count.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2722513002
Cr-Commit-Position: refs/heads/master@{#453248}

[modify] https://crrev.com/393f08e143d2d7c6d84744221db39516474fb159/components/nacl/browser/pnacl_translation_cache.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Feb 28 2017

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

commit 0e0765474bc7bf13cbdf778d06bbc3be5783f11e
Author: tzik <tzik@chromium.org>
Date: Tue Feb 28 05:03:32 2017

Make WebMediaConstraitsPrivate ThreadSafeRefCounted

WebMediaConstraitsPrivate had a non-thread-safe ref count, but accessed
from multiple threads. E.g. UserMediaClientImpl::SelectVideoDeviceSourceSettings()
makes a reference of WebMediaConstraitsPrivate via WebUserMediaRequest::videoConstraints(),
and passes it to a CategorizedWorkerPool, then the reference is dropped on the
worker thread without any protection.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2710423002
Cr-Commit-Position: refs/heads/master@{#453505}

[modify] https://crrev.com/0e0765474bc7bf13cbdf778d06bbc3be5783f11e/third_party/WebKit/Source/platform/exported/WebMediaConstraints.cpp

Project Member

Comment 12 by bugdroid1@chromium.org, Mar 2 2017

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

commit 5604ddf6347cb5023c40d7ecf4edfc2411fd6805
Author: tzik <tzik@chromium.org>
Date: Thu Mar 02 05:08:51 2017

Destroy web_app::ShortcutInfo on UI thread

web_app::ShortcutInfo used in cr/b/web_applications needs to be destroyed
on UI thread, because it has a reference to a ImageStorage via
gfx::ImageFamily. Since gfx::internal::ImageStorage has a non-thread-safe
ref count, created on UI thread, and shared by others on UI thread, it
needs on UI thread too on destruction.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2703283005
Cr-Commit-Position: refs/heads/master@{#454183}

[modify] https://crrev.com/5604ddf6347cb5023c40d7ecf4edfc2411fd6805/chrome/browser/web_applications/web_app.cc
[modify] https://crrev.com/5604ddf6347cb5023c40d7ecf4edfc2411fd6805/chrome/browser/web_applications/web_app.h
[modify] https://crrev.com/5604ddf6347cb5023c40d7ecf4edfc2411fd6805/chrome/browser/web_applications/web_app_chromeos.cc
[modify] https://crrev.com/5604ddf6347cb5023c40d7ecf4edfc2411fd6805/chrome/browser/web_applications/web_app_linux.cc
[modify] https://crrev.com/5604ddf6347cb5023c40d7ecf4edfc2411fd6805/chrome/browser/web_applications/web_app_mac.mm
[modify] https://crrev.com/5604ddf6347cb5023c40d7ecf4edfc2411fd6805/chrome/browser/web_applications/web_app_mac_unittest.mm
[modify] https://crrev.com/5604ddf6347cb5023c40d7ecf4edfc2411fd6805/chrome/browser/web_applications/web_app_win.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Mar 2 2017

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

commit 2b817313110d51783e803320cd2d102c51166742
Author: tzik <tzik@chromium.org>
Date: Thu Mar 02 08:29:17 2017

Destroy bound callback to BindToCurrentLoop on the target task runner

A callback object bound by BindToCurrentLoop runs on the target thread,
but the callback itself was not necessarily destroyed on the target
thread. That causes a data race when a non-thread-safe RefCounted objects
is bound to the callback. (E.g. a BindToCurrentLoop call for PPB_Buffer
in content::ContentDecryptorDelegate::DeliverFrame().)

This CL refactors BindToCurrentLoop and adds a clean up task to mitigate
the racy usage.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2712603002
Cr-Commit-Position: refs/heads/master@{#454212}

[modify] https://crrev.com/2b817313110d51783e803320cd2d102c51166742/media/base/bind_to_current_loop.h
[modify] https://crrev.com/2b817313110d51783e803320cd2d102c51166742/media/base/bind_to_current_loop_unittest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 30 2017

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

commit b580fa032bb7a9d48c295f87cf69361a39e449b7
Author: tzik <tzik@chromium.org>
Date: Thu Mar 30 08:15:41 2017

Make mojo::internal::MultiplexRouter::InterfaceEndPoint RefCountedThreadSafe

InterfaceEndPoint's ref count is a non-thread-safe, and is used behind a
lock. The usage of it is not racy, but an upcoming change to add an
assertion to RefCounted requires a suppression here.
This CL converts InterfaceEndPoint to RefCountedThreadSafe, which doesn't
need the suppression, regarding the atomic operations is lightweight
comparing to a lock that covers the object itself, and RefCountedThreadSafe
less error prone for cross thread usage.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2788543002
Cr-Commit-Position: refs/heads/master@{#460692}

[modify] https://crrev.com/b580fa032bb7a9d48c295f87cf69361a39e449b7/mojo/public/cpp/bindings/lib/multiplex_router.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Mar 30 2017

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

commit f9a212dec0fec22e21d7f66ad1aa9414ed1dc068
Author: tzik <tzik@chromium.org>
Date: Thu Mar 30 15:42:37 2017

Assert sequence validity on non-thread-safe RefCount manipulations

This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy
ref count manipulations. A ref counted object is considered to be bound
to a sequence if the count is more than one. After this CL, ref count
manipulations to a sequenced-bound one cause DCHECK failure.

This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out,
and applies the opt-out as needed.

BUG= 688072 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2666423002
Cr-Commit-Position: refs/heads/master@{#460773}

[modify] https://crrev.com/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068/base/at_exit.cc
[modify] https://crrev.com/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068/base/memory/ref_counted.cc
[modify] https://crrev.com/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068/base/memory/ref_counted.h
[modify] https://crrev.com/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068/cc/trees/proxy_impl.cc
[modify] https://crrev.com/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068/content/browser/gpu/gpu_data_manager_impl.cc
[modify] https://crrev.com/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068/gpu/command_buffer/service/mailbox_manager_sync.cc
[modify] https://crrev.com/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068/ppapi/shared_impl/proxy_lock.h
[modify] https://crrev.com/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068/ppapi/shared_impl/tracked_callback.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Mar 30 2017

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

commit 4d5b2df34c2023956579d96e89aaaacf04e510d1
Author: wjmaclean <wjmaclean@chromium.org>
Date: Thu Mar 30 19:58:02 2017

Revert of Assert sequence validity on non-thread-safe RefCount manipulations (2) (patchset #35 id:700001 of https://codereview.chromium.org/2666423002/ )

Reason for revert:
Speculative revert to see if this CL is the cause of the RenderWidgetHostViewChildFrameTest.ForwardsBeginFrameAcks failures on https://uberchromegw.corp.google.com/i/chromium.win/builders/Win%207%20Tests%20x64%20%281%29/builds/22400

Original issue's description:
> Assert sequence validity on non-thread-safe RefCount manipulations
>
> This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy
> ref count manipulations. A ref counted object is considered to be bound
> to a sequence if the count is more than one. After this CL, ref count
> manipulations to a sequenced-bound one cause DCHECK failure.
>
> This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out,
> and applies the opt-out as needed.
>
> BUG= 688072 
> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
>
> Review-Url: https://codereview.chromium.org/2666423002
> Cr-Commit-Position: refs/heads/master@{#460773}
> Committed: https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068

TBR=gab@chromium.org,raymes@chromium.org,zmo@chromium.org,yzshen@chromium.org,danakj@chromium.org,tzik@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 688072 

Review-Url: https://codereview.chromium.org/2783393002
Cr-Commit-Position: refs/heads/master@{#460859}

[modify] https://crrev.com/4d5b2df34c2023956579d96e89aaaacf04e510d1/base/at_exit.cc
[modify] https://crrev.com/4d5b2df34c2023956579d96e89aaaacf04e510d1/base/memory/ref_counted.cc
[modify] https://crrev.com/4d5b2df34c2023956579d96e89aaaacf04e510d1/base/memory/ref_counted.h
[modify] https://crrev.com/4d5b2df34c2023956579d96e89aaaacf04e510d1/cc/trees/proxy_impl.cc
[modify] https://crrev.com/4d5b2df34c2023956579d96e89aaaacf04e510d1/content/browser/gpu/gpu_data_manager_impl.cc
[modify] https://crrev.com/4d5b2df34c2023956579d96e89aaaacf04e510d1/gpu/command_buffer/service/mailbox_manager_sync.cc
[modify] https://crrev.com/4d5b2df34c2023956579d96e89aaaacf04e510d1/ppapi/shared_impl/proxy_lock.h
[modify] https://crrev.com/4d5b2df34c2023956579d96e89aaaacf04e510d1/ppapi/shared_impl/tracked_callback.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Mar 31 2017

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

commit 3f4231f979090ad23fd89622b3b36ed564650645
Author: tzik <tzik@chromium.org>
Date: Fri Mar 31 21:31:24 2017

Assert sequence validity on non-thread-safe RefCount manipulations

This CL adds DCHECKs to non-thread-safe base::RefCount to trace racy
ref count manipulations. A ref counted object is considered to be bound
to a sequence if the count is more than one. After this CL, ref count
manipulations to a sequenced-bound one cause DCHECK failure.

This CL also adds ScopedAllowCrossThreadRefCountAccess to opt out,
and applies the opt-out as needed.

BUG= 688072 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2666423002
Cr-Original-Commit-Position: refs/heads/master@{#460773}
Committed: https://chromium.googlesource.com/chromium/src/+/f9a212dec0fec22e21d7f66ad1aa9414ed1dc068
Review-Url: https://codereview.chromium.org/2666423002
Cr-Commit-Position: refs/heads/master@{#461235}

[modify] https://crrev.com/3f4231f979090ad23fd89622b3b36ed564650645/base/at_exit.cc
[modify] https://crrev.com/3f4231f979090ad23fd89622b3b36ed564650645/base/memory/ref_counted.cc
[modify] https://crrev.com/3f4231f979090ad23fd89622b3b36ed564650645/base/memory/ref_counted.h
[modify] https://crrev.com/3f4231f979090ad23fd89622b3b36ed564650645/cc/trees/proxy_impl.cc
[modify] https://crrev.com/3f4231f979090ad23fd89622b3b36ed564650645/content/browser/gpu/gpu_data_manager_impl.cc
[modify] https://crrev.com/3f4231f979090ad23fd89622b3b36ed564650645/gpu/command_buffer/service/mailbox_manager_sync.cc
[modify] https://crrev.com/3f4231f979090ad23fd89622b3b36ed564650645/ppapi/shared_impl/proxy_lock.h
[modify] https://crrev.com/3f4231f979090ad23fd89622b3b36ed564650645/ppapi/shared_impl/tracked_callback.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Apr 3 2017

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

commit 2e3a439966819d6ce1700f01e76cc671276c40a9
Author: tzik <tzik@chromium.org>
Date: Mon Apr 03 18:06:15 2017

Remove ScopedAllowCrossThreadRefCountAccess from GpuDataManagerImpl

ScopedAllowCrossThreadRefCountAccess here has suppressed an assertion
hit in a RefCount manipulation done by GpuControlListEntry. Since it's
no longer RefCounted class after http://crrev.com/2756793003, the
suppression is no longer needed.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2793803002
Cr-Commit-Position: refs/heads/master@{#461469}

[modify] https://crrev.com/2e3a439966819d6ce1700f01e76cc671276c40a9/content/browser/gpu/gpu_data_manager_impl.cc

Project Member

Comment 19 by bugdroid1@chromium.org, Apr 25 2017

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

commit 1920c68e77540a5a35e52fba0860f1b8a6262e57
Author: tzik <tzik@chromium.org>
Date: Tue Apr 25 15:42:21 2017

Convert NaClIPCAdapter::RewrittenMessage ownership management from shared to unique

NaClIPCAdapter::RewrittenMessage was a non-thread-safe ref count and is
accessed from multiple threads. Though it's used safely behind a lock,
it confuses an upcoming threading assertion in RefCounted.

This CL removes its ref count, and has NaClIPCAdapter handle it in
std::unique_ptr as C++11 STL container can hold non-copyable values and
the ref count is no longer needed.

BUG= 688072 

Review-Url: https://codereview.chromium.org/2781443004
Cr-Commit-Position: refs/heads/master@{#467000}

[modify] https://crrev.com/1920c68e77540a5a35e52fba0860f1b8a6262e57/components/nacl/loader/nacl_ipc_adapter.cc
[modify] https://crrev.com/1920c68e77540a5a35e52fba0860f1b8a6262e57/components/nacl/loader/nacl_ipc_adapter.h

Comment 20 by tzik@chromium.org, Jul 12 2017

Status: Fixed (was: Assigned)

Sign in to add a comment