Proposal: allow absl::FunctionRef

[Daniel Cheng]

Today, Chrome uses base::OnceCallback and base::RepeatingCallback to own a reference to a callable object. Because of the ownership requirements, the base callback implementation enforces certain restrictions to make it harder to write bugs. One of these restrictions is that base::BindOnce / base::BindRepeating do not allow capturing lambdas to reduce the likelihood of lifetime bugs such as:

// https://godbolt.org/z/hdb1T1Wvh

#include <functional>
int main() {
  std::function<int()> f;
  {
    int i = 0;
    f = [&] { return i; };
  }
  return f();
}

However, there are times when it is both safe and useful to use a capturing lambda. One common example is a function that iterate over a collection and synchronously applies the callable object to each item, e.g. blink::LocalFrameView::ForAllChildViewsAndPlugins (which uses a templated function to allow the use of capturing lambdas) and content::WebContents::ForEachRenderFrameHost (which must use base::RepeatingCallback since it is a virtual method that cannot be templated).

The latter is particularly unfortunate; since base::BindRepeating does not allow capturing lambdas, "capturing" parameters can be quite verbose, e.g.

  // Get the list of RenderFrameHosts from the current page.
  proxy_->GetWebContents()->GetPrimaryMainFrame()->ForEachRenderFrameHost(
      base::BindRepeating(
          [](std::vector<content::GlobalRenderFrameHostId>*
                 render_frame_host_ids,
             std::vector<mojo::Remote<blink::mojom::TextFragmentReceiver>>*
                 text_fragment_remotes,
             content::RenderFrameHost* rfh) {
            render_frame_host_ids->push_back(rfh->GetGlobalId());
            mojo::Remote<blink::mojom::TextFragmentReceiver> remote;
            text_fragment_remotes->push_back(std::move(remote));
          },
          &render_frame_host_ids_, &text_fragment_remotes_));

In contrast, using absl::FunctionRef allows callers to be simplified and reduces the amount of code duplication. The above example would look something like this instead:

  // Get the list of RenderFrameHosts from the current page.
  proxy_->GetWebContents()->GetPrimaryMainFrame()->ForEachRenderFrameHost(
      [this](content::RenderFrameHost* rfh) {
            render_frame_host_ids_->push_back(rfh->GetGlobalId());
            mojo::Remote<blink::mojom::TextFragmentReceiver> remote;
            text_fragment_remotes_->push_back(std::move(remote));
          });

I'd like to propose that we allow the use of absl::FunctionRef and encourage its use in places where a non-owning reference to a callable object is sufficient. It has additional benefits of being cheap to construct / copy / destroy. For example, content::WebContents::ForEachRenderFrameHost() requires a heap allocation just to adapt between its RenderFrameHost* and RenderFrameHostImpl* overloads: with absl::FunctionRef, no heap allocation would be required.

Daniel

[dan...@chromium.org]

Do you think we can ban it appearing as a class member - including in template instantiations?

 

Daniel

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqUZORDJ2yQsZNc%3DYJnFOkfiy9eHSEaVsM8Mtw%3DR9tk%2Bg%40mail.gmail.com.

[Jeremy Roman]

Might be nice, but it's problematic in largely the same way that, say, base::StringPiece is, so I'm not sure we should block on doing so. (Maybe a little worse because it does an indirect call through it so a bad one might be a little more useful as an exploit primitive than being able to access memory.)

It also wouldn't necessarily be an error to have it as a member in similarly short-lived objects passed as arguments, though it wouldn't be the end of the world if you had to expressly mark cases where that happens, if it happens at all.

 

Daniel

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqUZORDJ2yQsZNc%3DYJnFOkfiy9eHSEaVsM8Mtw%3DR9tk%2Bg%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSBOksE6Zt7o8J8kc8UCE%2BduYVMyMGMMnon8oKK5Yk5eQ%40mail.gmail.com.

[Daniel Cheng]

I guess I wonder how hard we want to try here.

We can try to block direct fields but I feel like there would be quite a bit of complexity to also block:

- absl::optional<absl::FunctionRef>

- std::unique_ptr<absl::FunctionRef>

- std::vector<absl::FunctionRef>

- std::pair<..., absl::FunctionRef>

- absl::FunctionRef*

- et cetera

I do think it's possible to make it work. But in my ideal world, we'd just allow the straightforward thing to be written and make it easy to audit absl::FunctionRef fields that got added (same for other potentially risky constructs where there /should/ be limited usage of the risky form). Maybe we could add a clang-tidy warning to highlight the danger instead of a hard compiler error?

Daniel

[dan...@chromium.org]

Ok these are all good points. I think what we're missing is some education, so people understand it's all non-owning and can't outlive what it refers to, the same as an int& can't outlive it's int.

Would you be willing to write a ToTT or whatever kinda piece to share on chromium-dev when we allow this?

Should we be concerned that absl won't be using raw_ptr?

[Lei Zhang]

Would the extra work of writing a base wrapper around
absl::FunctionRef be helpful here? Both for developer education, and
maybe to fit in raw_ptr use.

> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQ5hunvmEgxKYe5KoE2g2fxa-NO4pLZg2xdUYCbmgxQsw%40mail.gmail.com.

[dan...@chromium.org]

On Fri, Jun 10, 2022 at 2:51 PM Lei Zhang <the...@chromium.org> wrote:

Would the extra work of writing a base wrapper around
absl::FunctionRef be helpful here? Both for developer education, and
maybe to fit in raw_ptr use.

I kinda see it as a negative for education, as folks who learn about the absl type elsewhere won't know our type. But maybe for raw_ptr.. wondering what others think of removing raw_ptr from these use cases.

[Łukasz Anforowicz]

<+memory-s...@chromium.org for raw_ptr<T> question below>

In my mind, raw_ptr<T> is mostly beneficial for *long-lived* pointers.  In particular, we say that raw_ptr<T> shouldn't be used in function parameters, arguing that 1) this would have significant performance impact and 2) would have limited UaF/security benefits because pointers stored/passed in function parameters are *short-lived*.

And from that perspective, storing/capturing raw pointers in absl::FunctionRef seems okay (because we want to restrict absl::FunctionRef to short-lived scenarios, and continue using base::BindOnce / base::BindRepeating on other scenarios).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQ5aV6JgUp%2BrSo71rWzciKg9vCM878TpT_ip-1VMJEn4g%40mail.gmail.com.

[Kentaro Hara]

I'm not sure if long-lived vs. short-lived matters to evaluate the security impact of raw_ptr. ~30% of use-after-frees are happening on on-stack pointers, which are short-lived. The reason raw_ptr does not (yet) support on-stack pointers is NOT because it's less security impact but because on-stack pointers are performance-sensitive. We are planning to start an experiment to support on-stack pointers soon.

So, ideally I think that raw_ptr should be supported for absl::FunctionRef. However, given that it's not yet clear if raw_ptr can support on-stack pointers in general for performance reasons, I don't feel strongly about it.

You received this message because you are subscribed to the Google Groups "memory-safety-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to memory-safety-...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAA_NCUEEJrPLB%3DThLNg9H1RKUYwaSXDDZHSWQV3vj0wYqAVWRg%40mail.gmail.com.
For more options, visit https://groups.google.com/a/chromium.org/d/optout.

--

Kentaro Hara, Tokyo

[Gabriel Charette]

Feedback from first hearing about absl::FunctionRef and reading such code: I was initially a bit confused on how this instantiates an absl::FunctionRef instead of a std::function? Then realized it's just an implicit conversion based on `ForEachRenderFrameHost` accepting an absl::FunctionRef as a parameter.

Given we already allow using std::functions when calling synchronous STL APIs: the discussion is about whether we want to allow signing our own methods with this type?

I think this is okay given developers already must understand this pattern when calling such STL APIs. And as others have said, this is no different than other "ref" types (shouldn't use/store beyond caller scope).

As such, this LGTMs.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CABg10jyy0wU0wsq-abhEW86Tf5Z0JuA17P2L_GEoECkmx5xENQ%40mail.gmail.com.

[Jeremy Roman]

There is no std::function in those call sites; it seems as though you may be confusing lambda expressions for std::function objects. The type of [](int x) { return x+1; } is an unnamed lambda type, not std::function<int(int)>.

Both std::function and absl::FunctionRef can be constructed from lambdas (as well as other things). std::function does so by copying the lambda (or other functor) to the heap, and absl::FunctionRef does so by storing a pointer to it. (Both also erase the type of the underlying functor using an indirect function call.)

The thing that is typical in Chromium today is passing a lambda to an STL function, which expands the template using the actual type of that lambda (typically passed by reference). std::function isn't involved in those cases at all.

[Gabriel Charette]

Sorry, yes, this is what I meant. Hadn't fully understood the distinction between a lambda and a std::function until now.

Usability-wise, my thought process stands.

[Vladimir Levin]

The use of std::function is currently banned (https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++-features.md#Function-Objects-banned)

I'm not sure I follow how the argument for absl::FunctionRef is different from the arguments presented in the std::function thread

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LLx%2BkL5-pJhcP7h4ZxSnKo%3Dw-UoFiT00oEnjmWbWNFyKw%40mail.gmail.com.

[Jeremy Roman]

absl::FunctionRef would be restricted to narrow cases, where the function ref doesn't outlive the function call to which it was passed as an argument. In those cases most of the reasons to use base::*Callback over std::function don't apply.

[Vladimir Levin]

Turns out I was thinking of a different thread, where I was hoping to allow std::function for such cases: https://groups.google.com/a/chromium.org/g/cxx/c/YDDL33mJybc/m/KS6rr4DGBwAJ but it seems that that thread went off into a different direction

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13dqS-Bk_kHvzQCGcB0Ufp-HMak-fZmULKFyAoMi1hQFKQ%40mail.gmail.com.

[Sunny Sachanandani]

+1 to absl::FunctionRef. Recently I've seen methods that are templated just for the ability to pass in a capturing lambda e.g. DrawingBuffer::CopyToPlatformInternal. Using absl::FunctionRef would improve readability by documenting the lambda's signature and reduce code bloat from the unnecessary templating.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CADsXd2OmzbOWUacX1HzrdhamtzgEru%3DBm6320a9xt84wOjen8Q%40mail.gmail.com.

[K. Moon]

There don't seem to be unresolved concerns around this. Time to make it happen?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKqK8MkQUw6xg%2BV%2BCVCYa5NcMezoFoAuN_sRZOz5crf_vQY9dw%40mail.gmail.com.

[dan...@chromium.org]

I spoke to Daniel about this but forgot to raise it here. I'd really like to see it possible to construct a FunctionRef from a Callback. Otherwise, the receiver dictates what the sender(s) (transitively) must create. Similar to how string_view can be constructed from string.

[Daniel Cheng]

I'm working on writing this up, but I'm not entirely sure how to resolve the OnceCallback / RepeatingCallback -> FunctionRef implicit conversion.

FunctionRef doesn't have any notion of how many times it can run; does that mean we should only allow an implicit conversion from RepeatingCallback?

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CAHtyhaTeq0KyChSVoS8MSQA%3DRzmJHzPnZZTbax6xNJOYVsYizw%40mail.gmail.com.

[dan...@chromium.org]

On Thu, Jun 16, 2022 at 2:56 PM Daniel Cheng <dch...@chromium.org> wrote:

I'm working on writing this up, but I'm not entirely sure how to resolve the OnceCallback / RepeatingCallback -> FunctionRef implicit conversion.

FunctionRef doesn't have any notion of how many times it can run; does that mean we should only allow an implicit conversion from RepeatingCallback?

Yeah that sounds "right" then. Unfortunate mismatch in our APIs really. Is it better to not do it than to have this mismatch? Would this cause people to use RepeatingCallback when they should be using OnceCallback? That would be worse.

[K. Moon]

If we don't do this, should we add something like RepeatingCallbackRef/OnceCallbackRef?

[Jeremy Roman]

If we had operator() rather than Run it would just work for RepeatingCallback. Otherwise, it's of course possible to adapt, like:

[&callback](auto&& args...) { std::move(callback).Run(std::forward<decltype(args)>(args)...); }

and the same without std::move for RepeatingCallback. (Or a version without template voodoo if you know the concrete types involved. We could provide a helper function or conversion operator if we wished. In any case I don't think this confines callers from using callbacks.

On Thu, Jun 16, 2022 at 5:25 PM K. Moon <km...@chromium.org> wrote:

If we don't do this, should we add something like RepeatingCallbackRef/OnceCallbackRef?

How would this be different from a normal reference to RepeatingCallback/OnceCallback?

[dan...@chromium.org]

On Fri, Jun 17, 2022 at 10:48 AM Jeremy Roman <jbr...@chromium.org> wrote:

If we had operator() rather than Run it would just work for RepeatingCallback. Otherwise, it's of course possible to adapt, like:

[&callback](auto&& args...) { std::move(callback).Run(std::forward<decltype(args)>(args)...); }

and the same without std::move for RepeatingCallback. (Or a version without template voodoo if you know the concrete types involved. We could provide a helper function or conversion operator if we wished. In any case I don't think this confines callers from using callbacks.

On Thu, Jun 16, 2022 at 5:25 PM K. Moon <km...@chromium.org> wrote:

If we don't do this, should we add something like RepeatingCallbackRef/OnceCallbackRef?

How would this be different from a normal reference to RepeatingCallback/OnceCallback?

It should be different in the same way FunctionRef is different or it's not adding anything.

[Jeremy Roman]

On Fri, Jun 17, 2022 at 10:50 AM <dan...@chromium.org> wrote:

On Fri, Jun 17, 2022 at 10:48 AM Jeremy Roman <jbr...@chromium.org> wrote:

If we had operator() rather than Run it would just work for RepeatingCallback. Otherwise, it's of course possible to adapt, like:

[&callback](auto&& args...) { std::move(callback).Run(std::forward<decltype(args)>(args)...); }

and the same without std::move for RepeatingCallback. (Or a version without template voodoo if you know the concrete types involved. We could provide a helper function or conversion operator if we wished. In any case I don't think this confines callers from using callbacks.

On Thu, Jun 16, 2022 at 5:25 PM K. Moon <km...@chromium.org> wrote:

If we don't do this, should we add something like RepeatingCallbackRef/OnceCallbackRef?

How would this be different from a normal reference to RepeatingCallback/OnceCallback?

It should be different in the same way FunctionRef is different or it's not adding anything.

absl::FunctionRef adds type erasure, so that callers can supply functions of a variety of concrete types.

base::OnceCallback/RepeatingCallback already erase the type of the bound functor.

[K. Moon]

I think base::*Callback& would require a heap allocation, while base::*CallbackRef wouldn't? I thought this was the key motivation behind supporting something like this in the first place.

[K. Moon]

FWIW, the absl::FunctionRef documentation has this to say:

This class is suitable for use wherever a "const std::function<>&"
would be used without making a copy. ForEach functions and other versions of
the visitor pattern are a good example of when this class should be used.

So only supporting RepeatingCallback might not be that bad an outcome. I'm sure there are cases where a OnceCallback would be useful (boilerplate wrapper, I guess), but probably much rarer.

[Jeremy Roman]

On Fri, Jun 17, 2022 at 12:30 PM K. Moon <km...@chromium.org> wrote:

FWIW, the absl::FunctionRef documentation has this to say:

This class is suitable for use wherever a "const std::function<>&"
would be used without making a copy. ForEach functions and other versions of
the visitor pattern are a good example of when this class should be used.

So only supporting RepeatingCallback might not be that bad an outcome. I'm sure there are cases where a OnceCallback would be useful (boilerplate wrapper, I guess), but probably much rarer.

On Fri, Jun 17, 2022 at 8:56 AM K. Moon <km...@chromium.org> wrote:

I think base::*Callback& would require a heap allocation, while base::*CallbackRef wouldn't? I thought this was the key motivation behind supporting something like this in the first place.

If there's an existing base::*Callback then a & to it shouldn't require an allocation. While we could implement another Bind that produced on-stack callbacks, I think most of the reasons we prefer base::*Callback to std::function don't apply to the immediate on-stack case -- and the benefit of being able to transparently capture things using lambda expressions is stronger.

[K. Moon]

I agree, but I think danakj@'s arguments are that:

1. It shouldn't be hard to adapt an existing OnceCallback/RepeatingCallback to a signature that only takes a FunctionRef.

2. It's useful to capture the once vs. repeating distinction in the type system, just as we do with *Callback.

std::function and absl::FunctionRef are unconcerned with (2), so if that's important, I don't think absl::FunctionRef satisfies our requirements. In which case, the question is whether that requirement is useful for the situations that absl::FunctionRef is appropriate. (As a thought experiment, I do wonder now if some sort of Once<std::function<foo()>> or std::function<Once<foo()>> wrapper would be able to express the same requirements.)

(1) possibly is not that valuable, since the whole point of an API taking a FunctionRef is to avoid the overhead of *Callback. It should be easy to adapt this case, but also inconvenient enough to perhaps encourage a better design, so I don't think this is as critical as (2).

[K. Moon]

Incidentally, my two cents: I think the use cases for FunctionRef are sufficiently disjoint from the use cases for OnceCallback/RepeatingCallback that it's fine to have all three coexist. As jbroman@ pointed out, the most common way to use a FunctionRef probably will be with a lambda.

[Jan Wilken Dörrie]

+1 to allowing absl::FunctionRef and adding an operator() to base::RepeatingCallback to allow the implicit conversion to just work.

Re absl::FunctionRef and base::OnceCallback: Like jbroman@ said, one could add something like

base::OnceCallback<R(Args...)>::operator absl::FunctionRef<R(Args...)>() && {

  return [this](Args... args) { return std::move(*this).Run(args...); };

}

that allows you to do TakesFunctionRef(..., std::move(once_cb)), but this is not great for a few reasons:

 * You don't know whether the callback will actually be run

 * The compiler / tooling can't warn you about running it more than once

 * You basically circumvent const correctness, since the lambda is only shallowly const

If supporting OnceCallbacks is generally desired, I think the better solution is to also adopt absl::AnyInvocable (a std::move_only_function backport, just released three days ago). As opposed to std::function this wrapper is move-only (and thus can store move-only types), and can capture invoke once semantics (e.g. an absl::AnyInvocable<void() &&> is the equivalent of a base::OnceClosure). Thanks to SBO converting an OnceCallback to an AnyInvocable wouldn't result in a heap allocation either. All that is needed to opt into this conversion is to add an r-ref qualified operator() to OnceCallback.

That said, adopting AnyInvocable of course raises some other issues that should be resolved in a separate adoption proposal thread.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-58fS1kPv3SdXnjuCO9aSLOyZqEzrHQjU%2B6%3DS25K%2BNZwQ%40mail.gmail.com.

[dan...@chromium.org]

On Fri, Jun 17, 2022 at 5:23 PM K. Moon <km...@chromium.org> wrote:

I agree, but I think danakj@'s arguments are that:

1. It shouldn't be hard to adapt an existing OnceCallback/RepeatingCallback to a signature that only takes a FunctionRef.

2. It's useful to capture the once vs. repeating distinction in the type system, just as we do with *Callback.

std::function and absl::FunctionRef are unconcerned with (2), so if that's important, I don't think absl::FunctionRef satisfies our requirements. In which case, the question is whether that requirement is useful for the situations that absl::FunctionRef is appropriate. (As a thought experiment, I do wonder now if some sort of Once<std::function<foo()>> or std::function<Once<foo()>> wrapper would be able to express the same requirements.)

(1) possibly is not that valuable, since the whole point of an API taking a FunctionRef is to avoid the overhead of *Callback. It should be easy to adapt this case, but also inconvenient enough to perhaps encourage a better design, so I don't think this is as critical as (2).

Thanks Kahmy, this is a better summary than I did. :) I thought about this some more and agree with where this thread is going. The distinction of Once vs Repeating provides value in making ownership clear. It says that stored args will either be moved to the receiver or still be held in the callback. For a FunctionRef, there is no ownership so that value is not present. Also agree that the point of it is for avoiding the overhead of callback.

So +1 from me, thanks for the discussion here.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/memory-safety-dev/CACwGi-5xk_yb9YzDHXAX7mivmPJ7T1YF0tkkE9AE8f2445Smmg%40mail.gmail.com.

[K. Moon]

It's been a couple weeks, and it sounds like there's consensus. Good to go ahead with allowing FunctionRef as-is?

[Daniel Cheng]

Yes, I'll write it up later today :)

Daniel

[K. Moon]

Awesome, thanks!

[Daniel Cheng]

https://chromium-review.googlesource.com/c/chromium/src/+/3741218 is the proposal for this.

Note that one important difference from the discussion here is that I have opted to *disallow* conversions from base's callback to absl::FunctionRef for now. In short, the theory is that code passing a callback to an absl::FunctionRef is likely better off using a capturing lambda.

However, there might be some use cases that this doesn't account for, and so I've left explicit messages to provide feedback, and it's quite possible we would end up enabling these conversions in the future based on this feedback.

Daniel