Improve GetWeakPtr() to better protect against unsafe usage patterns.

- Add a comment to clarify the [lack of] thread-safety.
- Remove the hack to allow thread hand-off if there are zero outstanding WeakPtrs.

[Wez, 2016-01-19 17:51:18]

wez@chromium.org changed reviewers:
+ dcheng@chromium.org

[Wez, 2016-01-19 17:51:19]

PTAL at this wording...

[Wez, 2016-02-06 03:07:37]

ping

[Wez, 2016-02-19 20:41:46]

pingy

[dcheng, 2016-02-19 20:45:00]

https://codereview.chromium.org/1602443003/diff/1/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/1602443003/diff/1/base/memory/weak_ptr.h#newc...
base/memory/weak_ptr.h:275: // Returns a new WeakPtr. This is not thread-safe -
in general callers should
"in general" is only true for code that wants a weak pointer to (presumably)
bind on multiple threads, right? This recommendation feels a bit too strong, and
it doesn't seem to be consistent with how it's typically used today.

[Wez, 2016-03-01 23:20:28]

https://codereview.chromium.org/1602443003/diff/1/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

https://codereview.chromium.org/1602443003/diff/1/base/memory/weak_ptr.h#newc...
base/memory/weak_ptr.h:275: // Returns a new WeakPtr. This is not thread-safe -
in general callers should
On 2016/02/19 at 20:45:00, dcheng wrote:
> "in general" is only true for code that wants a weak pointer to (presumably)
bind on multiple threads, right? This recommendation feels a bit too strong, and
it doesn't seem to be consistent with how it's typically used today.

I've reworded the comment to be more explicit about concurrency, but without
making it even more verbose it is non-trivial to see why it would _ever_ be safe
to call GetWeakPtr() directly on the factory from another thread, since that
means the caller is making sure the WeakPtrFactory is still live at the time, in
which case it's questionable why they need the WeakPtr...

I'm thinking that replacing the wall-of-words under THREAD SAFETY with some
simple SAFE and UNSAFE threading examples, with a brief comment here, would be
better - WDYT?

[dcheng, 2016-06-22 12:35:23]

Sorry for being a slacker. I see you've updated this; let me know when you want
me to re-review =P

[Wez, 2016-11-09 00:55:31]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 00:56:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 01:37:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 01:37:33]

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...)

[Wez, 2016-11-20 02:51:59]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-20 02:52:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-20 03:24:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-20 03:24:49]

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...)

[Wez, 2016-11-21 01:38:54]

Description was changed from

==========
Add a comment to GetWeakPtr to call out lack of thread-safety.
==========

to

==========
- Add a comment to clarify the [lack of] thread-safety.
- Remove the hack to allow thread hand-off if there are zero outstanding
WeakPtrs.
==========

[Wez, 2016-11-21 01:40:09]

danakj: Could you take a look at the cc::ElementAnimations test failures? It
seems that the PlayersList is being iterated from different threads - is that
expected?

[danakj, 2016-11-21 21:29:04]

On Sun, Nov 20, 2016 at 5:40 PM, <wez@chromium.org> wrote:

> danakj: Could you take a look at the cc::ElementAnimations test failures?
> It
> seems that the PlayersList is being iterated from different threads - is
> that
> expected?
>

Spoke offline, it appears to be possible thru RenderWidgetCompositor APIs
for blink maybe? And I suggest talking to loyso and ajuma if it's a problem.


>
> https://codereview.chromium.org/1602443003/
>

-- 
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.

[Wez, 2016-11-21 23:40:39]

danakj@, loyso@, ajuma@: I added some instrumentation to ThreadChecker and
gathered this back-trace for the ObserverList iteration that is happening off
the Compositor thread.

Presumably there's a missing Main->Compositor thread hop somewhere above the
RWC::setRootLayer() call?

#1 0x7f0e3d9b38ae
_ZN4base10MakeUniqueINS_5debug10StackTraceEJEEENS_8internal16MakeUniqueResultIT_E6ScalarEDpOT0_
#2 0x7f0e3d9b3595 base::ThreadCheckerImpl::EnsureAssigned()
#3 0x7f0e3d9b362d base::ThreadCheckerImpl::CalledOnValidThread()
#4 0x7f0e3d90ad15 base::SequenceCheckerImpl::CalledOnValidSequence()
#5 0x7f0e3d85b77d base::internal::WeakReference::Flag::IsValid()
#6 0x7f0e3d85ba6e base::internal::WeakReference::is_valid()
#7 0x7f0e3405d56f base::WeakPtr<>::get()
#8 0x7f0e3405d225 base::WeakPtr<>::operator bool()
#9 0x7f0e3405d179 base::ObserverListBase<>::Iter<>::EnsureValidIndex()
#10 0x7f0e3405cfb6 base::ObserverListBase<>::Iter<>::Iter()
#11 0x7f0e34058626 base::ObserverListBase<>::begin()
#12 0x7f0e34053baa cc::ElementAnimations::UpdateActivation()
#13 0x7f0e34054127 cc::ElementAnimations::ElementRegistered()
#14 0x7f0e3402fc85 cc::AnimationHost::RegisterElement()
#15 0x7f0e34324c8d cc::LayerTree::RegisterLayer()
#16 0x7f0e340ca09f cc::Layer::SetLayerTreeHost()
#17 0x7f0e340fe3d7 cc::PictureLayer::SetLayerTreeHost()
#18 0x7f0e340ca1bc cc::Layer::SetLayerTreeHost()
#19 0x7f0e340fe3d7 cc::PictureLayer::SetLayerTreeHost()
#20 0x7f0e340ca1bc cc::Layer::SetLayerTreeHost()
#21 0x7f0e340fe3d7 cc::PictureLayer::SetLayerTreeHost()
#22 0x7f0e340ca1bc cc::Layer::SetLayerTreeHost()
#23 0x7f0e340fe3d7 cc::PictureLayer::SetLayerTreeHost()
#24 0x7f0e340ca1bc cc::Layer::SetLayerTreeHost()
#25 0x7f0e340fe3d7 cc::PictureLayer::SetLayerTreeHost()
#26 0x7f0e340ca1bc cc::Layer::SetLayerTreeHost()
#27 0x7f0e340fe3d7 cc::PictureLayer::SetLayerTreeHost()
#28 0x7f0e340ca1bc cc::Layer::SetLayerTreeHost()
#29 0x7f0e340fe3d7 cc::PictureLayer::SetLayerTreeHost()
#30 0x7f0e340ca1bc cc::Layer::SetLayerTreeHost()
#31 0x7f0e340fe3d7 cc::PictureLayer::SetLayerTreeHost()
#32 0x7f0e3432428b cc::LayerTree::SetRootLayer()
#33 0x7f0e380ab652 content::RenderWidgetCompositor::setRootLayer()
#34 0x7f0e2dadf083 blink::WebViewImpl::setRootGraphicsLayer()
#35 0x7f0e2dacb48c blink::WebViewFrameWidget::setRootGraphicsLayer()
#36 0x7f0e2d969cd4 blink::ChromeClientImpl::attachRootGraphicsLayer()
#37 0x7f0e24682818 blink::PaintLayerCompositor::attachRootLayer()
#38 0x7f0e2467f3fc blink::PaintLayerCompositor::ensureRootLayer()
#39 0x7f0e2467ef78 blink::PaintLayerCompositor::setCompositingModeEnabled()
#40 0x7f0e2467f77c blink::PaintLayerCompositor::enableCompositingModeIfNeeded()
#41 0x7f0e24680a7d blink::PaintLayerCompositor::didLayout()
#42 0x7f0e23fe33f7 blink::FrameView::layout()
#43 0x7f0e23fe9a3c blink::FrameView::scrollbarExistenceDidChange()
#44 0x7f0e23ff28cb blink::FrameView::adjustScrollbarExistence()
#45 0x7f0e23fe2178 blink::FrameView::updateScrollbars()
#46 0x7f0e23fe1f45 blink::FrameView::setContentsSize()
#47 0x7f0e23fe23b9 blink::FrameView::adjustViewSize()
#48 0x7f0e23fe23e9 blink::FrameView::adjustViewSizeAndLayout()
#49 0x7f0e23fe3170 blink::FrameView::layout()
#50 0x7f0e23fef557
blink::FrameView::updateStyleAndLayoutIfNeededRecursiveInternal()
#51 0x7f0e23fed83e blink::FrameView::updateStyleAndLayoutIfNeededRecursive()
#52 0x7f0e23fec953 blink::FrameView::updateLifecyclePhasesInternal()
#53 0x7f0e23fec632 blink::FrameView::updateAllLifecyclePhases()
#54 0x7f0e247ec49b blink::PageAnimator::updateAllLifecyclePhases()
#55 0x7f0e2d9cd825 blink::PageWidgetDelegate::updateAllLifecyclePhases()
#56 0x7f0e2dad4814 blink::WebViewImpl::updateAllLifecyclePhases()
#57 0x7f0e2dad6cc2 blink::WebViewImpl::resizeViewWhileAnchored()
#58 0x7f0e2dad70ac blink::WebViewImpl::resizeWithBrowserControls()
#59 0x7f0e3823ef2e content::RenderViewImpl::ResizeWebWidget()
#60 0x7f0e3825bdbb content::RenderWidget::Resize()
#61 0x7f0e38259102 content::RenderWidget::OnResize()

[loyso (OOO), 2016-11-22 00:41:06]

Is it a crash? Could you give more context?

Overall, it looks like a valid main-thread back-trace. No compositor thread
involved at all. FTR, we use identical cc Animation classes on both main and
compositor threads (and different objects, of course).

[Wez, 2016-11-22 01:18:34]

On 2016/11/22 00:41:06, loyso wrote:
> Is it a crash? Could you give more context?
> 
> Overall, it looks like a valid main-thread back-trace. No compositor thread
> involved at all. FTR, we use identical cc Animation classes on both main and
> compositor threads (and different objects, of course).

Ah, sorry, missing context. ;)

This is the back-trace of an ElementAnimations player_list_ iteration triggered
from the Main thread, where the same player_list_ is later iterated on the
Compositor thread.  This crashes with my patch because I'm specifically
hardening WeakPtr (which player_list_ uses internally via ObserverList::Iter)
not to allow re-use across threads without an explicit InvalidateWeakPtrs().

[loyso (OOO), 2016-11-22 03:16:36]

On 2016/11/22 01:18:34, Wez wrote:
> This is the back-trace of an ElementAnimations player_list_ iteration
triggered
> from the Main thread, where the same player_list_ is later iterated on the
> Compositor thread.  This crashes with my patch because I'm specifically
> hardening WeakPtr (which player_list_ uses internally via ObserverList::Iter)
> not to allow re-use across threads without an explicit InvalidateWeakPtrs().

A helper change: https://codereview.chromium.org/2522543005/

[loyso (OOO), 2016-11-22 04:00:28]

Related feature: https://bugs.chromium.org/p/chromium/issues/detail?id=667579

[Wez, 2016-11-24 00:42:56]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 00:43:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 01:22:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 01:22:45]

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...)

[Wez, 2016-11-30 21:07:04]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 21:07:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 22:06:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 22:06:38]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Wez, 2016-12-02 01:07:28]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 01:07:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 01:48:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 01:48:42]

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...)

[Wez, 2017-01-11 22:51:07]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 22:52:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 23:52:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 23:52:12]

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...)

[Wez, 2017-01-13 20:08:23]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 20:09:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 21:30:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 21:30:22]

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...)

[Wez, 2017-02-24 01:51:14]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 01:51:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 03:04:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 03:04:28]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Wez, 2017-03-13 01:36:19]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 01:36:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 02:29:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 02:29:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Wez, 2017-03-28 06:05:17]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 06:05:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 07:06:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 07:06:34]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Wez, 2017-04-21 01:04:41]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 01:04:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 02:19:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 02:19:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wez, 2017-05-30 21:40:36]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 21:41:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 23:01:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 23:01:49]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Wez, 2017-07-17 19:35:13]

Moved to https://chromium-review.googlesource.com/575307