chromiumembedded / cef

Chromium Embedded Framework (CEF). A simple framework for embedding Chromium-based browsers in other applications.
https://bitbucket.org/chromiumembedded/cef/
Other
3.09k stars 450 forks source link

docs: Improve example of map::erase() #3669

Open Dejmas opened 3 months ago

Dejmas commented 3 months ago

There is a code with bag in General Usage wiki text It is iterator invalidation bug. When erase is called on map, iterator is invalidated. It is located in GeneralUsage.md -> Custom Implementation -> example in point number 7.

Link https://bitbucket.org/chromiumembedded/cef/wiki/GeneralUsage.md#markdown-header-custom-implementation The text introducing the example.

7. Release any V8 references associated with the context in CefRenderProcessHandler::OnContextReleased().

original code example

void MyHandler::OnContextReleased(CefRefPtr<CefBrowser> browser,
                                  CefRefPtr<CefFrame> frame,
                                  CefRefPtr<CefV8Context> context) {
  // Remove any JavaScript callbacks registered for the context that has been released.
  if (!callback_map_.empty()) {
    CallbackMap::iterator it = callback_map_.begin();
    for (; it != callback_map_.end();) {
      if (it->second.first->IsSame(context))
        callback_map_.erase(it++); // <-- The iterator will be invalidated here. The rest of loop is undefined behavioural.
      else
        ++it;
    }
  }
}

I suggest changing.

void MyHandler::OnContextReleased(CefRefPtr<CefBrowser> browser,
                                  CefRefPtr<CefFrame> frame,
                                  CefRefPtr<CefV8Context> context) {
  // Remove any JavaScript callbacks registered for the context that has been released.
  if (!callback_map_.empty()) {
    std::set<CallbackMap::iterator> toDelete;
    CallbackMap::iterator it = callback_map_.begin();
    for (; it != callback_map_.end(); ++ it) {
      if (it->second.first->IsSame(context))
        toDelete.insert(it);
    }
    for (auto & it : toDelete) {
        callback_map_.erase(it);
    }
  }
}

Also, it can look simpler.

void MyHandler::OnContextReleased(CefRefPtr<CefBrowser> browser,
                                  CefRefPtr<CefFrame> frame,
                                  CefRefPtr<CefV8Context> context) {
    // Remove any JavaScript callbacks registered for the context that has been released.
    std::set<CallbackMap::key_type> toDelete;
    for (const auto & item : callback_map_) {
        if (item.second.first->IsSame(context))
            toDelete.insert(item.first);
    }
    for (const auto & key : toDelete) {
        callback_map_.erase(key);
    }
}
magreenblatt commented 3 months ago

callbackmap.erase(it++); // <-- The iterator will be invalidated here. The rest of loop is undefined behavioural.

The current code is correct, but it could be more clearly written as: it = callback_map_.erase(it);

See https://stackoverflow.com/a/2874533