From 6fd03425b20b4281ab635aa3b284b7f53cad2cd9 Mon Sep 17 00:00:00 2001 From: Gingeh <39150378+Gingeh@users.noreply.github.com> Date: Fri, 10 Jan 2025 20:33:27 +1100 Subject: [PATCH] UI: Prevent crash when right clicking on an unloaded image --- Libraries/LibWeb/Page/EventHandler.cpp | 6 +++++- Libraries/LibWeb/Page/Page.h | 2 +- Libraries/LibWebView/ViewImplementation.h | 2 +- Libraries/LibWebView/WebContentClient.cpp | 2 +- Libraries/LibWebView/WebContentClient.h | 2 +- Services/WebContent/PageClient.cpp | 7 +++++-- Services/WebContent/PageClient.h | 2 +- Services/WebContent/WebContentClient.ipc | 2 +- UI/AppKit/Interface/LadybirdWebView.mm | 19 +++++++++++++++---- UI/Qt/Tab.cpp | 16 ++++++++++------ UI/Qt/Tab.h | 3 ++- 11 files changed, 43 insertions(+), 20 deletions(-) diff --git a/Libraries/LibWeb/Page/EventHandler.cpp b/Libraries/LibWeb/Page/EventHandler.cpp index 519dfb2197d9..710276f3dd1e 100644 --- a/Libraries/LibWeb/Page/EventHandler.cpp +++ b/Libraries/LibWeb/Page/EventHandler.cpp @@ -508,7 +508,11 @@ EventResult EventHandler::handle_mouseup(CSSPixelPoint viewport_position, CSSPix if (is(*node)) { auto& image_element = verify_cast(*node); auto image_url = image_element.document().encoding_parse_url(image_element.src()); - m_navigable->page().client().page_did_request_image_context_menu(viewport_position, image_url, "", modifiers, image_element.immutable_bitmap()->bitmap()); + Optional bitmap; + if (image_element.immutable_bitmap()) + bitmap = image_element.immutable_bitmap()->bitmap(); + + m_navigable->page().client().page_did_request_image_context_menu(viewport_position, image_url, "", modifiers, bitmap); } else if (is(*node)) { auto& media_element = verify_cast(*node); diff --git a/Libraries/LibWeb/Page/Page.h b/Libraries/LibWeb/Page/Page.h index 9f3a510ac87d..48540e853181 100644 --- a/Libraries/LibWeb/Page/Page.h +++ b/Libraries/LibWeb/Page/Page.h @@ -339,7 +339,7 @@ class PageClient : public JS::Cell { virtual void page_did_request_cursor_change(Gfx::StandardCursor) { } virtual void page_did_request_context_menu(CSSPixelPoint) { } virtual void page_did_request_link_context_menu(CSSPixelPoint, URL::URL const&, [[maybe_unused]] ByteString const& target, [[maybe_unused]] unsigned modifiers) { } - virtual void page_did_request_image_context_menu(CSSPixelPoint, URL::URL const&, [[maybe_unused]] ByteString const& target, [[maybe_unused]] unsigned modifiers, Gfx::Bitmap const*) { } + virtual void page_did_request_image_context_menu(CSSPixelPoint, URL::URL const&, [[maybe_unused]] ByteString const& target, [[maybe_unused]] unsigned modifiers, Optional) { } virtual void page_did_request_media_context_menu(CSSPixelPoint, [[maybe_unused]] ByteString const& target, [[maybe_unused]] unsigned modifiers, Page::MediaContextMenu) { } virtual void page_did_click_link(URL::URL const&, [[maybe_unused]] ByteString const& target, [[maybe_unused]] unsigned modifiers) { } virtual void page_did_middle_click_link(URL::URL const&, [[maybe_unused]] ByteString const& target, [[maybe_unused]] unsigned modifiers) { } diff --git a/Libraries/LibWebView/ViewImplementation.h b/Libraries/LibWebView/ViewImplementation.h index 95f4c76030da..08581a29eb71 100644 --- a/Libraries/LibWebView/ViewImplementation.h +++ b/Libraries/LibWebView/ViewImplementation.h @@ -167,7 +167,7 @@ class ViewImplementation { Function on_close; Function on_context_menu_request; Function on_link_context_menu_request; - Function on_image_context_menu_request; + Function const&)> on_image_context_menu_request; Function on_media_context_menu_request; Function on_link_hover; Function on_link_unhover; diff --git a/Libraries/LibWebView/WebContentClient.cpp b/Libraries/LibWebView/WebContentClient.cpp index 2e2ec35ec409..19695eb47d00 100644 --- a/Libraries/LibWebView/WebContentClient.cpp +++ b/Libraries/LibWebView/WebContentClient.cpp @@ -234,7 +234,7 @@ void WebContentClient::did_request_link_context_menu(u64 page_id, Gfx::IntPoint } } -void WebContentClient::did_request_image_context_menu(u64 page_id, Gfx::IntPoint content_position, URL::URL const& url, ByteString const&, unsigned, Gfx::ShareableBitmap const& bitmap) +void WebContentClient::did_request_image_context_menu(u64 page_id, Gfx::IntPoint content_position, URL::URL const& url, ByteString const&, unsigned, Optional const& bitmap) { if (auto view = view_for_page_id(page_id); view.has_value()) { if (view->on_image_context_menu_request) diff --git a/Libraries/LibWebView/WebContentClient.h b/Libraries/LibWebView/WebContentClient.h index 68d07ac4398f..7983dea4b9fb 100644 --- a/Libraries/LibWebView/WebContentClient.h +++ b/Libraries/LibWebView/WebContentClient.h @@ -66,7 +66,7 @@ class WebContentClient final virtual void did_start_loading(u64 page_id, URL::URL const&, bool) override; virtual void did_request_context_menu(u64 page_id, Gfx::IntPoint) override; virtual void did_request_link_context_menu(u64 page_id, Gfx::IntPoint, URL::URL const&, ByteString const&, unsigned) override; - virtual void did_request_image_context_menu(u64 page_id, Gfx::IntPoint, URL::URL const&, ByteString const&, unsigned, Gfx::ShareableBitmap const&) override; + virtual void did_request_image_context_menu(u64 page_id, Gfx::IntPoint, URL::URL const&, ByteString const&, unsigned, Optional const&) override; virtual void did_request_media_context_menu(u64 page_id, Gfx::IntPoint, ByteString const&, unsigned, Web::Page::MediaContextMenu const&) override; virtual void did_get_source(u64 page_id, URL::URL const&, URL::URL const&, String const&) override; virtual void did_inspect_dom_tree(u64 page_id, ByteString const&) override; diff --git a/Services/WebContent/PageClient.cpp b/Services/WebContent/PageClient.cpp index 8e78e181bb10..2a76f117ab20 100644 --- a/Services/WebContent/PageClient.cpp +++ b/Services/WebContent/PageClient.cpp @@ -381,9 +381,12 @@ void PageClient::page_did_request_link_context_menu(Web::CSSPixelPoint content_p client().async_did_request_link_context_menu(m_id, page().css_to_device_point(content_position).to_type(), url, target, modifiers); } -void PageClient::page_did_request_image_context_menu(Web::CSSPixelPoint content_position, URL::URL const& url, ByteString const& target, unsigned modifiers, Gfx::Bitmap const* bitmap_pointer) +void PageClient::page_did_request_image_context_menu(Web::CSSPixelPoint content_position, URL::URL const& url, ByteString const& target, unsigned modifiers, Optional bitmap_pointer) { - auto bitmap = bitmap_pointer ? bitmap_pointer->to_shareable_bitmap() : Gfx::ShareableBitmap(); + Optional bitmap; + if (bitmap_pointer.has_value() && bitmap_pointer.value()) + bitmap = bitmap_pointer.value()->to_shareable_bitmap(); + client().async_did_request_image_context_menu(m_id, page().css_to_device_point(content_position).to_type(), url, target, modifiers, bitmap); } diff --git a/Services/WebContent/PageClient.h b/Services/WebContent/PageClient.h index f8457b6f3f85..f45b6c8c7f80 100644 --- a/Services/WebContent/PageClient.h +++ b/Services/WebContent/PageClient.h @@ -129,7 +129,7 @@ class PageClient final : public Web::PageClient { virtual void page_did_middle_click_link(URL::URL const&, ByteString const& target, unsigned modifiers) override; virtual void page_did_request_context_menu(Web::CSSPixelPoint) override; virtual void page_did_request_link_context_menu(Web::CSSPixelPoint, URL::URL const&, ByteString const& target, unsigned modifiers) override; - virtual void page_did_request_image_context_menu(Web::CSSPixelPoint, URL::URL const&, ByteString const& target, unsigned modifiers, Gfx::Bitmap const*) override; + virtual void page_did_request_image_context_menu(Web::CSSPixelPoint, URL::URL const&, ByteString const& target, unsigned modifiers, Optional) override; virtual void page_did_request_media_context_menu(Web::CSSPixelPoint, ByteString const& target, unsigned modifiers, Web::Page::MediaContextMenu) override; virtual void page_did_start_loading(URL::URL const&, bool) override; virtual void page_did_create_new_document(Web::DOM::Document&) override; diff --git a/Services/WebContent/WebContentClient.ipc b/Services/WebContent/WebContentClient.ipc index 6bc2ecfc736b..ffd1eb0a3ac8 100644 --- a/Services/WebContent/WebContentClient.ipc +++ b/Services/WebContent/WebContentClient.ipc @@ -37,7 +37,7 @@ endpoint WebContentClient did_middle_click_link(u64 page_id, URL::URL url, ByteString target, unsigned modifiers) =| did_request_context_menu(u64 page_id, Gfx::IntPoint content_position) =| did_request_link_context_menu(u64 page_id, Gfx::IntPoint content_position, URL::URL url, ByteString target, unsigned modifiers) =| - did_request_image_context_menu(u64 page_id, Gfx::IntPoint content_position, URL::URL url, ByteString target, unsigned modifiers, Gfx::ShareableBitmap bitmap) =| + did_request_image_context_menu(u64 page_id, Gfx::IntPoint content_position, URL::URL url, ByteString target, unsigned modifiers, Optional bitmap) =| did_request_media_context_menu(u64 page_id, Gfx::IntPoint content_position, ByteString target, unsigned modifiers, Web::Page::MediaContextMenu menu) =| did_request_alert(u64 page_id, String message) =| did_request_confirm(u64 page_id, String message) =| diff --git a/UI/AppKit/Interface/LadybirdWebView.mm b/UI/AppKit/Interface/LadybirdWebView.mm index 055a4c42ce39..9d13f8944898 100644 --- a/UI/AppKit/Interface/LadybirdWebView.mm +++ b/UI/AppKit/Interface/LadybirdWebView.mm @@ -33,6 +33,7 @@ static constexpr NSInteger CONTEXT_MENU_LOOP_TAG = 4; static constexpr NSInteger CONTEXT_MENU_SEARCH_SELECTED_TEXT_TAG = 5; static constexpr NSInteger CONTEXT_MENU_COPY_LINK_TAG = 6; +static constexpr NSInteger CONTEXT_MENU_COPY_IMAGE_TAG = 7; // Calls to [NSCursor hide] and [NSCursor unhide] must be balanced. We use this struct to ensure // we only call [NSCursor hide] once and to ensure that we do call [NSCursor unhide]. @@ -54,7 +55,7 @@ @interface LadybirdWebView () OwnPtr m_web_view_bridge; URL::URL m_context_menu_url; - Gfx::ShareableBitmap m_context_menu_bitmap; + Optional m_context_menu_bitmap; Optional m_context_menu_search_text; Optional m_hidden_cursor; @@ -669,6 +670,9 @@ - (void)setWebViewCallbacks TemporaryChange change_url { m_context_menu_url, url }; TemporaryChange change_bitmap { m_context_menu_bitmap, bitmap }; + auto* copy_image_menu_item = [self.image_context_menu itemWithTag:CONTEXT_MENU_COPY_IMAGE_TAG]; + [copy_image_menu_item setEnabled:bitmap.has_value()]; + auto* event = Ladybird::create_context_menu_mouse_event(self, position); [NSMenu popUpContextMenu:self.image_context_menu withEvent:event forView:self]; }; @@ -1186,7 +1190,10 @@ - (void)copyLink:(id)sender - (void)copyImage:(id)sender { - auto* bitmap = m_context_menu_bitmap.bitmap(); + if (!m_context_menu_bitmap.has_value()) { + return; + } + auto* bitmap = m_context_menu_bitmap.value().bitmap(); if (bitmap == nullptr) { return; } @@ -1310,6 +1317,7 @@ - (NSMenu*)image_context_menu { if (!_image_context_menu) { _image_context_menu = [[NSMenu alloc] initWithTitle:@"Image Context Menu"]; + [_image_context_menu setAutoenablesItems:NO]; [_image_context_menu addItem:[[NSMenuItem alloc] initWithTitle:@"Open Image" action:@selector(openLink:) @@ -1319,9 +1327,12 @@ - (NSMenu*)image_context_menu keyEquivalent:@""]]; [_image_context_menu addItem:[NSMenuItem separatorItem]]; - [_image_context_menu addItem:[[NSMenuItem alloc] initWithTitle:@"Copy Image" + auto* copy_image_menu_item = [[NSMenuItem alloc] initWithTitle:@"Copy Image" action:@selector(copyImage:) - keyEquivalent:@""]]; + keyEquivalent:@""]; + [copy_image_menu_item setTag:CONTEXT_MENU_COPY_IMAGE_TAG]; + [_image_context_menu addItem:copy_image_menu_item]; + [_image_context_menu addItem:[[NSMenuItem alloc] initWithTitle:@"Copy Image URL" action:@selector(copyLink:) keyEquivalent:@""]]; diff --git a/UI/Qt/Tab.cpp b/UI/Qt/Tab.cpp index f34397d57d77..a0d0a9a1e572 100644 --- a/UI/Qt/Tab.cpp +++ b/UI/Qt/Tab.cpp @@ -592,10 +592,13 @@ Tab::Tab(BrowserWindow* window, RefPtr parent_client, open_link_in_new_tab(m_image_context_menu_url); }); - auto* copy_image_action = new QAction("&Copy Image", this); - copy_image_action->setIcon(load_icon_from_uri("resource://icons/16x16/edit-copy.png"sv)); - QObject::connect(copy_image_action, &QAction::triggered, this, [this]() { - auto* bitmap = m_image_context_menu_bitmap.bitmap(); + m_image_context_menu_copy_image_action = new QAction("&Copy Image", this); + m_image_context_menu_copy_image_action->setIcon(load_icon_from_uri("resource://icons/16x16/edit-copy.png"sv)); + QObject::connect(m_image_context_menu_copy_image_action, &QAction::triggered, this, [this]() { + if (!m_image_context_menu_bitmap.has_value()) + return; + + auto* bitmap = m_image_context_menu_bitmap.value().bitmap(); if (bitmap == nullptr) return; @@ -621,14 +624,15 @@ Tab::Tab(BrowserWindow* window, RefPtr parent_client, m_image_context_menu->addAction(open_image_action); m_image_context_menu->addAction(open_image_in_new_tab_action); m_image_context_menu->addSeparator(); - m_image_context_menu->addAction(copy_image_action); + m_image_context_menu->addAction(m_image_context_menu_copy_image_action); m_image_context_menu->addAction(copy_image_url_action); m_image_context_menu->addSeparator(); m_image_context_menu->addAction(&m_window->inspect_dom_node_action()); - view().on_image_context_menu_request = [this](auto& image_url, Gfx::IntPoint content_position, Gfx::ShareableBitmap const& shareable_bitmap) { + view().on_image_context_menu_request = [this](auto& image_url, Gfx::IntPoint content_position, Optional const& shareable_bitmap) { m_image_context_menu_url = image_url; m_image_context_menu_bitmap = shareable_bitmap; + m_image_context_menu_copy_image_action->setEnabled(shareable_bitmap.has_value()); m_image_context_menu->exec(view().map_point_to_global_position(content_position)); }; diff --git a/UI/Qt/Tab.h b/UI/Qt/Tab.h index 6b286b50654c..5d3d14a92d0d 100644 --- a/UI/Qt/Tab.h +++ b/UI/Qt/Tab.h @@ -151,7 +151,8 @@ public slots: URL::URL m_link_context_menu_url; QMenu* m_image_context_menu { nullptr }; - Gfx::ShareableBitmap m_image_context_menu_bitmap; + QAction* m_image_context_menu_copy_image_action { nullptr }; + Optional m_image_context_menu_bitmap; URL::URL m_image_context_menu_url; QMenu* m_audio_context_menu { nullptr };