Skip to content

Commit

Permalink
Add WebContents source to methods in WebContentsDelegate
Browse files Browse the repository at this point in the history
The added parameter helps us avoid a potential timing-related security issue. The UI can be instructed to open a new tab and show a dialog roughly at the same time, and since it's handled asynchronously the dialog may end up in front of the new tab. Knowing what web contents issued the dialog would allow us to prevent that from happening.

BUG=428793

Review URL: https://codereview.chromium.org/691823002

Cr-Commit-Position: refs/heads/master@{#304781}
  • Loading branch information
mathiash authored and Commit bot committed Nov 19, 2014
1 parent bc4af03 commit 72a5e46
Show file tree
Hide file tree
Showing 30 changed files with 64 additions and 48 deletions.
2 changes: 1 addition & 1 deletion android_webview/native/aw_web_contents_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ AwWebContentsDelegate::~AwWebContentsDelegate() {
}

content::JavaScriptDialogManager*
AwWebContentsDelegate::GetJavaScriptDialogManager() {
AwWebContentsDelegate::GetJavaScriptDialogManager(WebContents* source) {
return g_javascript_dialog_manager.Pointer();
}

Expand Down
4 changes: 2 additions & 2 deletions android_webview/native/aw_web_contents_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ class AwWebContentsDelegate
public:
AwWebContentsDelegate(JNIEnv* env, jobject obj);
virtual ~AwWebContentsDelegate();
virtual content::JavaScriptDialogManager* GetJavaScriptDialogManager()
override;
virtual content::JavaScriptDialogManager* GetJavaScriptDialogManager(
content::WebContents* source) override;
virtual void FindReply(content::WebContents* web_contents,
int request_id,
int number_of_matches,
Expand Down
3 changes: 2 additions & 1 deletion apps/custom_launcher_page_contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ bool CustomLauncherPageContents::IsPopupOrPanel(
return true;
}

bool CustomLauncherPageContents::ShouldSuppressDialogs() {
bool CustomLauncherPageContents::ShouldSuppressDialogs(
content::WebContents* source) {
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion apps/custom_launcher_page_contents.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class CustomLauncherPageContents
bool user_gesture,
bool* was_blocked) override;
bool IsPopupOrPanel(const content::WebContents* source) const override;
bool ShouldSuppressDialogs() override;
bool ShouldSuppressDialogs(content::WebContents* source) override;
bool PreHandleGestureEvent(content::WebContents* source,
const blink::WebGestureEvent& event) override;
content::ColorChooser* OpenColorChooser(
Expand Down
3 changes: 2 additions & 1 deletion athena/content/web_activity.cc
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,8 @@ class AthenaWebView : public views::WebView {
layer->SetOpacity(0.f);
}

content::JavaScriptDialogManager* GetJavaScriptDialogManager() override {
content::JavaScriptDialogManager* GetJavaScriptDialogManager(
content::WebContents* contents) override {
return GetJavaScriptDialogManagerInstance();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ void ChromeWebContentsDelegateAndroid::FindMatchRectsReply(
}

content::JavaScriptDialogManager*
ChromeWebContentsDelegateAndroid::GetJavaScriptDialogManager() {
ChromeWebContentsDelegateAndroid::GetJavaScriptDialogManager(
WebContents* source) {
return GetJavaScriptDialogManagerInstance();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class ChromeWebContentsDelegateAndroid
const std::vector<gfx::RectF>& rects,
const gfx::RectF& active_rect) override;
virtual content::JavaScriptDialogManager*
GetJavaScriptDialogManager() override;
GetJavaScriptDialogManager(content::WebContents* source) override;
virtual void RequestMediaAccessPermission(
content::WebContents* web_contents,
const content::MediaStreamRequest& request,
Expand Down
10 changes: 6 additions & 4 deletions chrome/browser/devtools/devtools_window.cc
Original file line number Diff line number Diff line change
Expand Up @@ -929,11 +929,13 @@ void DevToolsWindow::HandleKeyboardEvent(
inspected_window->HandleKeyboardEvent(event);
}

content::JavaScriptDialogManager* DevToolsWindow::GetJavaScriptDialogManager() {
content::JavaScriptDialogManager* DevToolsWindow::GetJavaScriptDialogManager(
WebContents* source) {
WebContents* inspected_web_contents = GetInspectedWebContents();
return (inspected_web_contents && inspected_web_contents->GetDelegate()) ?
inspected_web_contents->GetDelegate()->GetJavaScriptDialogManager() :
content::WebContentsDelegate::GetJavaScriptDialogManager();
return (inspected_web_contents && inspected_web_contents->GetDelegate())
? inspected_web_contents->GetDelegate()
->GetJavaScriptDialogManager(inspected_web_contents)
: content::WebContentsDelegate::GetJavaScriptDialogManager(source);
}

content::ColorChooser* DevToolsWindow::OpenColorChooser(
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/devtools/devtools_window.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,8 @@ class DevToolsWindow : public DevToolsUIBindings::Delegate,
void HandleKeyboardEvent(
content::WebContents* source,
const content::NativeWebKeyboardEvent& event) override;
content::JavaScriptDialogManager* GetJavaScriptDialogManager() override;
content::JavaScriptDialogManager* GetJavaScriptDialogManager(
content::WebContents* source) override;
content::ColorChooser* OpenColorChooser(
content::WebContents* web_contents,
SkColor color,
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/prerender/prerender_contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ class PrerenderContents::WebContentsDelegateImpl
return false;
}

bool ShouldSuppressDialogs() override {
bool ShouldSuppressDialogs(WebContents* source) override {
// We still want to show the user the message when they navigate to this
// page, so cancel this prerender.
prerender_contents_->Destroy(FINAL_STATUS_JAVASCRIPT_ALERT);
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/prerender/prerender_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,10 @@ class PrerenderManager::OnCloseWebContentsDeleter
ScheduleWebContentsForDeletion(false);
}

bool ShouldSuppressDialogs() override {
bool ShouldSuppressDialogs(WebContents* source) override {
// Use this as a proxy for getting statistics on how often we fail to honor
// the beforeunload event.
DCHECK_EQ(tab_, source);
suppressed_dialog_ = true;
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/tab_contents/background_contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ void BackgroundContents::CloseContents(WebContents* source) {
delete this;
}

bool BackgroundContents::ShouldSuppressDialogs() {
bool BackgroundContents::ShouldSuppressDialogs(WebContents* source) {
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/tab_contents/background_contents.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class BackgroundContents : public content::WebContentsDelegate,

// content::WebContentsDelegate implementation:
void CloseContents(content::WebContents* source) override;
bool ShouldSuppressDialogs() override;
bool ShouldSuppressDialogs(content::WebContents* source) override;
void DidNavigateMainFramePostCommit(content::WebContents* tab) override;
void AddNewContents(content::WebContents* source,
content::WebContents* new_contents,
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/ui/browser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1622,7 +1622,8 @@ void Browser::DidNavigateToPendingEntry(WebContents* web_contents) {
UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_STATE);
}

content::JavaScriptDialogManager* Browser::GetJavaScriptDialogManager() {
content::JavaScriptDialogManager* Browser::GetJavaScriptDialogManager(
WebContents* source) {
return GetJavaScriptDialogManagerInstance();
}

Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/ui/browser.h
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,8 @@ class Browser : public TabStripModelObserver,
void DidNavigateMainFramePostCommit(
content::WebContents* web_contents) override;
void DidNavigateToPendingEntry(content::WebContents* web_contents) override;
content::JavaScriptDialogManager* GetJavaScriptDialogManager() override;
content::JavaScriptDialogManager* GetJavaScriptDialogManager(
content::WebContents* source) override;
content::ColorChooser* OpenColorChooser(
content::WebContents* web_contents,
SkColor color,
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/fast_unload_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class FastUnloadController::DetachedWebContentsDelegate

private:
// WebContentsDelegate implementation.
bool ShouldSuppressDialogs() override {
bool ShouldSuppressDialogs(content::WebContents* source) override {
return true; // Return true so dialogs are suppressed.
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,9 @@ class DeviceInertialSensorBrowserTest : public ContentBrowserTest {
}

void WaitForAlertDialogAndQuitAfterDelay(base::TimeDelta delay) {
ShellJavaScriptDialogManager* dialog_manager=
ShellJavaScriptDialogManager* dialog_manager =
static_cast<ShellJavaScriptDialogManager*>(
shell()->GetJavaScriptDialogManager());
shell()->GetJavaScriptDialogManager(shell()->web_contents()));

scoped_refptr<MessageLoopRunner> runner = new MessageLoopRunner();
dialog_manager->set_dialog_request_callback(
Expand Down
2 changes: 1 addition & 1 deletion content/browser/devtools/protocol/page_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ Response PageHandler::HandleJavaScriptDialog(bool accept,
return Response::InternalError("No JavaScript dialog to handle");

JavaScriptDialogManager* manager =
web_contents->GetDelegate()->GetJavaScriptDialogManager();
web_contents->GetDelegate()->GetJavaScriptDialogManager(web_contents);
if (manager && manager->HandleJavaScriptDialog(
web_contents, accept, prompt_text ? &prompt_override : nullptr)) {
return Response::OK();
Expand Down
18 changes: 8 additions & 10 deletions content/browser/web_contents/web_contents_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3396,15 +3396,14 @@ void WebContentsImpl::RunJavaScriptMessage(
// want the hidden page's dialogs to interfere with the interstitial.
bool suppress_this_message =
static_cast<RenderFrameHostImpl*>(render_frame_host)->is_swapped_out() ||
ShowingInterstitialPage() ||
!delegate_ ||
delegate_->ShouldSuppressDialogs() ||
!delegate_->GetJavaScriptDialogManager();
ShowingInterstitialPage() || !delegate_ ||
delegate_->ShouldSuppressDialogs(this) ||
!delegate_->GetJavaScriptDialogManager(this);

if (!suppress_this_message) {
std::string accept_lang = GetContentClient()->browser()->
GetAcceptLangs(GetBrowserContext());
dialog_manager_ = delegate_->GetJavaScriptDialogManager();
dialog_manager_ = delegate_->GetJavaScriptDialogManager(this);
dialog_manager_->RunJavaScriptDialog(
this,
frame_url.GetOrigin(),
Expand Down Expand Up @@ -3444,17 +3443,16 @@ void WebContentsImpl::RunBeforeUnloadConfirm(
delegate_->WillRunBeforeUnloadConfirm();

bool suppress_this_message =
rfhi->rfh_state() != RenderFrameHostImpl::STATE_DEFAULT ||
!delegate_ ||
delegate_->ShouldSuppressDialogs() ||
!delegate_->GetJavaScriptDialogManager();
rfhi->rfh_state() != RenderFrameHostImpl::STATE_DEFAULT || !delegate_ ||
delegate_->ShouldSuppressDialogs(this) ||
!delegate_->GetJavaScriptDialogManager(this);
if (suppress_this_message) {
rfhi->JavaScriptDialogClosed(reply_msg, true, base::string16(), true);
return;
}

is_showing_before_unload_dialog_ = true;
dialog_manager_ = delegate_->GetJavaScriptDialogManager();
dialog_manager_ = delegate_->GetJavaScriptDialogManager(this);
dialog_manager_->RunBeforeUnloadDialog(
this, message, is_reload,
base::Bind(&WebContentsImpl::OnDialogClosed, base::Unretained(this),
Expand Down
5 changes: 3 additions & 2 deletions content/public/browser/web_contents_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ gfx::Rect WebContentsDelegate::GetRootWindowResizerRect() const {
return gfx::Rect();
}

bool WebContentsDelegate::ShouldSuppressDialogs() {
bool WebContentsDelegate::ShouldSuppressDialogs(WebContents* source) {
return false;
}

Expand Down Expand Up @@ -140,7 +140,8 @@ bool WebContentsDelegate::ShouldCreateWebContents(
return true;
}

JavaScriptDialogManager* WebContentsDelegate::GetJavaScriptDialogManager() {
JavaScriptDialogManager* WebContentsDelegate::GetJavaScriptDialogManager(
WebContents* source) {
return NULL;
}

Expand Down
5 changes: 3 additions & 2 deletions content/public/browser/web_contents_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ class CONTENT_EXPORT WebContentsDelegate {

// Returns true if javascript dialogs and unload alerts are suppressed.
// Default is false.
virtual bool ShouldSuppressDialogs();
virtual bool ShouldSuppressDialogs(WebContents* source);

// Returns whether pending NavigationEntries for aborted browser-initiated
// navigations should be preserved (and thus returned from GetVisibleURL).
Expand Down Expand Up @@ -342,7 +342,8 @@ class CONTENT_EXPORT WebContentsDelegate {

// Returns a pointer to a service to manage JavaScript dialogs. May return
// NULL in which case dialogs aren't shown.
virtual JavaScriptDialogManager* GetJavaScriptDialogManager();
virtual JavaScriptDialogManager* GetJavaScriptDialogManager(
WebContents* source);

// Called when color chooser should open. Returns the opened color chooser.
// Returns NULL if we failed to open the color chooser (e.g. when there is a
Expand Down
4 changes: 2 additions & 2 deletions content/public/test/content_browser_test_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ bool NavigateToURLAndExpectNoCommit(Shell* window, const GURL& url) {
}

void WaitForAppModalDialog(Shell* window) {
ShellJavaScriptDialogManager* dialog_manager=
ShellJavaScriptDialogManager* dialog_manager =
static_cast<ShellJavaScriptDialogManager*>(
window->GetJavaScriptDialogManager());
window->GetJavaScriptDialogManager(window->web_contents()));

scoped_refptr<MessageLoopRunner> runner = new MessageLoopRunner();
dialog_manager->set_dialog_request_callback(runner->QuitClosure());
Expand Down
3 changes: 2 additions & 1 deletion content/shell/browser/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,8 @@ void Shell::DidNavigateMainFramePostCommit(WebContents* web_contents) {
PlatformSetAddressBarURL(web_contents->GetLastCommittedURL());
}

JavaScriptDialogManager* Shell::GetJavaScriptDialogManager() {
JavaScriptDialogManager* Shell::GetJavaScriptDialogManager(
WebContents* source) {
if (!dialog_manager_) {
const CommandLine& command_line = *CommandLine::ForCurrentProcess();
dialog_manager_.reset(command_line.HasSwitch(switches::kDumpRenderTree)
Expand Down
3 changes: 2 additions & 1 deletion content/shell/browser/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,8 @@ class Shell : public WebContentsDelegate,
void CloseContents(WebContents* source) override;
bool CanOverscrollContent() const override;
void DidNavigateMainFramePostCommit(WebContents* web_contents) override;
JavaScriptDialogManager* GetJavaScriptDialogManager() override;
JavaScriptDialogManager* GetJavaScriptDialogManager(
WebContents* source) override;
#if defined(OS_MACOSX)
void HandleKeyboardEvent(WebContents* source,
const NativeWebKeyboardEvent& event) override;
Expand Down
4 changes: 3 additions & 1 deletion extensions/browser/app_window/app_window.cc
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,9 @@ void AppWindow::CloseContents(WebContents* contents) {
native_app_window_->Close();
}

bool AppWindow::ShouldSuppressDialogs() { return true; }
bool AppWindow::ShouldSuppressDialogs(WebContents* source) {
return true;
}

content::ColorChooser* AppWindow::OpenColorChooser(
WebContents* web_contents,
Expand Down
2 changes: 1 addition & 1 deletion extensions/browser/app_window/app_window.h
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ class AppWindow : public content::NotificationObserver,

// content::WebContentsDelegate implementation.
void CloseContents(content::WebContents* contents) override;
bool ShouldSuppressDialogs() override;
bool ShouldSuppressDialogs(content::WebContents* source) override;
content::ColorChooser* OpenColorChooser(
content::WebContents* web_contents,
SkColor color,
Expand Down
3 changes: 2 additions & 1 deletion extensions/browser/extension_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,8 @@ void ExtensionHost::RenderViewDeleted(RenderViewHost* render_view_host) {
render_view_host_ = host_contents_->GetRenderViewHost();
}

content::JavaScriptDialogManager* ExtensionHost::GetJavaScriptDialogManager() {
content::JavaScriptDialogManager* ExtensionHost::GetJavaScriptDialogManager(
WebContents* source) {
return delegate_->GetJavaScriptDialogManager();
}

Expand Down
3 changes: 2 additions & 1 deletion extensions/browser/extension_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ class ExtensionHost : public content::WebContentsDelegate,
void DidStopLoading(content::RenderViewHost* render_view_host) override;

// content::WebContentsDelegate
content::JavaScriptDialogManager* GetJavaScriptDialogManager() override;
content::JavaScriptDialogManager* GetJavaScriptDialogManager(
content::WebContents* source) override;
void AddNewContents(content::WebContents* source,
content::WebContents* new_contents,
WindowOpenDisposition disposition,
Expand Down
4 changes: 2 additions & 2 deletions extensions/browser/guest_view/web_view/web_view_guest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -902,8 +902,8 @@ void WebViewGuest::WillAttachToEmbedder() {
PushWebViewStateToIOThread();
}

content::JavaScriptDialogManager*
WebViewGuest::GetJavaScriptDialogManager() {
content::JavaScriptDialogManager* WebViewGuest::GetJavaScriptDialogManager(
WebContents* source) {
return &javascript_dialog_helper_;
}

Expand Down
3 changes: 2 additions & 1 deletion extensions/browser/guest_view/web_view/web_view_guest.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ class WebViewGuest : public GuestView<WebViewGuest>,
const GURL& url,
const std::string& request_method,
const base::Callback<void(bool)>& callback) override;
content::JavaScriptDialogManager* GetJavaScriptDialogManager() override;
content::JavaScriptDialogManager* GetJavaScriptDialogManager(
content::WebContents* source) override;
content::ColorChooser* OpenColorChooser(
content::WebContents* web_contents,
SkColor color,
Expand Down

0 comments on commit 72a5e46

Please sign in to comment.