New issue
Advanced search Search tips

Issue 645774 link

Starred by 1 user
Status: Fixed
Owner:
dcheng@chromium.org
Closed: Dec 2017
Cc:
w...@chromium.org
tzik@chromium.org
esprehn@chromium.org
hirosh...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

base::WeakPtr and WTF::WeakPtr have different threading restrictions

Project Member Reported by dcheng@chromium.org, Sep 10 2016 
From https://bugs.chromium.org/p/chromium/issues/detail?id=643777#c3:

WTF::WeakPtrFactory:
The following operations must be executed on the same thread:
- WeakPtrFactory's invalidation (WeakPtrFactory::revokeAll())
- WeakPtr's dereference (WeakReference::get())
* WeakPtrFactory's construction
The following operations can be executed on any thread:
- WeakPtr's destruction
- WeakPtr's copy
* WeakPtrFactory's destruction
* WeakPtr's construction (WeakPtrFactory::createWeakPtr())

bindTo() is used for thread-related hacks.

base::WeakPtrFactory:
The following operations must be executed on the same sequence:
- WeakPtrFactory's invalidation (except when no WeakPtr exists)
- WeakPtr's dereference
* WeakPtrFactory's destruction (except when no WeakPtr exists)
* WeakPtr's construction (WeakPtrFactory::GetWeakPtr(); at least GetWeakPtr() must not be called simultaneously from two threads)
The following operations can be executed on any thread:
- WeakPtr's destruction
- WeakPtr's copy
* WeakPtrFactory's construction


1) Destroying a WeakPtrFactory invalidates weak ptrs, so how come it's OK to destroy WeakPtrFactory on any thread for the WTF version? That seems inherently dangerous.

2) GetWeakPtr() doesn't /have/ to be executed same sequence, but it is unsafe to have multiple threads calling it at the same time. Instead, MainThreadTaskRunner should store a WeakPtr as well and pass a copy of that when posting tasks.

I'd like to reland with that Blink fix, and land any changes to base::WeakPtrFactory afterwards.

Comment 1 by w...@chromium.org, Sep 10 2016 

Re (1): Are you sure WTF::WeakPtrFactory is safe to destroy on any thread? It calls WeakReference::clear(), which DCHECKs against the bound thread.

Re (2): Making createWeakPtr/GetWeakPtr safe to call concurrently from different threads introduces scope for race-conditions between WeakPtr construction and invalidation.

To balance ease-of-setup against safety at run-time, I'd suggest that:
1. GetWeakPtr() will not bind to the calling thread.
2. GetWeakPtr() will check against the bound thread if any.
3. Deref & invalidate will bind on first use, and check thereafter.

#1 is the current behaviour, and lets you create an object, take a WeakPtr to it, then "hand it off" to be called & destroyed on some other thread.

#2 will provide some protection against concurrent WeakPtr creation & invalidation; I expect a lot of call-sites will need updating to store WeakPtrs rather than using GetWeakPtr() directly at Bind()-time.

FWIW I'd also like to get rid of the "implicit un-bind" behaviour of WeakPtrFactory when the last WeakPtr to it is dropped, in favour of requiring explicit invalidation to achieve that.  I have a patch with that change drafted but it trips up in base::ObserverListBase, which relies on implicit un-bind to allow deletion on any thread so long as no Iterator is active.

Comment 2 by tzik@chromium.org, Sep 12 2016 

I also have a draft CL to remove the implicit un-bind: http://crrev.com/2325503003/
Looks like there are not so many caller that depends on that, at least users caught by tests.

Comment 3 by dcheng@chromium.org, Sep 12 2016 

After thinking about it, I agree that we don't want to allow GetWeakPtr() creation on any thread. In fact, my current CL to merge the two already does what wez@ proposed (grabbing the weak ptr ahead of time rather than creating it at bind time): https://codereview.chromium.org/2329243002/
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 14 2016 

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

commit 24430ece16aa10aa57644daeddafe25032fd359c
Author: dcheng <dcheng@chromium.org>
Date: Wed Sep 14 00:47:00 2016

Implement WTF::WeakPtr in terms of base::WeakPtr

BUG= 645774 

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

[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/core/dom/MainThreadTaskRunner.cpp
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/core/dom/MainThreadTaskRunner.h
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/platform/CrossThreadCopier.h
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/platform/fonts/FontFallbackList.cpp
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/wtf/DEPS
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/wtf/Forward.h
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/wtf/Functional.h
[modify] https://crrev.com/24430ece16aa10aa57644daeddafe25032fd359c/third_party/WebKit/Source/wtf/WeakPtr.h

Comment 5 by dcheng@chromium.org, Dec 26 2017

Status: Fixed (was: Started)
►

Sign in to add a comment