From 2f020ed28c8a68f581ace09c54573ffb998fdac5 Mon Sep 17 00:00:00 2001 From: Adam Date: Sat, 21 Oct 2023 03:51:38 +0100 Subject: [PATCH] browser,file_manager: rework file manager handling The previous design had a memory leak which couldn't be fixed, amongst other issues. --- src/browser.hxx | 3 +++ src/browser/client.cxx | 7 +++++-- src/browser/client.hxx | 10 +++------- src/browser/resource_handler.cxx | 9 +++++++++ src/browser/resource_handler.hxx | 13 +++++++++++-- src/browser/window_launcher.cxx | 10 ++++++++-- src/browser/window_launcher.hxx | 6 ++++-- src/file_manager.hxx | 3 ++- 8 files changed, 45 insertions(+), 16 deletions(-) diff --git a/src/browser.hxx b/src/browser.hxx index bd09939..dee206f 100644 --- a/src/browser.hxx +++ b/src/browser.hxx @@ -1,6 +1,8 @@ #ifndef _BOLT_BROWSER_HXX_ #define _BOLT_BROWSER_HXX_ #include "browser/common.hxx" +#include "file_manager.hxx" + #include "include/cef_client.h" #include "include/internal/cef_ptr.h" #include "include/views/cef_window.h" @@ -93,6 +95,7 @@ namespace Browser { CefRefPtr window; CefRefPtr browser_view; CefRefPtr browser; + CefRefPtr file_manager; CefRefPtr pending_child; std::vector> children; CefPopupFeatures popup_features; diff --git a/src/browser/client.cxx b/src/browser/client.cxx index ae6328e..ab11290 100644 --- a/src/browser/client.cxx +++ b/src/browser/client.cxx @@ -29,6 +29,9 @@ constexpr Browser::Details LAUNCHER_DETAILS = { }; Browser::Client::Client(CefRefPtr app,std::filesystem::path config_dir, std::filesystem::path data_dir): +#if defined(BOLT_DEV_LAUNCHER_DIRECTORY) + CLIENT_FILEHANDLER(BOLT_DEV_LAUNCHER_DIRECTORY), +#endif is_closing(false), show_devtools(SHOW_DEVTOOLS), config_dir(config_dir), data_dir(data_dir) { app->SetBrowserProcessHandler(this); @@ -42,7 +45,7 @@ void Browser::Client::OpenLauncher() { std::lock_guard _(this->windows_lock); auto it = std::find_if(this->windows.begin(), this->windows.end(), [](CefRefPtr& win) { return win->IsLauncher(); }); if (it == this->windows.end()) { - CefRefPtr w = new Launcher(this, LAUNCHER_DETAILS, this->show_devtools, &this->file_manager, this->config_dir, this->data_dir); + CefRefPtr w = new Launcher(this, LAUNCHER_DETAILS, this->show_devtools, this, this->config_dir, this->data_dir); this->windows.push_back(w); } else { (*it)->Focus(); @@ -97,7 +100,7 @@ void Browser::Client::OnContextInitialized() { // After main() enters its event loop, this function will be called on the main thread when CEF // context is ready to go, so, as suggested by CEF examples, Bolt treats this as an entry point. std::lock_guard _(this->windows_lock); - CefRefPtr w = new Launcher(this, LAUNCHER_DETAILS, this->show_devtools, &this->file_manager, this->config_dir, this->data_dir); + CefRefPtr w = new Launcher(this, LAUNCHER_DETAILS, this->show_devtools, this, this->config_dir, this->data_dir); this->windows.push_back(w); } diff --git a/src/browser/client.hxx b/src/browser/client.hxx index be520ec..83ff825 100644 --- a/src/browser/client.hxx +++ b/src/browser/client.hxx @@ -18,8 +18,10 @@ #if defined(BOLT_DEV_LAUNCHER_DIRECTORY) #include "../file_manager/directory.hxx" +typedef FileManager::Directory CLIENT_FILEHANDLER; #else #include "../file_manager/launcher.hxx" +typedef FileManager::Launcher CLIENT_FILEHANDLER; #endif namespace Browser { @@ -28,7 +30,7 @@ namespace Browser { /// https://github.com/chromiumembedded/cef/blob/5735/include/cef_browser_process_handler.h /// https://github.com/chromiumembedded/cef/blob/5735/include/cef_life_span_handler.h /// https://github.com/chromiumembedded/cef/blob/5735/include/cef_request_handler.h - struct Client: public CefClient, CefBrowserProcessHandler, CefLifeSpanHandler, CefRequestHandler { + struct Client: public CefClient, CefBrowserProcessHandler, CefLifeSpanHandler, CefRequestHandler, CLIENT_FILEHANDLER { Client(CefRefPtr, std::filesystem::path config_dir, std::filesystem::path data_dir); /// Either opens a launcher window, or focuses an existing one. No more than one launcher window @@ -98,12 +100,6 @@ namespace Browser { std::filesystem::path config_dir; std::filesystem::path data_dir; -#if defined(BOLT_DEV_LAUNCHER_DIRECTORY) - FileManager::Directory file_manager = FileManager::Directory(BOLT_DEV_LAUNCHER_DIRECTORY); -#else - FileManager::Launcher file_manager; -#endif - #if defined(CEF_X11) xcb_connection_t* xcb; #endif diff --git a/src/browser/resource_handler.cxx b/src/browser/resource_handler.cxx index 50b7d63..d3fa5cc 100644 --- a/src/browser/resource_handler.cxx +++ b/src/browser/resource_handler.cxx @@ -31,6 +31,7 @@ bool Browser::ResourceHandler::Read(void* data_out, int bytes_to_read, int& byte memcpy(data_out, this->data + this->cursor, bytes_read); this->cursor = this->data_len; } + if (this->cursor == this->data_len) this->finish(); return true; } @@ -49,8 +50,16 @@ bool Browser::ResourceHandler::Skip(int64 bytes_to_skip, int64& bytes_skipped, C void Browser::ResourceHandler::Cancel() { this->cursor = this->data_len; + this->finish(); } CefRefPtr Browser::ResourceHandler::GetResourceHandler(CefRefPtr, CefRefPtr, CefRefPtr) { return this; } + +void Browser::ResourceHandler::finish() { + if (this->file_manager) { + this->file_manager->free(FileManager::File { .contents = this->data, .size = this->data_len, .mime_type = this->mime }); + this->file_manager = nullptr; + } +} diff --git a/src/browser/resource_handler.hxx b/src/browser/resource_handler.hxx index 2f436d6..7aea6dd 100644 --- a/src/browser/resource_handler.hxx +++ b/src/browser/resource_handler.hxx @@ -1,6 +1,8 @@ #ifndef _BOLT_RESOURCE_HANDLER_HXX_ #define _BOLT_RESOURCE_HANDLER_HXX_ +#include "../file_manager.hxx" + #include "include/cef_base.h" #include "include/cef_request_handler.h" @@ -10,9 +12,13 @@ namespace Browser { /// https://github.com/chromiumembedded/cef/blob/5735/include/cef_resource_handler.h struct ResourceHandler: public CefResourceRequestHandler, CefResourceHandler { ResourceHandler(const unsigned char* data, size_t len, int status, CefString mime): - data(data), data_len(len), status(status), mime(mime), has_location(false), cursor(0) { } + data(data), data_len(len), status(status), mime(mime), has_location(false), cursor(0), file_manager(nullptr) { } ResourceHandler(const unsigned char* data, size_t len, int status, CefString mime, CefString location): - data(data), data_len(len), status(status), mime(mime), location(location), has_location(true), cursor(0) { } + data(data), data_len(len), status(status), mime(mime), location(location), has_location(true), cursor(0), file_manager(nullptr) { } + + /// This constructor assumes the file does exist i.e. all params are initialised, and status will be 200 + ResourceHandler(FileManager::File file, CefRefPtr file_manager): + ResourceHandler(file.contents, file.size, 200, file.mime_type) { this->file_manager = file_manager; } bool Open(CefRefPtr, bool&, CefRefPtr) override; void GetResponseHeaders(CefRefPtr, int64&, CefString&) override; @@ -24,6 +30,8 @@ namespace Browser { protected: ResourceHandler(CefString mime): cursor(0), has_location(false), mime(mime) { } + void finish(); + const unsigned char* data; size_t data_len; int status; @@ -31,6 +39,7 @@ namespace Browser { CefString location; bool has_location; size_t cursor; + CefRefPtr file_manager; IMPLEMENT_REFCOUNTING(ResourceHandler); DISALLOW_COPY_AND_ASSIGN(ResourceHandler); }; diff --git a/src/browser/window_launcher.cxx b/src/browser/window_launcher.cxx index fd02764..ac98e70 100644 --- a/src/browser/window_launcher.cxx +++ b/src/browser/window_launcher.cxx @@ -76,7 +76,7 @@ Browser::Launcher::Launcher( CefRefPtr client, Details details, bool show_devtools, - const FileManager::FileManager* const file_manager, + CefRefPtr file_manager, std::filesystem::path config_dir, std::filesystem::path data_dir ): Window(client, details, show_devtools), data_dir(data_dir), file_manager(file_manager) { @@ -293,8 +293,9 @@ CefRefPtr Browser::Launcher::GetResourceRequestHandle // respond using internal hashmap of filenames FileManager::File file = this->file_manager->get(path); if (file.contents) { - return new ResourceHandler(file.contents, file.size, 200, file.mime_type); + return new ResourceHandler(file, this->file_manager); } else { + this->file_manager->free(file); const char* data = "Not Found\n"; return new ResourceHandler(reinterpret_cast(data), strlen(data), 404, "text/plain"); } @@ -303,6 +304,11 @@ CefRefPtr Browser::Launcher::GetResourceRequestHandle return nullptr; } +void Browser::Launcher::OnBrowserDestroyed(CefRefPtr view, CefRefPtr browser) { + Window::OnBrowserDestroyed(view, browser); + this->file_manager = nullptr; +} + CefRefPtr SaveFileFromPost(CefRefPtr request, const std::filesystem::path::value_type* path) { CefRefPtr post_data = request->GetPostData(); if (post_data->GetElementCount() != 1) { diff --git a/src/browser/window_launcher.hxx b/src/browser/window_launcher.hxx index a1d6f37..4cf8563 100644 --- a/src/browser/window_launcher.hxx +++ b/src/browser/window_launcher.hxx @@ -10,7 +10,7 @@ namespace Browser { struct Launcher: public Window { - Launcher(CefRefPtr, Details, bool, const FileManager::FileManager* const, std::filesystem::path, std::filesystem::path); + Launcher(CefRefPtr, Details, bool, CefRefPtr, std::filesystem::path, std::filesystem::path); bool IsLauncher() const override; @@ -24,6 +24,8 @@ namespace Browser { bool& ) override; + void OnBrowserDestroyed(CefRefPtr, CefRefPtr) override; + /// Attempts to open the given URL externally in the user's browser void OpenExternalUrl(char* url) const; @@ -37,7 +39,7 @@ namespace Browser { private: const std::string internal_url = "https://bolt-internal/"; - const FileManager::FileManager* file_manager; + CefRefPtr file_manager; std::filesystem::path data_dir; std::filesystem::path creds_path; std::filesystem::path config_path; diff --git a/src/file_manager.hxx b/src/file_manager.hxx index e60b9c6..9e9d5b5 100644 --- a/src/file_manager.hxx +++ b/src/file_manager.hxx @@ -1,5 +1,6 @@ #ifndef _BOLT_FILE_MANAGER_HXX_ #define _BOLT_FILE_MANAGER_HXX_ +#include "include/cef_base.h" #include "include/internal/cef_string.h" #include @@ -16,7 +17,7 @@ namespace FileManager { CefString mime_type; }; - class FileManager { + class FileManager: public CefBaseRefCounted { public: /// Fetches a file, as if from a web server, by its abolsute path. /// For example, in the url "https://adamcake.com/index.html", the path would be "/index.html".