[WARP] Fix misc sidebar navigation / focus event bugs

- Fixed crash related to pending fetch callback for deleted view frame
- Fixed focus not being kept for tables in the sidebar, as well as focus in general
- Made matched function list navigation require a double click, to fix accidental navigation to other functions

Author: emesare

plugins/warp/ui/matched.cpp

@@ -47,7 +47,7 @@ WarpMatchedWidget::WarpMatchedWidget(BinaryViewRef current)
 
 	Update();
 
-	connect(m_tableWidget->GetTableView(), &QTableView::clicked, this, [this](const QModelIndex& index) {
+	connect(m_tableWidget->GetTableView(), &QTableView::doubleClicked, this, [this](const QModelIndex& index) {
 		if (m_current == nullptr)
 			return;
 		if (!index.isValid())
@@ -60,6 +60,7 @@ WarpMatchedWidget::WarpMatchedWidget(BinaryViewRef current)
 			return;
 		// Navigate to the address in the view, so the user feels like they are doing something.
 		auto currentView = m_current->GetCurrentView();
+		// TODO: Navigate unconditionally steals focus, need to figure out how to prevent the loss of focus.
 		m_current->Navigate(currentView, selectedItem.value());
 	});
 }

plugins/warp/ui/plugin.cpp

@@ -174,23 +174,22 @@ WarpSidebarWidget::WarpSidebarWidget(BinaryViewRef data) : SidebarWidget("WARP")
 	// Do a full update if analysis has been done, otherwise we may persist old data and not have new data.
 	m_analysisEvent = new AnalysisCompletionEvent(m_data, [this]() { ExecuteOnMainThread([this]() { Update(); }); });
 
-	const std::shared_ptr<WarpFetcher> fetcher = WarpFetcher::Global();
-	fetcher->AddCompletionCallback([this]() {
-		Update();
+	m_fetcher = WarpFetcher::Global();
+	m_callbackId = m_fetcher->AddCompletionCallback([this]() {
+		ExecuteOnMainThread([this]() { Update(); });
 		return KeepCallback;
 	});
 
 	// NOTE: This fetcher is shared with the fetch dialog that is constructed on initialization of this plugin.
-	m_currentFunctionWidget->SetFetcher(fetcher);
+	m_currentFunctionWidget->SetFetcher(m_fetcher);
 }
 
 WarpSidebarWidget::~WarpSidebarWidget()
 {
 	m_analysisEvent->Cancel();
+	m_fetcher->RemoveCompletionCallback(m_callbackId);
 }
 
-void WarpSidebarWidget::focus() {}
-
 void WarpSidebarWidget::Update()
 {
 	m_currentFunctionWidget->UpdateMatches();

plugins/warp/ui/plugin.h

@@ -18,6 +18,9 @@ class WarpSidebarWidget : public SidebarWidget
 	QAction* m_matcherAction;
 	bool isMatcherRunning = false;
 
+	std::shared_ptr<WarpFetcher> m_fetcher;
+	WarpFetcher::CallbackId m_callbackId;
+
 	WarpCurrentFunctionWidget* m_currentFunctionWidget;
 	WarpMatchedWidget* m_matchedWidget;
 	WarpContainersPane* m_containerWidget;
@@ -29,8 +32,6 @@ class WarpSidebarWidget : public SidebarWidget
 
 	QWidget* headerWidget() override { return m_headerWidget; }
 
-	void focus() override;
-
 	void Update();
 
 	void setMatcherActionIcon(bool running);

plugins/warp/ui/shared/fetcher.cpp

@@ -24,14 +24,20 @@ std::vector<FunctionRef> WarpFetcher::FlushPendingFunctions()
 
 void WarpFetcher::ExecuteCompletionCallback()
 {
-	BinaryNinja::ExecuteOnMainThread([this]() {
-		// TODO: Holding the mutex here is dangerous!
+	std::vector<std::pair<CallbackId, CompletionCallback>> callbacks;
+	{
 		std::lock_guard<std::mutex> lock(m_requestMutex);
-		m_completionCallbacks.erase(
-			std::ranges::remove_if(m_completionCallbacks, [](const auto& cb) { return cb() == RemoveCallback; })
-				.begin(),
-			m_completionCallbacks.end());
-	});
+		callbacks.insert(callbacks.end(), m_completionCallbacks.begin(), m_completionCallbacks.end());
+	}
+
+	std::vector<CallbackId> toRemove = {};
+	for (auto& [id, cb] : callbacks)
+		if (cb() == RemoveCallback)
+			toRemove.push_back(id);
+
+	std::lock_guard<std::mutex> lock(m_requestMutex);
+	for (auto id : toRemove)
+		m_completionCallbacks.erase(id);
 }
 
 std::shared_ptr<WarpFetcher> WarpFetcher::Global()

plugins/warp/ui/shared/fetcher.h

@@ -24,22 +24,29 @@ class WarpFetcher
 	std::mutex m_requestMutex;
 	std::vector<FunctionRef> m_pendingRequests;
 	std::unordered_set<Warp::FunctionGUID> m_processedGuids;
-
-	// List of callbacks to call when done fetching data, assume that others are using this as well.
-	std::vector<std::function<WarpFetchCompletionStatus()>> m_completionCallbacks;
-
 public:
+	using CallbackId = uint64_t;
+	using CompletionCallback = std::function<WarpFetchCompletionStatus()>;
+
 	explicit WarpFetcher();
 
 	// The global fetcher instance, this is used for the fetch dialog and the sidebar.
 	static std::shared_ptr<WarpFetcher> Global();
 
 	std::atomic<bool> m_requestInProgress = false;
 
-	void AddCompletionCallback(std::function<WarpFetchCompletionStatus()> cb)
+	[[nodiscard]] CallbackId AddCompletionCallback(CompletionCallback cb)
 	{
 		std::lock_guard<std::mutex> lock(m_requestMutex);
-		m_completionCallbacks.push_back(std::move(cb));
+		const CallbackId id = m_nextCallbackId++;
+		m_completionCallbacks.emplace(id, std::move(cb));
+		return id;
+	}
+
+	void RemoveCompletionCallback(CallbackId id)
+	{
+		std::lock_guard<std::mutex> lock(m_requestMutex);
+		m_completionCallbacks.erase(id);
 	}
 
 	void AddPendingFunction(const FunctionRef& func);
@@ -49,7 +56,10 @@ class WarpFetcher
 	void ClearProcessed();
 
 private:
-	std::vector<FunctionRef> FlushPendingFunctions();
-
+	// List of callbacks to call when done fetching data, assume that others are using this as well.
+	std::atomic<CallbackId> m_nextCallbackId = 1;
+	std::unordered_map<CallbackId, CompletionCallback> m_completionCallbacks;
 	void ExecuteCompletionCallback();
+
+	std::vector<FunctionRef> FlushPendingFunctions();
 };