Is it common to have mem bad access crashes because of `base::internal::BindStateHolder::polymorphic_invoke() const`?

[Vincent Boisselle]

Hi,

We've seen some browser crashes on iOS because the `bind_state_` object held by OnceCallback is deallocated (at least this is what we think) when calling .Run() (happens here). Seems to happen "rarely" though and the code branch is frequently hit as it is called at least once for every page load.

Context: We are currently working on a feature that is queuing OnceCallback objects in a vector to run periodic batches of queued tasks (see code). A task corresponds to a OnceCallback instance.

Moving over the `holder_` to a local holder (here) seems to work, so I assume that the OnceCallback object itself is allocated. Otherwise if the callback obj was deallocated I would assume that the crash would happen at that moment, before using the `bind_state_` object.

The only possibility I see is that the once callback is run more than once but I don't see how this would be possible as we clear the vector of callbacks after it is iterated.

Maybe there is something wrong in the loop below that is used to run the queue
```
  for (auto& completion : fetch_requests_) {
    std::move(completion).Run(forms);
  }
```
fetch_requests_ is the std::vector of OnceCallback

so, we iterate the callbacks using a reference , move them, run them, then clear the vector. Maybe there is something there that may trigger a rare condition, idk.

Any clue? Are those crashes expected to happen from time to time because of some sort of memory corruption?

Thx!

[Christian Biesinger]

If I had to guess, one of the callbacks is doing something that deallocates the vector.

That std::move wouldn't necessarily fail -- if the memory is still mapped, it will work, but actually accessing the pointer is now garbage.

Christian

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CANktkuFtG5b9aOONBpvbzM_nrRJ6%2B-Um%2BGLPCWNs6vwzojCjGw%40mail.gmail.com.

[Lambros Lambrou]

I think it depends on how much control you have over the callbacks?

Arbitrary callbacks could delete the "this" object, or the vector you are iterating over. Because you're using std::vector, just adding a new callback to it could cause a reallocation.

Maybe copy (or move) the vector into the local stack, then clear the vector, then run the callbacks?

```

  std::vector<...> requests(std::move(fetch_requests_));

  fetch_requests_.clear();

  for (auto& completion : requests) {
    std::move(completion).Run(forms);
  }
```

Or use base::CallbackList instead, which is designed to deal with this issue?

To view this discussion visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAPTJ0XEBgAt36KVDxyXDfMdcZf6Xmcc5q54LSH%3D8dm%2BitJ7Ppw%40mail.gmail.com.

[Vincent Boisselle]

that would do it, then we can push any request that was added while completing the callbacks to a follow-up batch

Wondering whether this is a good use case for https://source.chromium.org/chromium/chromium/src/+/main:base/callback_list.h

[Vincent Boisselle]

Or this might be also be a good use case for a base::queue