base::Bind and friends

[Ken Rockot]

Does it seem reasonable to start allowing use of base::Bind in blink?

I took a cursory look at what dependencies this would bring with it and AFAICT there isn't much overlap or conflict with anything in Blink land. Logging is the only thing that stands out to me, and I don't know if that's really an issue (is it?).

This would mean allowing base::Bind, base::Callback, as well as probably at least scoped_ptr for argument binding. WeakPtr would be nice in some circumstances.

The immediate motivation is some exploratory work using Mojo in Blink to bypass the content layer and talk directly to browser components. Behold the many lines of deleted code.

An alternative solution would be to implement a reduced mojo::Bind (i.e., something which yields a mojo::Callback) without bringing in any //base deps.

[Daniel Cheng]

Blink already has WTF::bind. If we wanted to use base::Bind() in Blink, we should probably remove WTF::bind. At minimum, I think this should also involve:

- Merging base::WeakPtr and WTF::WeakPtr

- Figuring out scoped_ptr vs WTF::OwnPtr usage in Blink (I don't think we should use both of them)

- Figuring out the thread-safety story (someone else can probably comment more on this, but I recall there were some issues with binding WTF::Strings on cross-thread tasks)

base::Bind also has some features that WTF::bind does not:

- It tries to enforce lifetime semantics of |this| when binding a method.

- Binding a refcounted argument (that's not |this|) as a raw pointer is a compile-time error

Also, this is somewhat offtopic, but I looked at the Mojo in Blink patch, and it seems strange to me that //content depends on //third_party/WebKit but then parts of WebKit depend on //content. That doesn't seem like something we should do.

Daniel

[Jeremy Roman]

In the interest of caution, my preference would be to permit base::Bind in Blink only where it's needed by platform code interacting with Chromium code that accepts base::Callback arguments (i.e. places where today we'd create a Web* callback interface), and possibly in WTF where we could use it to adapt a base/ implementation onto a WTF interface to avoid code duplication.

This lets us begin disentangling the dependencies without immediately trying to rewrite all of Blink (especially core/) with Chromium idioms. From there we can look at adding deeper dependencies, if we think that's worthwhile.

[Ken Rockot]

On Tue, Sep 29, 2015 at 11:42 AM, Daniel Cheng <dch...@chromium.org> wrote:

Blink already has WTF::bind. If we wanted to use base::Bind() in Blink, we should probably remove WTF::bind. At minimum, I think this should also involve:

- Merging base::WeakPtr and WTF::WeakPtr

- Figuring out scoped_ptr vs WTF::OwnPtr usage in Blink (I don't think we should use both of them)

- Figuring out the thread-safety story (someone else can probably comment more on this, but I recall there were some issues with binding WTF::Strings on cross-thread tasks)

base::Bind also has some features that WTF::bind does not:

- It tries to enforce lifetime semantics of |this| when binding a method.

- Binding a refcounted argument (that's not |this|) as a raw pointer is a compile-time error

This all sounds reasonable. Getting rid of WTF::bind does not seem like a huge hurdle.

 

Also, this is somewhat offtopic, but I looked at the Mojo in Blink patch, and it seems strange to me that //content depends on //third_party/WebKit but then parts of WebKit depend on //content. That doesn't seem like something we should do.

The content dependency from within Blink isn't really there AFAICT, I think that's just an oversight. The mojom dependencies live in their own component targets which in turn have no content dependencies.

[Elliott Sprehn]

+1, I think allowing base::Bind at the boundary (platform/) makes sense if it simplifies our interaction with content. It also makes sense in WTF where we might wrap it or leverage it to implement something else. I don't think we should bring it to the rest of the codebase just yet.

I don't think we want scoped_refptr at this time.

- E

[Kentaro Hara]

+hiroshige

--

Kentaro Hara, Tokyo, Japan

[Kinuko Yasuda]

On Wed, Sep 30, 2015 at 4:12 AM, 'Elliott Sprehn' via blink-dev <blin...@chromium.org> wrote:

+1, I think allowing base::Bind at the boundary (platform/) makes sense if it simplifies our interaction with content. It also makes sense in WTF where we might wrap it or leverage it to implement something else. I don't think we should bring it to the rest of the codebase just yet.

I don't think we want scoped_refptr at this time.

- E

On Sep 29, 2015 11:55 AM, "Jeremy Roman" <jbr...@chromium.org> wrote:

In the interest of caution, my preference would be to permit base::Bind in Blink only where it's needed by platform code interacting with Chromium code that accepts base::Callback arguments (i.e. places where today we'd create a Web* callback interface), and possibly in WTF where we could use it to adapt a base/ implementation onto a WTF interface to avoid code duplication.

This lets us begin disentangling the dependencies without immediately trying to rewrite all of Blink (especially core/) with Chromium idioms. From there we can look at adding deeper dependencies, if we think that's worthwhile.

On Tue, Sep 29, 2015 at 2:42 PM, Daniel Cheng <dch...@chromium.org> wrote:

Blink already has WTF::bind. If we wanted to use base::Bind() in Blink, we should probably remove WTF::bind. At minimum, I think this should also involve:

- Merging base::WeakPtr and WTF::WeakPtr

- Figuring out scoped_ptr vs WTF::OwnPtr usage in Blink (I don't think we should use both of them)

- Figuring out the thread-safety story (someone else can probably comment more on this, but I recall there were some issues with binding WTF::Strings on cross-thread tasks)

A few more differences from thread-safety pov:

- WTF::bind deliberately returns OwnPtr while base::Bind returns copyable callback type

- (Partly due to the above fact) base::Bind can never work safely with WTF::String in cross-thread post tasking

- WTF::bind has a thread-safe version, i.e. WTF::threadSafeBind

Making (any) bind work safely with WTF::String or any type that could (directly or indirectly) contain WTF:String in cross-thread case is very tricky, while base::Bind tends to be used for cross-thread post tasking, which makes me worry a bit.

 

base::Bind also has some features that WTF::bind does not:

- It tries to enforce lifetime semantics of |this| when binding a method.

- Binding a refcounted argument (that's not |this|) as a raw pointer is a compile-time error

If we start to allow base::Bind in Blink code could we also add some restrictions like:

- Binding WTF::String to base::Bind should lead to a compile-time error

for the time being?

This disallows binding WTF::String even for same-thread post tasking, and wouldn't protect us from more subtle cases (e.g. KURL contains WTF::String, which means binding KURL for cross-thread post tasking is unsafe), but could save some easy crashy cases.  (We, i.e. worker team and hiroshige@, have been fixing a lot of such crashes caused by binding WebString for cross-thread post tasking at Blink/Chromium boundary)

I and hiroshige@ have been thinking about making any potentially racy Blink classes identifiable via clang-plugin to make cross-thread post tasking safer in blink (rough design doc), if we start to bind more Blink classes with base::Bind or in cross-thread environment we might want to revisit this.

[Hiroshige Hayashizaki]

base::Bind() and Blink's bind()/threadSafeBind() have thread-safety issues and I'm afraid mixing them makes the situations worse.

I'd like to make a better design for callbacks before merging.

Blink-side has more thread-unsafe objects/classes than Chromium-side (e.g. WTF::String vs. base::string16), and base::Bind() and WTF::bind()/threadSafeBind() have important differences related to ensuring thread safety (discussed with tzik@):

• Blink's bind() is move-only while Chromium's Bind() is copyable.
• Blink's uses of bind() are oneshot (a callback is called at most only once), while Chromium's Bind() is often called more than once.
• Blink has threadSafeBind() to distinguish cross-thread tasks from same-thread ones and to add more checks, restrictions and processing for thread-safety.

For thread-safety, cross-thread tasks should be oneshot and preferrably move-only, when bound with WTF::String or other thread-unsafe objects.

See [1] for discussion about how to bind WTF::String to base::Bind(), where making callbacks with WTF::String oneshot is discussed.

A bug caused by non-oneshot and non-move-only callbacks: [2]

Also, for cross-thread tasks, all classes containing WTF::String or WTF::RefCounted should not use base::Bind().

Most significant bugs are using base::Bind() with WebString ([3] and many other).

Sometimes base::Bind() is used with Blink's objects, but it requires very careful coding and resulted in very subtle bugs (e.g. [2] and [4]).

[1] https://groups.google.com/a/chromium.org/d/msg/blink-dev/ynvFfu9rQrc/Co2_1vLn4SIJ

[2] https://codereview.chromium.org/807423002/

[3] https://codereview.chromium.org/1257393005/

[4] https://code.google.com/p/chromium/issues/detail?id=478149 (restricted)

I and hiroshige@ have been thinking about making any potentially racy Blink classes identifiable via clang-plugin to make cross-thread post tasking safer in blink (rough design doc), if we start to bind more Blink classes with base::Bind or in cross-thread environment we might want to revisit this.

This effort is related to prohibiting unsafe Blink objects + base::Bind(), and is part of efforts to make task posting and thread-safety more automatically verifiable.

If we start to allow base::Bind in Blink code could we also add some restrictions like:
- Binding WTF::String to base::Bind should lead to a compile-time error
for the time being?
This disallows binding WTF::String even for same-thread post tasking, and wouldn't protect us from more subtle cases (e.g. KURL contains WTF::String, which means binding KURL for cross-thread post tasking is unsafe), but could save some easy crashy cases.  (We, i.e. worker team and hiroshige@, have been fixing a lot of such crashes caused by binding WebString for cross-thread post tasking at Blink/Chromium boundary)

base::Bind() doesn't distinguish cross-thread tasks, and attempts to check this kind of restriction might result in many false positives.

For example, there are already ~2 calls of base::Bind() with WebString for same-thread tasks in Chromium.

[Dana Jansens]

On Tue, Sep 29, 2015 at 6:14 PM, Kinuko Yasuda <kin...@chromium.org> wrote:

On Wed, Sep 30, 2015 at 4:12 AM, 'Elliott Sprehn' via blink-dev <blin...@chromium.org> wrote:

+1, I think allowing base::Bind at the boundary (platform/) makes sense if it simplifies our interaction with content. It also makes sense in WTF where we might wrap it or leverage it to implement something else. I don't think we should bring it to the rest of the codebase just yet.

I don't think we want scoped_refptr at this time.

- E

On Sep 29, 2015 11:55 AM, "Jeremy Roman" <jbr...@chromium.org> wrote:

In the interest of caution, my preference would be to permit base::Bind in Blink only where it's needed by platform code interacting with Chromium code that accepts base::Callback arguments (i.e. places where today we'd create a Web* callback interface), and possibly in WTF where we could use it to adapt a base/ implementation onto a WTF interface to avoid code duplication.

This lets us begin disentangling the dependencies without immediately trying to rewrite all of Blink (especially core/) with Chromium idioms. From there we can look at adding deeper dependencies, if we think that's worthwhile.

On Tue, Sep 29, 2015 at 2:42 PM, Daniel Cheng <dch...@chromium.org> wrote:

Blink already has WTF::bind. If we wanted to use base::Bind() in Blink, we should probably remove WTF::bind. At minimum, I think this should also involve:

- Merging base::WeakPtr and WTF::WeakPtr

- Figuring out scoped_ptr vs WTF::OwnPtr usage in Blink (I don't think we should use both of them)

- Figuring out the thread-safety story (someone else can probably comment more on this, but I recall there were some issues with binding WTF::Strings on cross-thread tasks)

A few more differences from thread-safety pov:

- WTF::bind deliberately returns OwnPtr while base::Bind returns copyable callback type

- (Partly due to the above fact) base::Bind can never work safely with WTF::String in cross-thread post tasking

As someone who has no idea how WTF::String works, can you explain the problems involved here?

Are you worried about the Callback being called multiple times on different threads? How does this differ from having a raw WTF::String and using it on different threads, or a raw WTF::String on one thread and an owned WTF callback on another thread? Does bindings a WTF::String with WTF::bind move/destroy the original WTF::String?

[Daniel Cheng]

The problems with WTF::String stem from the fact that it is internally refcounted, but the refcounting is not thread-safe. https://docs.google.com/document/d/17qforp6RQ6GIy3Jr5qA04krKTyfZe3LdPQVTjYHyFpA/edit has the details (sorry, I think this is google.com only).

Daniel

[Dana Jansens]

On Mon, Oct 12, 2015 at 4:42 PM, Daniel Cheng <dch...@chromium.org> wrote:

The problems with WTF::String stem from the fact that it is internally refcounted, but the refcounting is not thread-safe. https://docs.google.com/document/d/17qforp6RQ6GIy3Jr5qA04krKTyfZe3LdPQVTjYHyFpA/edit has the details (sorry, I think this is google.com only).

Oh :( This is a huge problem in chromium too FWIW. Any time you stick a RefCountedThreadSafe object into a Closure and PostTask it, you have no idea where it will be deleted. Often people want to do some thread-specific thing in the destructor and they just.. can't. So the (unfortunately) common pattern is to PostTask to the right thread from the destructor, but by then it's too late to handle the case that PostTask can fail.. so you can just leak/fail to destruct.

Hypothetically.. would making Callback move-only solve all of the problems here? I'm not sure how intractable that would be but it could help a lot in Chrome probably.

There's some mention of WTF callbacks being one-shot which differs from Base. But what impact does this have on WTF::String? We could perhaps provide a wrapper around a base::Callback like WTF::OneShotCallback that crashes if you try call it twice or something (we have a similar thing in cc::SingleReleaseCallback), or more specific wrappers in the places in blink that really care about this?

[Kinuko Yasuda]

On Tue, Oct 13, 2015 at 8:52 AM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 12, 2015 at 4:42 PM, Daniel Cheng <dch...@chromium.org> wrote:

The problems with WTF::String stem from the fact that it is internally refcounted, but the refcounting is not thread-safe. https://docs.google.com/document/d/17qforp6RQ6GIy3Jr5qA04krKTyfZe3LdPQVTjYHyFpA/edit has the details (sorry, I think this is google.com only).

Oh :( This is a huge problem in chromium too FWIW. Any time you stick a RefCountedThreadSafe object into a Closure and PostTask it, you have no idea where it will be deleted. Often people want to do some thread-specific thing in the destructor and they just.. can't. So the (unfortunately) common pattern is to PostTask to the right thread from the destructor, but by then it's too late to handle the case that PostTask can fail.. so you can just leak/fail to destruct.

WTF::String is NOT thread-safe ref-counted, yet it's used in cross-thread post tasking (partly because we have no other thread-safe string representation in Blink other than std::string), so the situation is probably worse.

Hypothetically.. would making Callback move-only solve all of the problems here? I'm not sure how intractable that would be but it could help a lot in Chrome probably.

There's some mention of WTF callbacks being one-shot which differs from Base. But what impact does this have on WTF::String? We could perhaps provide a wrapper around a base::Callback like WTF::OneShotCallback that crashes if you try call it twice or something (we have a similar thing in cc::SingleReleaseCallback), or more specific wrappers in the places in blink that really care about this?

This is all very subtle but let me try to explain.  (hiroshige@ could probably give more details and concrete examples)

Having a move-only closure helps to avoid leaking a reference on the calling thread after postTask().  Or having a move-only wrapper for WTF::String (and all such racy classes) could be also useful.  But this only solves one aspect, in order to safely pass a WTF::String to another thread one also needs to create an isolated copy of WTF::String which doesn't share any ref-counting with existing String objects.  (I think this answers to one of your questions)

And one more thing to note, the same thing needs to apply to all objects that contain WTF::String in any of their members.  So if one wants to bind KURL for running a method on another thread, we first need to create an isolated copy of KURL without leaking any reference after postTask because KURL contains WTF::String.  Ditto if one wants to bind an object that contains KURL.

So the biggest problem is, even if we have move-only callback, the caller needs to choose the right one and does the right thing, and it's almost impractical to assume that everyone understands all these well and does the right thing (TM).

So what hiroshige@ did in Blink was to make WTF::bind() always return move-only (so that no one can leave an unexpected reference after postTask), and to introduce a new WTF::bind() for cross-thread version which does this all (so that all cross-thread post tasking could be automatically taken care of, while keeping the same-thread post tasking to work as is with full benefit of ref-counting for performance).

So getting back to the original question: if we could make Callback move-only in Chrome, yes I think it'd be helpful, but it doesn't solve all issues.  If we could also make cross-thread(able) post tasking and same-thread post tasking distinguishable in Chrome, (say, by having ThreadSafeCallback and start asserting non-thread-safe callback usage on multi-threaded code) we could start to be comfortable allowing base::Bind in Blink, or allowing WTF::String used in other Chrome code.

Sorry for lengthy response.  Maybe we could have a quick VC to chat about this too?

[Dana Jansens]

On Mon, Oct 12, 2015 at 7:18 PM, Kinuko Yasuda <kin...@chromium.org> wrote:

On Tue, Oct 13, 2015 at 8:52 AM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 12, 2015 at 4:42 PM, Daniel Cheng <dch...@chromium.org> wrote:

The problems with WTF::String stem from the fact that it is internally refcounted, but the refcounting is not thread-safe. https://docs.google.com/document/d/17qforp6RQ6GIy3Jr5qA04krKTyfZe3LdPQVTjYHyFpA/edit has the details (sorry, I think this is google.com only).

Oh :( This is a huge problem in chromium too FWIW. Any time you stick a RefCountedThreadSafe object into a Closure and PostTask it, you have no idea where it will be deleted. Often people want to do some thread-specific thing in the destructor and they just.. can't. So the (unfortunately) common pattern is to PostTask to the right thread from the destructor, but by then it's too late to handle the case that PostTask can fail.. so you can just leak/fail to destruct.

WTF::String is NOT thread-safe ref-counted, yet it's used in cross-thread post tasking (partly because we have no other thread-safe string representation in Blink other than std::string), so the situation is probably worse.

Hypothetically.. would making Callback move-only solve all of the problems here? I'm not sure how intractable that would be but it could help a lot in Chrome probably.

There's some mention of WTF callbacks being one-shot which differs from Base. But what impact does this have on WTF::String? We could perhaps provide a wrapper around a base::Callback like WTF::OneShotCallback that crashes if you try call it twice or something (we have a similar thing in cc::SingleReleaseCallback), or more specific wrappers in the places in blink that really care about this?

This is all very subtle but let me try to explain.  (hiroshige@ could probably give more details and concrete examples)

Having a move-only closure helps to avoid leaking a reference on the calling thread after postTask().  Or having a move-only wrapper for WTF::String (and all such racy classes) could be also useful.  But this only solves one aspect, in order to safely pass a WTF::String to another thread one also needs to create an isolated copy of WTF::String which doesn't share any ref-counting with existing String objects.  (I think this answers to one of your questions)

And one more thing to note, the same thing needs to apply to all objects that contain WTF::String in any of their members.  So if one wants to bind KURL for running a method on another thread, we first need to create an isolated copy of KURL without leaking any reference after postTask because KURL contains WTF::String.  Ditto if one wants to bind an object that contains KURL.

So the biggest problem is, even if we have move-only callback, the caller needs to choose the right one and does the right thing, and it's almost impractical to assume that everyone understands all these well and does the right thing (TM).

So what hiroshige@ did in Blink was to make WTF::bind() always return move-only (so that no one can leave an unexpected reference after postTask), and to introduce a new WTF::bind() for cross-thread version which does this all (so that all cross-thread post tasking could be automatically taken care of, while keeping the same-thread post tasking to work as is with full benefit of ref-counting for performance).

So getting back to the original question: if we could make Callback move-only in Chrome, yes I think it'd be helpful, but it doesn't solve all issues.  If we could also make cross-thread(able) post tasking and same-thread post tasking distinguishable in Chrome, (say, by having ThreadSafeCallback and start asserting non-thread-safe callback usage on multi-threaded code) we could start to be comfortable allowing base::Bind in Blink, or allowing WTF::String used in other Chrome code.

Sorry for lengthy response.  Maybe we could have a quick VC to chat about this too?

Thanks, this is a really great response and helps a lot. I'm going to read some more stuff and try to understand exactly what the WTF::bind for cross-thread asserts and how. Maybe we can put together a proposal for changing base::Callback and friends.

+base owners to put this on radars.

[Vladimir Levin]

On Thu, Oct 15, 2015 at 1:18 PM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 12, 2015 at 7:18 PM, Kinuko Yasuda <kin...@chromium.org> wrote:

On Tue, Oct 13, 2015 at 8:52 AM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 12, 2015 at 4:42 PM, Daniel Cheng <dch...@chromium.org> wrote:

The problems with WTF::String stem from the fact that it is internally refcounted, but the refcounting is not thread-safe. https://docs.google.com/document/d/17qforp6RQ6GIy3Jr5qA04krKTyfZe3LdPQVTjYHyFpA/edit has the details (sorry, I think this is google.com only).

Oh :( This is a huge problem in chromium too FWIW. Any time you stick a RefCountedThreadSafe object into a Closure and PostTask it, you have no idea where it will be deleted. Often people want to do some thread-specific thing in the destructor and they just.. can't. So the (unfortunately) common pattern is to PostTask to the right thread from the destructor, but by then it's too late to handle the case that PostTask can fail.. so you can just leak/fail to destruct.

WTF::String is NOT thread-safe ref-counted, yet it's used in cross-thread post tasking (partly because we have no other thread-safe string representation in Blink other than std::string), so the situation is probably worse.

Hypothetically.. would making Callback move-only solve all of the problems here? I'm not sure how intractable that would be but it could help a lot in Chrome probably.

There's some mention of WTF callbacks being one-shot which differs from Base. But what impact does this have on WTF::String? We could perhaps provide a wrapper around a base::Callback like WTF::OneShotCallback that crashes if you try call it twice or something (we have a similar thing in cc::SingleReleaseCallback), or more specific wrappers in the places in blink that really care about this?

This is all very subtle but let me try to explain.  (hiroshige@ could probably give more details and concrete examples)

Having a move-only closure helps to avoid leaking a reference on the calling thread after postTask().  Or having a move-only wrapper for WTF::String (and all such racy classes) could be also useful.  But this only solves one aspect, in order to safely pass a WTF::String to another thread one also needs to create an isolated copy of WTF::String which doesn't share any ref-counting with existing String objects.  (I think this answers to one of your questions)

And one more thing to note, the same thing needs to apply to all objects that contain WTF::String in any of their members.  So if one wants to bind KURL for running a method on another thread, we first need to create an isolated copy of KURL without leaking any reference after postTask because KURL contains WTF::String.  Ditto if one wants to bind an object that contains KURL.

So the biggest problem is, even if we have move-only callback, the caller needs to choose the right one and does the right thing, and it's almost impractical to assume that everyone understands all these well and does the right thing (TM).

So what hiroshige@ did in Blink was to make WTF::bind() always return move-only (so that no one can leave an unexpected reference after postTask), and to introduce a new WTF::bind() for cross-thread version which does this all (so that all cross-thread post tasking could be automatically taken care of, while keeping the same-thread post tasking to work as is with full benefit of ref-counting for performance).

As far as I can tell, the thread safe bind uses WTF::bind, but it just makes deep copies of objects that are known to be ref counted but not thread safe (such as KURL and String). This thread safe bind can still remain, but use base::Bind internally instead, right? Assuming of course that base::Bind has a version that returns a move only callback. So, if bind returns a move only callback of a different type like base::MoveOnlyCallback for lack of a better name, then PostTask would also have to deal with the fact that there's a move only callback as well as a copy-able one and possibly do asserts in the copy-able version to ensure that it's not going cross threads.

Additionally, I don't think there's any checks in place to enforce that people actually use threadSafeBind instead of regular WTF::bind with objects that are not thread safe ref counted. That is, at least I couldn't find any of these checks. So, even if you use move only callbacks, there's still no guarantee that both problems are solved. One needs to ensure that threadSafeBind is used instead WTF::bind

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Dana Jansens]

On Thu, Oct 15, 2015 at 2:50 PM, Vladimir Levin <vmp...@google.com> wrote:

On Thu, Oct 15, 2015 at 1:18 PM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 12, 2015 at 7:18 PM, Kinuko Yasuda <kin...@chromium.org> wrote:

On Tue, Oct 13, 2015 at 8:52 AM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 12, 2015 at 4:42 PM, Daniel Cheng <dch...@chromium.org> wrote:

The problems with WTF::String stem from the fact that it is internally refcounted, but the refcounting is not thread-safe. https://docs.google.com/document/d/17qforp6RQ6GIy3Jr5qA04krKTyfZe3LdPQVTjYHyFpA/edit has the details (sorry, I think this is google.com only).

Oh :( This is a huge problem in chromium too FWIW. Any time you stick a RefCountedThreadSafe object into a Closure and PostTask it, you have no idea where it will be deleted. Often people want to do some thread-specific thing in the destructor and they just.. can't. So the (unfortunately) common pattern is to PostTask to the right thread from the destructor, but by then it's too late to handle the case that PostTask can fail.. so you can just leak/fail to destruct.

WTF::String is NOT thread-safe ref-counted, yet it's used in cross-thread post tasking (partly because we have no other thread-safe string representation in Blink other than std::string), so the situation is probably worse.

Hypothetically.. would making Callback move-only solve all of the problems here? I'm not sure how intractable that would be but it could help a lot in Chrome probably.

There's some mention of WTF callbacks being one-shot which differs from Base. But what impact does this have on WTF::String? We could perhaps provide a wrapper around a base::Callback like WTF::OneShotCallback that crashes if you try call it twice or something (we have a similar thing in cc::SingleReleaseCallback), or more specific wrappers in the places in blink that really care about this?

This is all very subtle but let me try to explain.  (hiroshige@ could probably give more details and concrete examples)

Having a move-only closure helps to avoid leaking a reference on the calling thread after postTask().  Or having a move-only wrapper for WTF::String (and all such racy classes) could be also useful.  But this only solves one aspect, in order to safely pass a WTF::String to another thread one also needs to create an isolated copy of WTF::String which doesn't share any ref-counting with existing String objects.  (I think this answers to one of your questions)

And one more thing to note, the same thing needs to apply to all objects that contain WTF::String in any of their members.  So if one wants to bind KURL for running a method on another thread, we first need to create an isolated copy of KURL without leaking any reference after postTask because KURL contains WTF::String.  Ditto if one wants to bind an object that contains KURL.

So the biggest problem is, even if we have move-only callback, the caller needs to choose the right one and does the right thing, and it's almost impractical to assume that everyone understands all these well and does the right thing (TM).

So what hiroshige@ did in Blink was to make WTF::bind() always return move-only (so that no one can leave an unexpected reference after postTask), and to introduce a new WTF::bind() for cross-thread version which does this all (so that all cross-thread post tasking could be automatically taken care of, while keeping the same-thread post tasking to work as is with full benefit of ref-counting for performance).

As far as I can tell, the thread safe bind uses WTF::bind, but it just makes deep copies of objects that are known to be ref counted but not thread safe (such as KURL and String). This thread safe bind can still remain, but use base::Bind internally instead, right? Assuming of course that base::Bind has a version that returns a move only callback. So, if bind returns a move only callback of a different type like base::MoveOnlyCallback for lack of a better name, then PostTask would also have to deal with the fact that there's a move only callback as well as a copy-able one and possibly do asserts in the copy-able version to ensure that it's not going cross threads.

Additionally, I don't think there's any checks in place to enforce that people actually use threadSafeBind instead of regular WTF::bind with objects that are not thread safe ref counted. That is, at least I couldn't find any of these checks. So, even if you use move only callbacks, there's still no guarantee that both problems are solved. One needs to ensure that threadSafeBind is used instead WTF::bind

Was your hope to replace blink::threadSafeBind (in platform/) with uses of base::Bind? Or just to replace WTF::bind() (in wtf/) with base?

The former has type-specfic deep copies for every type that can be posted, which isn't super scalable and doesn't seem well suited to base, and is probably in the right place where it is (modulo completely different ideas).

The document lists two problems. AIUI, move-only Callback solves one, deep copies in blink::threadSafeBind solves the other. Either solution is not good enough without the other, right?  So we still want to make callers in blink use blink::threadSafeBind where it makes sense (for isolatedCopy()). Then if threadSafeBind returned a base::MoveOnlyCallback, we'd have both problems solved?

I can imagine our PostTask methods DCHECKing that non-MoveOnly callbacks are only posted to the same thread. It may or may not be possible, but that seems like strictly more safety over what blink is providing now, is that right?

[Kinuko Yasuda]

Thanks, if we could put together a proposal for changing base::Callback and friends that'd be really really awesome.

On Fri, Oct 16, 2015 at 7:03 AM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, Oct 15, 2015 at 2:50 PM, Vladimir Levin <vmp...@google.com> wrote:

On Thu, Oct 15, 2015 at 1:18 PM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 12, 2015 at 7:18 PM, Kinuko Yasuda <kin...@chromium.org> wrote:

On Tue, Oct 13, 2015 at 8:52 AM, Dana Jansens <dan...@chromium.org> wrote:

On Mon, Oct 12, 2015 at 4:42 PM, Daniel Cheng <dch...@chromium.org> wrote:

The problems with WTF::String stem from the fact that it is internally refcounted, but the refcounting is not thread-safe. https://docs.google.com/document/d/17qforp6RQ6GIy3Jr5qA04krKTyfZe3LdPQVTjYHyFpA/edit has the details (sorry, I think this is google.com only).

Oh :( This is a huge problem in chromium too FWIW. Any time you stick a RefCountedThreadSafe object into a Closure and PostTask it, you have no idea where it will be deleted. Often people want to do some thread-specific thing in the destructor and they just.. can't. So the (unfortunately) common pattern is to PostTask to the right thread from the destructor, but by then it's too late to handle the case that PostTask can fail.. so you can just leak/fail to destruct.

WTF::String is NOT thread-safe ref-counted, yet it's used in cross-thread post tasking (partly because we have no other thread-safe string representation in Blink other than std::string), so the situation is probably worse.

Hypothetically.. would making Callback move-only solve all of the problems here? I'm not sure how intractable that would be but it could help a lot in Chrome probably.

There's some mention of WTF callbacks being one-shot which differs from Base. But what impact does this have on WTF::String? We could perhaps provide a wrapper around a base::Callback like WTF::OneShotCallback that crashes if you try call it twice or something (we have a similar thing in cc::SingleReleaseCallback), or more specific wrappers in the places in blink that really care about this?

This is all very subtle but let me try to explain.  (hiroshige@ could probably give more details and concrete examples)

Having a move-only closure helps to avoid leaking a reference on the calling thread after postTask().  Or having a move-only wrapper for WTF::String (and all such racy classes) could be also useful.  But this only solves one aspect, in order to safely pass a WTF::String to another thread one also needs to create an isolated copy of WTF::String which doesn't share any ref-counting with existing String objects.  (I think this answers to one of your questions)

And one more thing to note, the same thing needs to apply to all objects that contain WTF::String in any of their members.  So if one wants to bind KURL for running a method on another thread, we first need to create an isolated copy of KURL without leaking any reference after postTask because KURL contains WTF::String.  Ditto if one wants to bind an object that contains KURL.

So the biggest problem is, even if we have move-only callback, the caller needs to choose the right one and does the right thing, and it's almost impractical to assume that everyone understands all these well and does the right thing (TM).

So what hiroshige@ did in Blink was to make WTF::bind() always return move-only (so that no one can leave an unexpected reference after postTask), and to introduce a new WTF::bind() for cross-thread version which does this all (so that all cross-thread post tasking could be automatically taken care of, while keeping the same-thread post tasking to work as is with full benefit of ref-counting for performance).

As far as I can tell, the thread safe bind uses WTF::bind, but it just makes deep copies of objects that are known to be ref counted but not thread safe (such as KURL and String). This thread safe bind can still remain, but use base::Bind internally instead, right? Assuming of course that base::Bind has a version that returns a move only callback. So, if bind returns a move only callback of a different type like base::MoveOnlyCallback for lack of a better name, then PostTask would also have to deal with the fact that there's a move only callback as well as a copy-able one and possibly do asserts in the copy-able version to ensure that it's not going cross threads.

Additionally, I don't think there's any checks in place to enforce that people actually use threadSafeBind instead of regular WTF::bind with objects that are not thread safe ref counted. That is, at least I couldn't find any of these checks. So, even if you use move only callbacks, there's still no guarantee that both problems are solved. One needs to ensure that threadSafeBind is used instead WTF::bind

Because we're still on the way of making things safer :)  We have a plan to add more systematic checks, both statically and dynamically, so that the things developers need to take care become smaller (if not zero):

https://docs.google.com/document/d/1mGPT6ypz9ZBEbD0KpZ4J4fvhI195TAwtlql0VwI6Wbs/edit#

I haven't been giving high priority on this one but I want to have something like this (in addition to safety changes we could probably make in base::Callback) before we start to allow base::Bind.

Was your hope to replace blink::threadSafeBind (in platform/) with uses of base::Bind? Or just to replace WTF::bind() (in wtf/) with base?

The former has type-specfic deep copies for every type that can be posted, which isn't super scalable and doesn't seem well suited to base, and is probably in the right place where it is (modulo completely different ideas).

I think defining and using type-specific deep copy part can be done in a more generic and scalable way (with static checks like described in the above doc), but I agree that it might not be what we want to have in base/.

The document lists two problems. AIUI, move-only Callback solves one, deep copies in blink::threadSafeBind solves the other. Either solution is not good enough without the other, right?  So we still want to make callers in blink use blink::threadSafeBind where it makes sense (for isolatedCopy()). Then if threadSafeBind returned a base::MoveOnlyCallback, we'd have both problems solved?

Sounds like a plan.  We'll also want to apply this on boundary classes like WebString but the boundary situation will be rapidly changing once we have plan from esprehn@ we could probably address those later.  (hiroshige@, wdyt?)

I can imagine our PostTask methods DCHECKing that non-MoveOnly callbacks are only posted to the same thread. It may or may not be possible, but that seems like strictly more safety over what blink is providing now, is that right?

Yeah this would be tough but if we can have consensus I'm all for it (and am willing to help/work moving this forward too).

[Ken Rockot]

Hello thread! I'm wondering if there's been any progress on these fronts lately, and otherwise if there's some way I or others can help make progress.

Specifically:

• Does the Blink team want to move forward with mass annotation for safer cross-thread object handling? Do you need help getting it done (i.e. moar people typing CLs)?
• Is there some material explaining specifically how base::Callback needs to change to be safer? Anyone working on it implementation changes?
• From earlier in the thread: anyone looked/looking at making PostTask enforce that copyable callbacks are called on the posting thread? (FWIW I'm not sure this is something that really makes sense to do since it is hard to implement and IIUC would be trivial to circumvent, accidentally or otherwise.)
• Is there a working draft of an overall plan from esprehn@ available yet?

Getting Blink to consume Mojo interfaces is a high priority for us, but we want to make sure it's done right. We'll take any opportunity we can to help with preparation. :)

Cheers,

Ken

[Kinuko Yasuda]

Hi,

you might already know some of these, but let me put some updates to close the loop:

On Wed, Nov 11, 2015 at 6:50 AM, Ken Rockot <roc...@chromium.org> wrote:

Hello thread! I'm wondering if there's been any progress on these fronts lately, and otherwise if there's some way I or others can help make progress.

Specifically:

• Does the Blink team want to move forward with mass annotation for safer cross-thread object handling? Do you need help getting it done (i.e. moar people typing CLs)?

hiroshige@ has been experimenting, there seem a few problems esp. around raw ptrs (attributing ownership is hard) and I expect we'll need a little more time to make a good progress.  I think this could be discussed/addressed separately from the callback discussion though. (More details can be found in hiroshige's BlinkOn slide: https://docs.google.com/presentation/d/1nDe5iDUQ6U-4WmJMvfBll72xNCUG8wXRrHmofEjLcHw/edit#slide=id.p)

• Is there some material explaining specifically how base::Callback needs to change to be safer? Anyone working on it implementation changes?
• From earlier in the thread: anyone looked/looking at making PostTask enforce that copyable callbacks are called on the posting thread? (FWIW I'm not sure this is something that really makes sense to do since it is hard to implement and IIUC would be trivial to circumvent, accidentally or otherwise.)

Yes this is being discussed and considered.  Dana has started putting up a doc for the possible plan for base::Callback and there's discussion ongoing.

• Is there a working draft of an overall plan from esprehn@ available yet?

You might have already noticed but esprehn@ has given a (awesome) talk about Blink Architecture at BlinkOn: https://docs.google.com/presentation/d/1Am42_Tqn0uqgep6Keoiko0MPMtTsXTbTt5mtA4Z-CwY/edit#slide=id.gd2db2df3a_0_0

I can't speak for Elliot, but in short: Platform and WTF are going to be allowed to directly use base/ (and mojo/IPC), but some files/features (like base::Bind and Callback in our context) are to be excluded by whitelisting (or blacklisting) for the time being until we figure out the plan.