Fix handler insertion in CefMessageRouterBrowserSideImpl (fixes #3586)

libcef_dll/wrapper/cef_message_router.cc

@@ -198,9 +198,9 @@ class CefMessageRouterBrowserSideImpl : public CefMessageRouterBrowserSide {
 
   bool AddHandler(Handler* handler, bool first) override {
     CEF_REQUIRE_UI_THREAD();
-    if (handler_set_.find(handler) == handler_set_.end()) {
+    if (std::find(handlers_.begin(), handlers_.end(), handler) ==
-      handler_set_.insert(first ? handler_set_.begin() : handler_set_.end(),
+        handlers_.end()) {
-                          handler);
+      handlers_.insert(first ? handlers_.begin() : handlers_.end(), handler);
       return true;
     }
     return false;
@@ -208,7 +208,9 @@ class CefMessageRouterBrowserSideImpl : public CefMessageRouterBrowserSide {
 
   bool RemoveHandler(Handler* handler) override {
     CEF_REQUIRE_UI_THREAD();
-    if (handler_set_.erase(handler) > 0) {
+    auto it = std::find(handlers_.begin(), handlers_.end(), handler);
+    if (it != handlers_.end()) {
+      handlers_.erase(it);
       CancelPendingFor(nullptr, handler, true);
       return true;
     }
@@ -297,7 +299,7 @@ class CefMessageRouterBrowserSideImpl : public CefMessageRouterBrowserSide {
       const int request_id = content.request_id;
       const bool persistent = content.is_persistent;
 
-      if (handler_set_.empty()) {
+      if (handlers_.empty()) {
         // No handlers so cancel the query.
         CancelUnhandledQuery(browser, frame, context_id, request_id);
         return true;
@@ -312,7 +314,7 @@ class CefMessageRouterBrowserSideImpl : public CefMessageRouterBrowserSide {
 
       // Make a copy of the handler list in case the user adds or removes a
       // handler while we're iterating.
-      const HandlerSet handlers = handler_set_;
+      const HandlerSet handlers = handlers_;
 
       Handler* handler = std::visit(
           [&](const auto& arg) -> CefMessageRouterBrowserSide::Handler* {
@@ -615,8 +617,8 @@ class CefMessageRouterBrowserSideImpl : public CefMessageRouterBrowserSide {
 
   // Set of currently registered handlers. An entry is added when a handler is
   // registered and removed when a handler is unregistered.
-  using HandlerSet = std::set<Handler*>;
+  using HandlerSet = std::vector<Handler*>;
-  HandlerSet handler_set_;
+  HandlerSet handlers_;
 
   // Map of query ID to QueryInfo instance. An entry is added when a Handler
   // indicates that it will handle the query and removed when either the query

tests/ceftests/message_router_multi_query_unittest.cc

@@ -1164,35 +1164,65 @@ class MultiQueryMultiHandlerTestHandler : public SingleLoadTestHandler,
                  const CefString& request,
                  bool persistent,
                  CefRefPtr<Callback> callback) override {
-      // Each handler only handles a single request.
+      // We expect handlers to be called in order 0, 1, 2.
-      std::stringstream ss;
-      ss << kMultiQueryRequest << ":" << index_;
-      const std::string& handled_request = ss.str();
-      if (request != handled_request) {
-        return false;
-      }
 
-      // Verify that handlers are called in the correct order.
+      // The handler0 is called 3 times:
+      // - 1st request = "MultiQueryRequest:0", returns true, preventing calls
+      //   to the other handlers.
+      // - 2nd request = "MultiQueryRequest:1", returns false
+      // - 3rd request = "MultiQueryRequest:2", returns false
       if (index_ == 0) {
+        if (request == HandledRequest(0)) {
           EXPECT_FALSE(test_handler_->got_query0_);
           EXPECT_FALSE(test_handler_->got_query1_);
           EXPECT_FALSE(test_handler_->got_query2_);
 
           test_handler_->got_query0_.yes();
-      } else if (index_ == 1) {
+        } else if (request == HandledRequest(1)) {
+          EXPECT_TRUE(test_handler_->got_query0_);
+          EXPECT_FALSE(test_handler_->got_query1_);
+          EXPECT_FALSE(test_handler_->got_query2_);
+        } else {
+          EXPECT_EQ(request, HandledRequest(2));
+          EXPECT_TRUE(test_handler_->got_query0_);
+          EXPECT_TRUE(test_handler_->got_query1_);
+          EXPECT_FALSE(test_handler_->got_query2_);
+        }
+      }
+
+      // The handler1 is called 2 times:
+      // - 1st request = "MultiQueryRequest:1", returns true, preventing calls
+      //   to the handler2.
+      // - 2nd request = "MultiQueryRequest:2", returns false
+      if (index_ == 1) {
+        if (request == HandledRequest(1)) {
           EXPECT_TRUE(test_handler_->got_query0_);
           EXPECT_FALSE(test_handler_->got_query1_);
           EXPECT_FALSE(test_handler_->got_query2_);
 
           test_handler_->got_query1_.yes();
-      } else if (index_ == 2) {
+        } else {
+          EXPECT_EQ(request, HandledRequest(2));
           EXPECT_TRUE(test_handler_->got_query0_);
           EXPECT_TRUE(test_handler_->got_query1_);
           EXPECT_FALSE(test_handler_->got_query2_);
+        }
+      }
 
+      // The handler2 is called 1 time with request = "MultiQueryRequest:2".
+      if (index_ == 2) {
+        EXPECT_EQ(request, HandledRequest(2));
+        EXPECT_TRUE(test_handler_->got_query0_);
+        EXPECT_TRUE(test_handler_->got_query1_);
+        EXPECT_FALSE(test_handler_->got_query2_);
         test_handler_->got_query2_.yes();
       }
 
+      // Each handler only handles a single request.
+      if (request != HandledRequest(index_)) {
+        return false;
+      }
+
       query_id_ = query_id;
       return test_handler_->OnQuery(browser, frame, query_id, request,
                                     persistent, callback);
@@ -1219,6 +1249,12 @@ class MultiQueryMultiHandlerTestHandler : public SingleLoadTestHandler,
     }
 
    private:
+    static std::string HandledRequest(int index) {
+      std::stringstream ss;
+      ss << kMultiQueryRequest << ":" << index;
+      return ss.str();
+    }
+
     MultiQueryMultiHandlerTestHandler* test_handler_;
     const int index_;
     int64_t query_id_ = 0;
@@ -1227,9 +1263,9 @@ class MultiQueryMultiHandlerTestHandler : public SingleLoadTestHandler,
   MultiQueryMultiHandlerTestHandler(bool synchronous,
                                     bool cancel_by_removing_handler)
       : manager_(std::string(), synchronous, 0),
-        handler0_(this, 0),
-        handler1_(this, 1),
         handler2_(this, 2),
+        handler1_(this, 1),
+        handler0_(this, 0),
         cancel_by_removing_handler_(cancel_by_removing_handler) {
     manager_.AddObserver(this);
 
@@ -1347,8 +1383,7 @@ class MultiQueryMultiHandlerTestHandler : public SingleLoadTestHandler,
  protected:
   void AddHandlers(
       CefRefPtr<CefMessageRouterBrowserSide> message_router) override {
-    // OnQuery call order will verify that the first/last ordering works as
+    // OnQuery call order will verify that the ordering works as expected.
-    // expected.
     EXPECT_TRUE(message_router->AddHandler(&handler1_, true));
     EXPECT_TRUE(message_router->AddHandler(&handler0_, true));
     EXPECT_TRUE(message_router->AddHandler(&handler2_, false));
@@ -1359,9 +1394,9 @@ class MultiQueryMultiHandlerTestHandler : public SingleLoadTestHandler,
 
  private:
   MultiQueryManager manager_;
-  Handler handler0_;
-  Handler handler1_;
   Handler handler2_;
+  Handler handler1_;
+  Handler handler0_;
 
   const bool cancel_by_removing_handler_;