Parameter "first" is ignored in CefMessageRouterBrowserSide:AddHandler implementation

[nik-sp]

Description

In the current implementation of the AddHandler method within CefMessageRouterBrowserSideImpl, it appears that the bool first parameter is not having the intended effect due to the data structure used to store handlers.

Here is AddHandler description:

  ///
  /// Add a new query handler. If |first| is true it will be added as the first
  /// handler, otherwise it will be added as the last handler. Returns true if
  /// the handler is added successfully or false if the handler has already been
  /// added. Must be called on the browser process UI thread. The Handler object
  /// must either outlive the router or be removed before deletion.
  ///
  virtual bool AddHandler(Handler* handler, bool first) = 0;

Here is the AddHandler implementation:

bool AddHandler(Handler* handler, bool first) override {
  CEF_REQUIRE_UI_THREAD();
  if (handler_set_.find(handler) == handler_set_.end()) {
    handler_set_.insert(first ? handler_set_.begin() : handler_set_.end(), handler);
    return true;
  }
  return false;
}

The issue arises from the fact that handler_set_ is declared as a std::set<Handler*>. In a std::set, elements are sorted and ordered by the values they point to, not by the order of insertion or any custom criteria like bool first would suggest.

Unit tests coverage

It is important to note that there are existing unit tests that should cover this functionality. These unit tests can be found in the CEF source code:

  void AddHandlers(
      CefRefPtr<CefMessageRouterBrowserSide> message_router) override {
    // OnQuery call order will verify that the first/last ordering works as
    // expected.
    EXPECT_TRUE(message_router->AddHandler(&handler1_, true));
    EXPECT_TRUE(message_router->AddHandler(&handler0_, true));
    EXPECT_TRUE(message_router->AddHandler(&handler2_, false));

    // Can't add the same handler multiple times.
    EXPECT_FALSE(message_router->AddHandler(&handler1_, true));
  }

The unit tests verify the functionality of AddHandler, but they currently work correctly because the pointers to handlers have increasing values. This approach is not a reliable way to ensure the expected ordering behavior and does not follow the documented behavior of the AddHandler method.

Expected behavior

In the AddHandler method, when the bool first parameter is true, the handler should be inserted at the beginning of the set, and when false, it should be inserted at the end. However, due to the nature of std::set, this is not happening as expected.

Proposed Solution:

To address this issue, it may be necessary to change the underlying data structure used for storing handlers in the CefMessageRouterBrowserSide or introduce an alternative mechanism that can maintain the desired order of handlers based on the bool first parameter. Another approach could be to remove bool first from the API, or change API description to mention that parameter is ignored.

[magreenblatt]

To address this issue, it may be necessary to change the underlying data structure used for storing handlers

That sounds like the best option.