Skip to content

Commit

Permalink
[Nearby] Handle keyboard events in NearbyShareDialogUI
Browse files Browse the repository at this point in the history 
This moves the responsibility for handling keyboard events from
NearbyShareAction to NearbyShareDialogUI.

Only one instance of NearbyShareAction is created and reused by the
SharesheetService. We previously stored a reference to a WebView in
NearbyShareAction so that we could use its FocusManager in the handling
of keyboard events. But opening and closing multiple sharesheets in a
specific sequence could lead to a UAF as in crbug.com/1294097.

Shifting this responsibility to NearbyShareDialogUI allows multiple
sharesheets to exist safely at the same time.

Manually verified by testing various sequences with multiple sharesheets
on-device.

Bug: 1294097
Change-Id: I7c737057ee70e31e5221ef6ac6e97d47735db798
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3449337
Reviewed-by: Josh Nohle <nohle@chromium.org>
Commit-Queue: Michael Hansen <hansenmichael@google.com>
Cr-Commit-Position: refs/heads/main@{#969113}
  • Loading branch information
Michael Hansen authored and Chromium LUCI CQ committed Feb 9, 2022
1 parent d35f89e commit 2d9e826
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 51 deletions.
35 changes: 6 additions & 29 deletions chrome/browser/nearby_sharing/sharesheet/nearby_share_action.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/sharesheet/sharesheet_types.h"
#include "chrome/browser/ui/browser_navigator.h"
#include "chrome/browser/ui/browser_navigator_params.h"
#include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
#include "chrome/browser/ui/webui/nearby_share/nearby_share_dialog_ui.h"
#include "chrome/common/webui_url_constants.h"
#include "chrome/grit/generated_resources.h"
Expand Down Expand Up @@ -148,19 +145,18 @@ void NearbyShareAction::LaunchAction(
auto view = std::make_unique<views::WebView>(profile_);
// If this is not done, we don't see anything in our view.
view->SetPreferredSize(size);
web_view_ = root_view->AddChildView(std::move(view));
web_view_->GetWebContents()->SetDelegate(this);
views::WebView* web_view = root_view->AddChildView(std::move(view));
// TODO(vecore): Query this from the container view
web_view_->holder()->SetCornerRadii(gfx::RoundedCornersF(kCornerRadius));
web_view->holder()->SetCornerRadii(gfx::RoundedCornersF(kCornerRadius));

// load chrome://nearby into the webview
web_view_->LoadInitialURL(GURL(chrome::kChromeUINearbyShareURL));
web_view->LoadInitialURL(GURL(chrome::kChromeUINearbyShareURL));

// Without requesting focus, the sharesheet will launch in an unfocused state
// which raises accessibility issues with the "Device name" input.
web_view_->RequestFocus();
web_view->RequestFocus();

auto* webui = web_view_->GetWebContents()->GetWebUI();
auto* webui = web_view->GetWebContents()->GetWebUI();
DCHECK(webui != nullptr);

auto* nearby_ui =
Expand All @@ -170,6 +166,7 @@ void NearbyShareAction::LaunchAction(
nearby_ui->SetSharesheetController(controller);
nearby_ui->SetAttachments(
CreateAttachmentsFromIntent(profile_, std::move(intent)));
nearby_ui->SetWebView(web_view);
}

bool NearbyShareAction::HasActionView() {
Expand Down Expand Up @@ -250,23 +247,3 @@ void NearbyShareAction::SetActionCleanupCallbackForArc(
}
nearby_sharing_service->SetArcTransferCleanupCallback(std::move(callback));
}

bool NearbyShareAction::HandleKeyboardEvent(
content::WebContents* source,
const content::NativeWebKeyboardEvent& event) {
return unhandled_keyboard_event_handler_.HandleKeyboardEvent(
event, web_view_->GetFocusManager());
}

void NearbyShareAction::WebContentsCreated(
content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) {
chrome::ScopedTabbedBrowserDisplayer displayer(profile_);
NavigateParams nav_params(displayer.browser(), target_url,
ui::PageTransition::PAGE_TRANSITION_LINK);
Navigate(&nav_params);
}
24 changes: 3 additions & 21 deletions chrome/browser/nearby_sharing/sharesheet/nearby_share_action.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,12 @@

#include "chrome/browser/sharesheet/share_action/share_action.h"
#include "chrome/browser/ui/webui/nearby_share/nearby_share_dialog_ui.h"
#include "content/public/browser/web_contents_delegate.h"
#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"

class Profile;

namespace views {
class WebView;
} // namespace views

class NearbyShareAction : public sharesheet::ShareAction,
content::WebContentsDelegate {
// NearbyShareAction is responsible for loading the Nearby UI within the
// Sharesheet. A single instance is created on sign in to handle all operations.
class NearbyShareAction : public sharesheet::ShareAction {
public:
explicit NearbyShareAction(Profile* profile);
~NearbyShareAction() override;
Expand All @@ -38,17 +33,6 @@ class NearbyShareAction : public sharesheet::ShareAction,
void SetActionCleanupCallbackForArc(
base::OnceCallback<void()> callback) override;

// content::WebContentsDelegate:
bool HandleKeyboardEvent(
content::WebContents* source,
const content::NativeWebKeyboardEvent& event) override;
void WebContentsCreated(content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) override;

static std::vector<std::unique_ptr<Attachment>> CreateAttachmentsFromIntent(
Profile* profile,
apps::mojom::IntentPtr intent);
Expand All @@ -63,8 +47,6 @@ class NearbyShareAction : public sharesheet::ShareAction,
Profile* profile_;
absl::optional<bool> nearby_share_disabled_by_policy_for_testing_ =
absl::nullopt;
views::WebView* web_view_;
views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;
};

#endif // CHROME_BROWSER_NEARBY_SHARING_SHARESHEET_NEARBY_SHARE_ACTION_H_
34 changes: 34 additions & 0 deletions chrome/browser/ui/webui/nearby_share/nearby_share_dialog_ui.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
#include "chrome/browser/nearby_sharing/nearby_sharing_service_impl.h"
#include "chrome/browser/nearby_sharing/text_attachment.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser_navigator.h"
#include "chrome/browser/ui/browser_navigator_params.h"
#include "chrome/browser/ui/scoped_tabbed_browser_displayer.h"
#include "chrome/browser/ui/webui/metrics_handler.h"
#include "chrome/browser/ui/webui/nearby_share/shared_resources.h"
#include "chrome/browser/ui/webui/plural_string_handler.h"
Expand All @@ -36,6 +39,7 @@
#include "services/network/public/mojom/content_security_policy.mojom.h"
#include "ui/base/resource/resource_bundle.h"
#include "ui/base/webui/web_ui_util.h"
#include "ui/views/controls/webview/webview.h"

namespace nearby_share {

Expand Down Expand Up @@ -108,6 +112,12 @@ void NearbyShareDialogUI::SetAttachments(
attachments_ = std::move(attachments);
}

void NearbyShareDialogUI::SetWebView(views::WebView* web_view) {
CHECK(web_view);
web_view_ = web_view;
web_view_->GetWebContents()->SetDelegate(this);
}

void NearbyShareDialogUI::BindInterface(
mojo::PendingReceiver<mojom::DiscoveryManager> manager) {
mojo::MakeSelfOwnedReceiver(
Expand All @@ -132,6 +142,30 @@ void NearbyShareDialogUI::BindInterface(
nearby_sharing_service->GetContactManager()->Bind(std::move(receiver));
}

bool NearbyShareDialogUI::HandleKeyboardEvent(
content::WebContents* source,
const content::NativeWebKeyboardEvent& event) {
if (!web_view_) {
return false;
}

return unhandled_keyboard_event_handler_.HandleKeyboardEvent(
event, web_view_->GetFocusManager());
}

void NearbyShareDialogUI::WebContentsCreated(
content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) {
chrome::ScopedTabbedBrowserDisplayer displayer(Profile::FromWebUI(web_ui()));
NavigateParams nav_params(displayer.browser(), target_url,
ui::PageTransition::PAGE_TRANSITION_LINK);
Navigate(&nav_params);
}

void NearbyShareDialogUI::HandleClose(const base::ListValue* args) {
if (!sharesheet_controller_)
return;
Expand Down
23 changes: 22 additions & 1 deletion chrome/browser/ui/webui/nearby_share/nearby_share_dialog_ui.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,22 @@
#include "chrome/browser/sharesheet/sharesheet_controller.h"
#include "chrome/browser/ui/webui/nearby_share/nearby_share.mojom.h"
#include "chrome/browser/ui/webui/nearby_share/public/mojom/nearby_share_settings.mojom.h"
#include "content/public/browser/web_contents_delegate.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
#include "ui/webui/mojo_web_ui_controller.h"

class NearbySharingService;

namespace views {
class WebView;
} // namespace views

namespace nearby_share {

// The WebUI controller for chrome://nearby.
class NearbyShareDialogUI : public ui::MojoWebUIController {
class NearbyShareDialogUI : public ui::MojoWebUIController,
content::WebContentsDelegate {
public:
explicit NearbyShareDialogUI(content::WebUI* web_ui);
NearbyShareDialogUI(const NearbyShareDialogUI&) = delete;
Expand All @@ -31,6 +38,7 @@ class NearbyShareDialogUI : public ui::MojoWebUIController {
sharesheet_controller_ = controller;
}
void SetAttachments(std::vector<std::unique_ptr<Attachment>> attachments);
void SetWebView(views::WebView* web_view);

// Instantiates the implementor of the mojom::DiscoveryManager mojo
// interface passing the pending receiver that will be internally bound.
Expand All @@ -43,6 +51,17 @@ class NearbyShareDialogUI : public ui::MojoWebUIController {
void BindInterface(
mojo::PendingReceiver<nearby_share::mojom::ContactManager> receiver);

// content::WebContentsDelegate:
bool HandleKeyboardEvent(
content::WebContents* source,
const content::NativeWebKeyboardEvent& event) override;
void WebContentsCreated(content::WebContents* source_contents,
int opener_render_process_id,
int opener_render_frame_id,
const std::string& frame_name,
const GURL& target_url,
content::WebContents* new_contents) override;

private:
void HandleClose(const base::ListValue* args);

Expand All @@ -60,6 +79,8 @@ class NearbyShareDialogUI : public ui::MojoWebUIController {

std::vector<std::unique_ptr<Attachment>> attachments_;
NearbySharingService* nearby_service_;
views::WebView* web_view_ = nullptr;
views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;

WEB_UI_CONTROLLER_TYPE_DECL();
};
Expand Down

0 comments on commit 2d9e826

Please sign in to comment.