base::Bind and WTF::bind

[Kentaro Hara]

Hi

yukishiino@ is now migrating content/renderer/battery_status/ to Blink's platform and hitting an issue around function binding. See this CL, in particular this file and this file.

In short, the problem is:

- Blink uses WTF::bind.

- Chromium uses base::Bind.

- BatteryStatusDispatcher (in Blink) needs to register a callback (i.e., BatteryStatusDispatcher::onDidChange) on device::BatteryMonitorPtr (in Chromium). In other words, we need to bind a function in Blink and ask Chromium to call back the function.

- Which should I use, WTF::bind or base::Bind?

There are multiple approaches for this issue:

(a) Allow Blink to use base::Bind as well as WTF::bind.

(b) Replace all WTF::bind's with base::Bind.

(c) Implement some converter between WTF::bind and base::Bind.

(d) Use C++'s lambda function.

Currently (d) is not allowed in http://chromium-cpp.appspot.com/. (b) is rejected by kinuko@ and hiroshige@ because of some complexity around threading.

Do you have any thoughts on this?

--

Kentaro Hara, Tokyo, Japan

[Anand Mistry]

For your CLs which have to do with Mojo, we don't care about base::Callback, but rather mojo::Callback, so there are options that don't involve base::Bind.

There's option (e) which is to extend mojo::Callback to support the way Blink callbacks work. mojo::Callback doesn't explicitly support base::Callback. But instead, it matches against a pattern use by base::Callback, namely the presence of a Run() function. The issue with Blink's callbacks are that they are wrapped in an OwnPtr. Another thread on this list mentions converting OwnPtr to std::unique_ptr. That would make supporting Blink callbacks directly in mojo::Callback possible, bypassing any issues with base::Bind.

But for now, you can create your own adapter class that does (c) without referring to base::Bind. I've asked Sam to share what he's written.

[Kinuko Yasuda]

(+Dana)

Eventually I think we're aiming for (b'), i.e. we unify WTF::bind and base::Bind so that we can use the consistent, same bind throughout our code base.

(As amistry suggests for this case making wtf closure run in mojo binding code could be another non-conflicting option which could probably happen earlier before we unify bind's.)

Until then I think allowing base::Bind only in a very limited / controlled context with its own adapter class could be a reasonable way to go.

[Yuta Kitamura]

Not directly answering haraken's question but let me add some info.

BackgroundTaskRunner (in Source/platform) uses base::Bind:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/threading/BackgroundTaskRunner.cpp&sq=package:chromium&type=cs&l=22

It binds PassOwnPtr with base::Bind which is a surprise to me, because that means PassOwnPtr type leaks to the Chromium side. This seems unsafe because PassOwnPtr has "destructive" copy semantics like std::auto_ptr and base::Bind gives no warranty that PassOwnPtr is handled correctly. I think in this specific case PassOwnPtr should be converted to std::unique_ptr before calling base::Bind.

Using base::Bind in Blink is kinda tricky so it's best if we can avoid it.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAMWgRNa-hguHdHp7h910Ue%2Bd7gjv239t8ajgTbs3w2GM72L%2BXg%40mail.gmail.com.

[Kentaro Hara]

kinuko-san: I just want to understand but what is making difficult to unifying WTF::bind into base::Bind? (Is it blocked by the OwnPtr => unique_ptr replacement or...?)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAFJcur8JUiG9%3Dm02kWxhJvHX63XiG-xtWRjBgcr2s4LARybu1A%40mail.gmail.com.

[Hiroshige Hayashizaki]

> (c) Implement some converter between WTF::bind and base::Bind.

I think this is acceptable, if we can control the usage of the converter: we have only one converter (or a few converters), and we can remove them mechanically when

(b'), we unify WTF::bind and base::Bind in the future (https://docs.google.com/document/d/114s1KTrf6xXdwInRDMDJAdxX_zIKt6xJSbZ-9qBgU38/edit?ts=565e9e37#).

> (d) Use C++'s lambda function.

C++'s lambda (or std::function) can bypass restrictions by WTF::bind() or base::Bind() (for threading issues or so), so I think we should avoid this.

On Wed, Jan 27, 2016 at 2:22 PM, Yuta Kitamura <yu...@chromium.org> wrote:

[Kinuko Yasuda]

On Wed, Jan 27, 2016 at 2:22 PM, Yuta Kitamura <yu...@chromium.org> wrote:

Not directly answering haraken's question but let me add some info.

BackgroundTaskRunner (in Source/platform) uses base::Bind:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/threading/BackgroundTaskRunner.cpp&sq=package:chromium&type=cs&l=22

It binds PassOwnPtr with base::Bind which is a surprise to me, because that means PassOwnPtr type leaks to the Chromium side. This seems unsafe because PassOwnPtr has "destructive" copy semantics like std::auto_ptr and base::Bind gives no warranty that PassOwnPtr is handled correctly. I think in this specific case PassOwnPtr should be converted to std::unique_ptr before calling base::Bind.

This is tricky code which shouldn't prevail, but in this particular case the PassOwnPtr'd be stored in base::Callback as is and then destructed when a method runs, so I think this could work...?

 

Using base::Bind in Blink is kinda tricky so it's best if we can avoid it.

Yup... for now.

[Sam McNally]

Here's the adapter I wrote. It needs WTF::Function to support move-only types to support mojo structs, but it seems to work for primitive types.

template <typename... Args>

class CallbackAdapter {

public:

    using Callback = Function<void(Args...)>;

    CallbackAdapter(PassOwnPtr<Callback> callback)

        : m_core(adoptRef(new Core(callback)))

    {

    }

    template <typename... RunArgs>

    void Run(RunArgs&&... args) const

    {

        ASSERT(m_core->m_callback);

        OwnPtr<Callback> callback = m_core->m_callback.release();

        (*callback)(std::forward<RunArgs>(args)...);

    }

private:

    struct Core : public WTF::RefCounted<Core> {

        Core(PassOwnPtr<Callback> callback) : m_callback(callback) {}

        OwnPtr<Callback> m_callback;

    };

    RefPtr<Core> m_core;

};

template <typename... Args>

mojo::Callback<void(Args...)> adaptFunctionForMojo(PassOwnPtr<Function<void(Args...)>> callback)

{

    return CallbackAdapter<Args...>(callback);

[Kinuko Yasuda]

Would the document hiroshige@ pasted help?

In short: base::Bind makes copyable, reusable callbacks while wtf::bind makes non-copyable but reusable callbacks.  Current promising plan is to unify these by 1) introducing a new bind/callback template where we can configure copyablity/reusability as different types, then 2) switching all base::Bind (and wtf::bind maybe) to the new one and then 3) gradually migrating all dangerous configurations to the safe ones (i.e. all copyable usages to non-copyable usages, all reusable usages on cross-thread/sequence to non-reusable usages).  The step 3) would take longer time, while we could likely start using the same bind after step 2) is done (assuming that we could prohibit dangerous bind/callback types in blink by presubmits so that we can keep current wtf::bind's semantics at least)

[Yuki Shiino]

hiroshige@'s document and sammc@'s work are very helpful for me.

For the migration of mojo-based services to Blink, the limitation of move-only && one-shot will not be an issue because we create a new callback each time when Blink needs to communicate to a mojo-based service, IIUC.  The goal of the new bind/callback seems great to me.

For the time being (until the new bind/callback gets ready), can we go on with limited use of base::Bind()?  Considering that WTF::Function doesn't support move-only types so far and that anyway the adapter will be replaced with the new bind/callback, limited use of base::Bind seems acceptable to me.  Probably we'd like to name the thin wrapper of base::Bind something like blink::bindInstanceAndMethodToMojoCallback so that it's clear that this function is designed only for mojo bindings in platform/.

Cheers,

Yuki Shiino

[Kinuko Yasuda]

Yes, I think that could be the way to go (and I think we're already doing in some places).  I think all we'd like to make sure is to limit the context (e.g. directory) we allow base/bind for now, in order to avoid someone just finds out it's available in, say, platform/ and start using it wherever possible.

[Kentaro Hara]

The plan sounds reasonable to me.

[Nico Weber]

(Everything Kinuko said sounds right to me :-) )

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAMWgRNb%3DX%2BJw8HiNOETVC-O-UeKBGWu-j3V1nn%2Buobq-dwsLxA%40mail.gmail.com.

[Dana Jansens]

On Wed, Jan 27, 2016 at 12:01 AM, Kinuko Yasuda <kin...@chromium.org> wrote:

Yes, I think that could be the way to go (and I think we're already doing in some places).  I think all we'd like to make sure is to limit the context (e.g. directory) we allow base/bind for now, in order to avoid someone just finds out it's available in, say, platform/ and start using it wherever possible.

Sounds good to me also.

It would be helpful probably if the code used a scoped_ptr with Bind instead of a PassOwnPtr (it could use unique_ptr technically, but we'll autoconvert that at some point anyhow).  Bind+PassOwnPtr is not well understood by devs and PassOwnPtr is up for deletion anyway.