Skip to content

Commit

Permalink
Fix WebViewPermissionHelper lookup for dialogs from MimeHandlers
Browse files Browse the repository at this point in the history
MimeHandlerViewGuest uses its embedder's JavaScriptDialogManager, which
is the WebViewGuest's dialog manager when a mime handler loads in a
webview. However, we still use the source WebContents when looking up
the WebViewPermissionHelper, which does not exist for the inner mime
handler contents.

We now look up the WebViewPermissionHelper associated with the dialog
manager's WebViewGuest.

Bug: 1168439
Change-Id: If2d2d2d0a300a7ca7a9f9de40dfa564ac174ce30
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2645159
Reviewed-by: Lucas Gadani <lfg@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#847229}
  • Loading branch information
kjmcnee authored and Chromium LUCI CQ committed Jan 26, 2021
1 parent e5b376f commit 265c092
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 24 deletions.
4 changes: 4 additions & 0 deletions chrome/browser/apps/guest_view/web_view_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3517,6 +3517,10 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, ContextMenuNavigationInMimeHandlerView) {
web_view_contents->GetLastCommittedURL());
}

IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestDialogInPdf) {
TestHelper("testDialogInPdf", "web_view/shim", NO_TEST_SERVER);
}

IN_PROC_BROWSER_TEST_F(WebViewTest, Shim_TestMailtoLink) {
TestHelper("testMailtoLink", "web_view/shim", NEEDS_TEST_SERVER);
}
Expand Down
16 changes: 16 additions & 0 deletions chrome/test/data/extensions/platform_apps/web_view/shim/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -3152,6 +3152,21 @@ function testNavigateToPDFInWebview() {
document.body.appendChild(webview);
}

// Test that when a PDF loaded in a webview triggers a JS dialog, the webview's
// embedder receives the request.
function testDialogInPdf() {
let webview = document.createElement('webview');
let pdfUrl = 'pdf_with_dialog.pdf';
// Partition 'foobar' has access to local resource |pdfUrl|.
webview.partition = 'foobar';
webview.src = pdfUrl;
webview.addEventListener('dialog', (e) => {
e.dialog.ok();
embedder.test.succeed();
});
document.body.appendChild(webview);
}

// This test verifies that mailto links are enabled.
function testMailtoLink() {
var webview = new WebView();
Expand Down Expand Up @@ -3465,6 +3480,7 @@ embedder.test.testList = {
'testFocusWhileFocused': testFocusWhileFocused,
'testPDFInWebview': testPDFInWebview,
'testNavigateToPDFInWebview': testNavigateToPDFInWebview,
'testDialogInPdf': testDialogInPdf,
'testMailtoLink': testMailtoLink,
'testRendererNavigationRedirectWhileUnattached':
testRendererNavigationRedirectWhileUnattached,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"guest_with_inline_script.html",
"test.bmp",
"test.pdf",
"pdf_with_dialog.pdf",
"guest_with_select.html"
]
}
Expand Down
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ ExtensionFunction::ResponseAction WebViewInternalSetPermissionFunction::Run() {
user_input = *params->user_input;

WebViewPermissionHelper* web_view_permission_helper =
WebViewPermissionHelper::FromWebContents(guest_->web_contents());
guest_->web_view_permission_helper();

WebViewPermissionHelper::SetPermissionResult result =
web_view_permission_helper->SetPermission(
Expand Down
13 changes: 2 additions & 11 deletions extensions/browser/guest_view/web_view/javascript_dialog_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,6 @@ void JavaScriptDialogHelper::RunJavaScriptDialog(
const base::string16& default_prompt_text,
DialogClosedCallback callback,
bool* did_suppress_message) {
WebViewPermissionHelper* web_view_permission_helper =
WebViewPermissionHelper::FromWebContents(web_contents);
// crbug.com/1144419: Exit if the permission helper is missing.
// TODO(mcnee) - if |web_contents| is an inner MimeHandlerView,
// this will be null. We should get the WebViewPermissionHelper
// from our WebViewGuest instead.
if (!web_view_permission_helper) {
std::move(callback).Run(false, base::string16());
return;
}

base::DictionaryValue request_info;
request_info.SetString(webview::kDefaultPromptText,
base::UTF16ToUTF8(default_prompt_text));
Expand All @@ -72,6 +61,8 @@ void JavaScriptDialogHelper::RunJavaScriptDialog(
request_info.SetString(guest_view::kUrl,
render_frame_host->GetLastCommittedURL().spec());

WebViewPermissionHelper* web_view_permission_helper =
web_view_guest_->web_view_permission_helper();
web_view_permission_helper->RequestPermission(
WEB_VIEW_PERMISSION_TYPE_JAVASCRIPT_DIALOG, request_info,
base::BindOnce(&JavaScriptDialogHelper::OnPermissionResponse,
Expand Down
5 changes: 3 additions & 2 deletions extensions/browser/guest_view/web_view/web_view_guest.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ class WebViewGuest : public guest_view::GuestView<WebViewGuest> {
base::OnceClosure callback);

ScriptExecutor* script_executor() { return script_executor_.get(); }
WebViewPermissionHelper* web_view_permission_helper() {
return web_view_permission_helper_.get();
}

// Enables or disables spatial navigation.
void SetSpatialNavigationEnabled(bool enabled);
Expand All @@ -157,8 +160,6 @@ class WebViewGuest : public guest_view::GuestView<WebViewGuest> {
bool IsSpatialNavigationEnabled() const;

private:
friend class WebViewPermissionHelper;

explicit WebViewGuest(content::WebContents* owner_web_contents);

~WebViewGuest() override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,21 +164,18 @@ WebViewPermissionHelper::~WebViewPermissionHelper() {
WebViewPermissionHelper* WebViewPermissionHelper::FromFrameID(
int render_process_id,
int render_frame_id) {
WebViewGuest* web_view_guest = WebViewGuest::FromFrameID(
render_process_id, render_frame_id);
if (!web_view_guest) {
return NULL;
}
return web_view_guest->web_view_permission_helper_.get();
WebViewGuest* web_view_guest =
WebViewGuest::FromFrameID(render_process_id, render_frame_id);
return web_view_guest ? web_view_guest->web_view_permission_helper()
: nullptr;
}

// static
WebViewPermissionHelper* WebViewPermissionHelper::FromWebContents(
content::WebContents* web_contents) {
content::WebContents* web_contents) {
WebViewGuest* web_view_guest = WebViewGuest::FromWebContents(web_contents);
if (!web_view_guest)
return NULL;
return web_view_guest->web_view_permission_helper_.get();
return web_view_guest ? web_view_guest->web_view_permission_helper()
: nullptr;
}

#if BUILDFLAG(ENABLE_PLUGINS)
Expand Down

0 comments on commit 265c092

Please sign in to comment.