Re: Proposal: allow absl::FunctionRef

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

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

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.

 

On Fri, Jun 10, 2022 at 11:43 AM <dan...@chromium.org> wrote:
>
> 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?
>
> On Fri, Jun 10, 2022 at 2:04 PM Daniel Cheng <dch...@chromium.org> wrote:
>>
>> 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
>>
>> On Fri, 10 Jun 2022 at 08:33, Jeremy Roman <jbr...@chromium.org> wrote:
>>>
>>>
>>>
>>> On Fri, Jun 10, 2022 at 10:13 AM <dan...@chromium.org> wrote:
>>>>
>>>> On Fri, Jun 10, 2022 at 5:38 AM Daniel Cheng <dch...@chromium.org> wrote:
>>>>>
>>>>> 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.
>>>>
>>>>
>>>> Do you think we can ban it appearing as a class member - including in template instantiations?
>>>
>>>
>>> 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.
>
> --
> 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/CAHtyhaQ5hunvmEgxKYe5KoE2g2fxa-NO4pLZg2xdUYCbmgxQsw%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/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

[dan...@chromium.org]

Thanks Daniel, it LGTM

[Daniel Cheng]

I've been experimenting with absl::FunctionRef more, and I'm actually wondering if we should reconsider its usage. absl::FunctionRef<R(Args...)> allows many implicit conversions, similar to std::function<R(Args...)>. This has some unfortunate implications.

This piece of code compiles:

  void F(absl::FunctionRef<void(int)>);

  int main() {
    F([] (int x) { });
  }

But this works too:

  void F(absl::FunctionRef<void(int)>);

  int main() {
    F([] (int x) -> int { return 0; });

  }

As mek pointed out, this is because absl::FunctionRef's invoke helper can silently adapt itself to ignore return values if binding to a callable that does not return void.

In https://chromium-review.googlesource.com/c/chromium/src/+/3767487/5, I attempted to migrate several helpers in //content to use absl::FunctionRef.

Previously, we had:

  // `callback` may return `kStop` to cancel the traversal, `kSkipChildren` to skip traversing child frames,

  // or `kContinue` to continue.

  void ForEachRenderFrameHost(base::RepeatingCallback<FrameIterationAction(RenderFrameHost*)> callback);

  // Similar to the above, but always continues.

  void ForEachRenderFrameHost(base::RepeatingCallback<void(RenderFrameHost*)> callback);

This gets changed to:

  // `callback` may return `kStop` to cancel the traversal, `kSkipChildren` to skip traversing child frames,
  // or `kContinue` to continue.
  void ForEachRenderFrameHostWithAction(absl::FunctionRef<FrameIterationAction(RenderFrameHost*)> callback);
  // Similar to the above, but always continues.
  void ForEachRenderFrameHost(absl::FunctionRef<void(RenderFrameHost*)> callback);

Note that one of the previously overloaded functions has been renamed. This is required: otherwise, the compiler would not be able to pick an overload because [] (RenderFrameHost* rfh) { ... } is convertible to both absl::FunctionRef<FrameIterationAction(RenderFrameHost*)> and absl::FunctionRef<void(RenderFrameHost*)>.

But now we have a new problem: if someone accidentally writes:

  // Should be `ForEachRenderFrameHostWithAction()`

  rfh->ForEachRenderFrameHost([&first_rfh] (RenderFrameHost* rfh) {
    first_rfh = rfh;
    return RenderFrameHost::FrameIterationAction::kStop;
  });

This compiles but does not do what the author expected, because absl::FunctionRef<void(RenderFrameHost*)> silently allows the implicit conversion and swallows the returned FrameIterationAction.

Daniel

[Daniel Cheng]

For a more real-life demonstration, https://chromium-review.googlesource.com/c/chromium/src/+/3767487/4..6 is a diff of issues I accidentally introduced while trying to make a "no-op" conversion CL.

I also wrote a stricter FunctionRef prototype that doesn't allow these implicit conversions to silently take place: https://chromium-review.googlesource.com/c/chromium/src/+/3768723/

So it is definitely possible to implement.

Daniel

[Jeremy Roman]

We could, but often these conversions are desirable, like converting Derived* to Base* if a function needs to produce a pointer to an object. The case of having two different overloads of a function where one takes a void-returning function and the other takes a non-void-returning function with the same parameter types seems like the exception rather than the rule.

These are the exact same kinds of issues you would encounter if you did the same thing as direct rather than indirect function calls:

int Foo(); // without [[nodiscard]]

Foo(); // compiles, silently discards the return value

In particular cases where we think this mistake is likely like the ForEachRenderFrameHost case, we could provide an overload to catch the "bad" ones.

[Jeremy Roman]

Another thought: does marking FrameIterationAction itself [[nodisard]] produce an error in this case?

[Daniel Cheng]

I think we should distinguish between:

- allowing the conversion when making the actual call

- converting between function pointer types

I see absl::FunctionRef's implicit conversions as far more in the spirit of the latter than the former. For example, it's not legal to pass a void (*)(Base*) to a void (*)(Derived*), even though it's possible to invoke a void (*)(Base*) by passing it a Derived*. There are times when it's convenient of course—but that seems to be relatively rare at the moment, based on my initial conversion attempts. It's always possible to consider ways to relax the conversions in the future (e.g. allow Derived -> Base conversions but not non-void -> void).

Marking it [[nodiscard]] doesn't help because of how the FunctionRef's helper thunk works: https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/functional/internal/function_ref.h;l=73;drc=90ee244bce30e32b4a70090caef3e9eef12e7f52

Godbolt example: https://godbolt.org/z/f5WTrvKxh

Daniel

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

[Jeremy Roman]

On Mon, Jul 18, 2022 at 12:17 PM Daniel Cheng <dch...@chromium.org> wrote:

I think we should distinguish between:

- allowing the conversion when making the actual call

- converting between function pointer types

I see absl::FunctionRef's implicit conversions as far more in the spirit of the latter than the former. For example, it's not legal to pass a void (*)(Base*) to a void (*)(Derived*), even though it's possible to invoke a void (*)(Base*) by passing it a Derived*. There are times when it's convenient of course—but that seems to be relatively rare at the moment, based on my initial conversion attempts. It's always possible to consider ways to relax the conversions in the future (e.g. allow Derived -> Base conversions but not non-void -> void).

The reason this isn't possible with function pointers seems to me to have more to do with ABI level considerations than API level considerations.

The other approach would be to identify cases we think are more likely to be problematic and introduce (bypassable?) warnings for those exact cases, rather than changing the semantics of existing behavior.

I'd also register my generic resistance to swimming upstream against both the C++ standard library and Abseil (and by extension, Google internal C++) without any Chromium-specific reason.

[Daniel Cheng]

On Mon, 18 Jul 2022 at 10:47, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 12:17 PM Daniel Cheng <dch...@chromium.org> wrote:

I think we should distinguish between:

- allowing the conversion when making the actual call

- converting between function pointer types

I see absl::FunctionRef's implicit conversions as far more in the spirit of the latter than the former. For example, it's not legal to pass a void (*)(Base*) to a void (*)(Derived*), even though it's possible to invoke a void (*)(Base*) by passing it a Derived*. There are times when it's convenient of course—but that seems to be relatively rare at the moment, based on my initial conversion attempts. It's always possible to consider ways to relax the conversions in the future (e.g. allow Derived -> Base conversions but not non-void -> void).

The reason this isn't possible with function pointers seems to me to have more to do with ABI level considerations than API level considerations.

The other approach would be to identify cases we think are more likely to be problematic and introduce (bypassable?) warnings for those exact cases, rather than changing the semantics of existing behavior.

I guess that would require clang plugin modifications, which I can't say I'm enthusiastic about.

 

I'd also register my generic resistance to swimming upstream against both the C++ standard library and Abseil (and by extension, Google internal C++) without any Chromium-specific reason.

Well, there are reasons though :)

1. absl::FunctionRef's adaptability is a mismatch from base::{Once,Repeating}Callback [1]. What should be a straightforward migration becomes much trickier when our core constructs have pitfalls like this. It *can* be worked around, but this brings me to #2.

2. In general, Chrome has been trending towards encouraging constructs that are resistant to misuse, *even if* that means some divergence from the C++ standard library. Some differences are less obvious (e.g. explicitly crashing when trying to deref a null optional rather than treating it as UB), but some are more obvious (e.g. base::expected does not have operator*).

3. It's always much easier to relax restrictions rather than try to add them later. Why not start out with the use cases we know we need, and work on allowing more in the future?

Daniel

[1] Yes, this is in large part due to implementation details of base::{Once,Repeating}Callback, but base::{Once,Repeating}Callback also made a very intentional decision to disallow seamlessly adapting a non-void returning function to a void return function.

[Jeremy Roman]

On Mon, Jul 18, 2022 at 2:39 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 18 Jul 2022 at 10:47, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 12:17 PM Daniel Cheng <dch...@chromium.org> wrote:

I think we should distinguish between:

- allowing the conversion when making the actual call

- converting between function pointer types

I see absl::FunctionRef's implicit conversions as far more in the spirit of the latter than the former. For example, it's not legal to pass a void (*)(Base*) to a void (*)(Derived*), even though it's possible to invoke a void (*)(Base*) by passing it a Derived*. There are times when it's convenient of course—but that seems to be relatively rare at the moment, based on my initial conversion attempts. It's always possible to consider ways to relax the conversions in the future (e.g. allow Derived -> Base conversions but not non-void -> void).

The reason this isn't possible with function pointers seems to me to have more to do with ABI level considerations than API level considerations.

The other approach would be to identify cases we think are more likely to be problematic and introduce (bypassable?) warnings for those exact cases, rather than changing the semantics of existing behavior.

I guess that would require clang plugin modifications, which I can't say I'm enthusiastic about.

 

I'd also register my generic resistance to swimming upstream against both the C++ standard library and Abseil (and by extension, Google internal C++) without any Chromium-specific reason.

Well, there are reasons though :)

1. absl::FunctionRef's adaptability is a mismatch from base::{Once,Repeating}Callback [1]. What should be a straightforward migration becomes much trickier when our core constructs have pitfalls like this. It *can* be worked around, but this brings me to #2.

2. In general, Chrome has been trending towards encouraging constructs that are resistant to misuse, *even if* that means some divergence from the C++ standard library. Some differences are less obvious (e.g. explicitly crashing when trying to deref a null optional rather than treating it as UB), but some are more obvious (e.g. base::expected does not have operator*).

I do admit to wondering why some of these are so bad for Chromium but perfectly fine in Google servers. I suspect this boils down to the individual opinions of the people making the decisions.

Changing UB to a crash won't break otherwise correct code (and in fact crash was always a valid thing to do in UB). I still think we shouldn't write code that assumes that optional::operator* crashes, if we can avoid it. base::expected lacking operator* is a little different (honestly not sure I necessarily agree there either) but is at least pretty obvious what the workaround is (call value() instead).

 

3. It's always much easier to relax restrictions rather than try to add them later. Why not start out with the use cases we know we need, and work on allowing more in the future?

I guess it just feels like by carving out a different line we're going to be more surprising for anyone familiar with the more usual implementations. I wouldn't fight incredibly hard on this, but if we do want our own wrapper we could just ban void-to-non-void conversions, or return value conversions that wouldn't be implicitly allowed (assuming any are possible here at all), or void, etc.

[Daniel Cheng]

On Mon, 18 Jul 2022 at 12:01, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 2:39 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 18 Jul 2022 at 10:47, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 12:17 PM Daniel Cheng <dch...@chromium.org> wrote:

I think we should distinguish between:

- allowing the conversion when making the actual call

- converting between function pointer types

I see absl::FunctionRef's implicit conversions as far more in the spirit of the latter than the former. For example, it's not legal to pass a void (*)(Base*) to a void (*)(Derived*), even though it's possible to invoke a void (*)(Base*) by passing it a Derived*. There are times when it's convenient of course—but that seems to be relatively rare at the moment, based on my initial conversion attempts. It's always possible to consider ways to relax the conversions in the future (e.g. allow Derived -> Base conversions but not non-void -> void).

The reason this isn't possible with function pointers seems to me to have more to do with ABI level considerations than API level considerations.

The other approach would be to identify cases we think are more likely to be problematic and introduce (bypassable?) warnings for those exact cases, rather than changing the semantics of existing behavior.

I guess that would require clang plugin modifications, which I can't say I'm enthusiastic about.

 

I'd also register my generic resistance to swimming upstream against both the C++ standard library and Abseil (and by extension, Google internal C++) without any Chromium-specific reason.

Well, there are reasons though :)

1. absl::FunctionRef's adaptability is a mismatch from base::{Once,Repeating}Callback [1]. What should be a straightforward migration becomes much trickier when our core constructs have pitfalls like this. It *can* be worked around, but this brings me to #2.

2. In general, Chrome has been trending towards encouraging constructs that are resistant to misuse, *even if* that means some divergence from the C++ standard library. Some differences are less obvious (e.g. explicitly crashing when trying to deref a null optional rather than treating it as UB), but some are more obvious (e.g. base::expected does not have operator*).

I do admit to wondering why some of these are so bad for Chromium but perfectly fine in Google servers. I suspect this boils down to the individual opinions of the people making the decisions.

Because internal Google code ended up just using std::function<T> directly, having something that matches std::function<T>'s interface makes the most sense for them. They don't need to be consistent with base::{Once,Repeating}Callback.

There is certainly some amount of individual preference at play here, but I think the conversion CL I already linked pretty clearly illustrates the potential problems. The only benefit of unconstrained conversion is adhering to the same interface as std::function<T>, but when we have demonstrated examples of how this conversion can easily (and silently) lead to bugs, that doesn't actually seem like a benefit anymore.

Another solution here is to get rid of the overloads, and that's certainly another possible approach. But IMO, that's just kicking the can down the road; it's not particularly hard to introduce this problem again elsewhere in the future.

Daniel

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13f%3DcCH-vqFeB6z-Mp%3DK5gEzUYVpd73a49gBj04WnuGNsQ%40mail.gmail.com.

[dan...@chromium.org]

On Mon, Jul 18, 2022 at 6:55 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 18 Jul 2022 at 12:01, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 2:39 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 18 Jul 2022 at 10:47, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 12:17 PM Daniel Cheng <dch...@chromium.org> wrote:

I think we should distinguish between:

- allowing the conversion when making the actual call

- converting between function pointer types

I see absl::FunctionRef's implicit conversions as far more in the spirit of the latter than the former. For example, it's not legal to pass a void (*)(Base*) to a void (*)(Derived*), even though it's possible to invoke a void (*)(Base*) by passing it a Derived*. There are times when it's convenient of course—but that seems to be relatively rare at the moment, based on my initial conversion attempts. It's always possible to consider ways to relax the conversions in the future (e.g. allow Derived -> Base conversions but not non-void -> void).

The reason this isn't possible with function pointers seems to me to have more to do with ABI level considerations than API level considerations.

The other approach would be to identify cases we think are more likely to be problematic and introduce (bypassable?) warnings for those exact cases, rather than changing the semantics of existing behavior.

I guess that would require clang plugin modifications, which I can't say I'm enthusiastic about.

 

I'd also register my generic resistance to swimming upstream against both the C++ standard library and Abseil (and by extension, Google internal C++) without any Chromium-specific reason.

Well, there are reasons though :)

1. absl::FunctionRef's adaptability is a mismatch from base::{Once,Repeating}Callback [1]. What should be a straightforward migration becomes much trickier when our core constructs have pitfalls like this. It *can* be worked around, but this brings me to #2.

2. In general, Chrome has been trending towards encouraging constructs that are resistant to misuse, *even if* that means some divergence from the C++ standard library. Some differences are less obvious (e.g. explicitly crashing when trying to deref a null optional rather than treating it as UB), but some are more obvious (e.g. base::expected does not have operator*).

I do admit to wondering why some of these are so bad for Chromium but perfectly fine in Google servers. I suspect this boils down to the individual opinions of the people making the decisions.

I think it is for similar reasons why we need to use raw_ptr<T> instead of T*. Chrome is required to handle an enormous amount of arbitrary (and malicious) inputs, and has full access to users' machines. The threat model differs, which has produced different requirements, and as we continue to invest in hardening and improving the Chrome codebase, they continue to diverge along similar axes. go/google3-builds-for-chrome-binaries

[dan...@chromium.org]

On Tue, Jul 19, 2022 at 10:30 AM <dan...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 6:55 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 18 Jul 2022 at 12:01, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 2:39 PM Daniel Cheng <dch...@chromium.org> wrote:

On Mon, 18 Jul 2022 at 10:47, Jeremy Roman <jbr...@chromium.org> wrote:

On Mon, Jul 18, 2022 at 12:17 PM Daniel Cheng <dch...@chromium.org> wrote:

I think we should distinguish between:

- allowing the conversion when making the actual call

- converting between function pointer types

I see absl::FunctionRef's implicit conversions as far more in the spirit of the latter than the former. For example, it's not legal to pass a void (*)(Base*) to a void (*)(Derived*), even though it's possible to invoke a void (*)(Base*) by passing it a Derived*. There are times when it's convenient of course—but that seems to be relatively rare at the moment, based on my initial conversion attempts. It's always possible to consider ways to relax the conversions in the future (e.g. allow Derived -> Base conversions but not non-void -> void).

The reason this isn't possible with function pointers seems to me to have more to do with ABI level considerations than API level considerations.

The other approach would be to identify cases we think are more likely to be problematic and introduce (bypassable?) warnings for those exact cases, rather than changing the semantics of existing behavior.

I guess that would require clang plugin modifications, which I can't say I'm enthusiastic about.

 

I'd also register my generic resistance to swimming upstream against both the C++ standard library and Abseil (and by extension, Google internal C++) without any Chromium-specific reason.

Well, there are reasons though :)

1. absl::FunctionRef's adaptability is a mismatch from base::{Once,Repeating}Callback [1]. What should be a straightforward migration becomes much trickier when our core constructs have pitfalls like this. It *can* be worked around, but this brings me to #2.

2. In general, Chrome has been trending towards encouraging constructs that are resistant to misuse, *even if* that means some divergence from the C++ standard library. Some differences are less obvious (e.g. explicitly crashing when trying to deref a null optional rather than treating it as UB), but some are more obvious (e.g. base::expected does not have operator*).

I do admit to wondering why some of these are so bad for Chromium but perfectly fine in Google servers. I suspect this boils down to the individual opinions of the people making the decisions.

I think it is for similar reasons why we need to use raw_ptr<T> instead of T*. Chrome is required to handle an enormous amount of arbitrary (and malicious) inputs, and has full access to users' machines. The threat model differs, which has produced different requirements, and as we continue to invest in hardening and improving the Chrome codebase, they continue to diverge along similar axes. go/google3-builds-for-chrome-binaries

I guess I should add: the size and age of the codebase (as a single compilation target) differs too.

[K. Moon]

I don't have an opinion on FunctionRef, but FWIW, I've always found ForEachRenderFrameHost()'s overloaded behavior based on the callback's return type to be clever, but confusing. I find myself always checking the ForEachRenderFrameHost() to see what's allowed, as well as the call site to see which version is being invoked. I think not using overloading would be an improvement in this case.

[Daniel Cheng]

On Tue, 19 Jul 2022 at 10:31, K. Moon <km...@chromium.org> wrote:

I don't have an opinion on FunctionRef, but FWIW, I've always found ForEachRenderFrameHost()'s overloaded behavior based on the callback's return type to be clever, but confusing. I find myself always checking the ForEachRenderFrameHost() to see what's allowed, as well as the call site to see which version is being invoked. I think not using overloading would be an improvement in this case.

Well, the current CL does remove that overloading by renaming one of them (though some people still consider this an overload in spirit). The problem is that even with that, it's still possible to write code that silently does something other than what you'd expect.

Daniel

[K. Moon]

This behavior seems to trace back to the C++ standard's definition of invocability (such as https://en.cppreference.com/w/cpp/types/is_invocable), which explicitly allows the conversion of any return type to void. I can see this being useful in some cases, but in any case, I think it might be unfair to single out FunctionRef for this concern, as it seems like the same problem can exist when using any API that relies on this definition of invocability. If it's something we want to prevent, a compiler plugin does sound like a better approach to me.

Incidentally, std::is_invocable is on our list of allowed features:

https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-features.md#std_is_invocable-allowed

By the way, std::function_ref (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p0792r10.html) looks like it wouldn't become part of the standard until C++26 at the earliest (https://github.com/cplusplus/papers/issues/256).

[dan...@chromium.org]

On Tue, Jul 19, 2022 at 2:02 PM K. Moon <km...@chromium.org> wrote:

This behavior seems to trace back to the C++ standard's definition of invocability (such as https://en.cppreference.com/w/cpp/types/is_invocable), which explicitly allows the conversion of any return type to void. I can see this being useful in some cases, but in any case, I think it might be unfair to single out FunctionRef for this concern, as it seems like the same problem can exist when using any API that relies on this definition of invocability. If it's something we want to prevent, a compiler plugin does sound like a better approach to me.

I personally am less a fan of deferring to a stdlib type_trait for what is good for our code. A library author can require the return type to be std::same_as the type written in the FunctionRef, it's their choice. If anything, the stdlib is leaving it up to the library author, not giving a strong opinion (in my opinion). We can take a valid position that we don't want such implicit conversions. We also require explicit on constructors even though C++ does not require (and _defaults_) to allowing implicit conversion/construction.

[K. Moon]

I'm not opposed to that, I just think it's not a problem localized to FunctionRef.

Implicit conversions I would consider more of a historical design mistake that you can never get rid of now. (Converting everything to void may fall under the same category, of course.)

[Daniel Cheng]

Based on the updated discussions, absl::FunctionRef has been moved to the banned section in favor of the stricter base::FunctionRef: https://chromium-review.googlesource.com/c/chromium/src/+/3768723

If anyone encounters use cases where the implicit conversion would have been valuable, please do raise that here: the idea is to disallow surprising implicit conversions, but if there are instances where disallowing implicit conversions requires excessive boilerplate, that would be a useful data point for allowing some degree of implicit conversions.

Daniel