Skip to content

GitLab

T
tangled
Commit 257c32d6 authored by robliao@chromium.org's avatar robliao@chromium.org
Browse files
Options

Revert of Ensure that modal dialogs from subframes can be cleaned up... 

Revert of Ensure that modal dialogs from subframes can be cleaned up correctly. (https://codereview.chromium.org/255543009/)

Reason for revert:
Causes leaks:

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%282%29/builds/2046/steps/browser_tests/logs/SadTabCancelsSubframeDialogs

Direct leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x56973b in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:62
    #1 0x52464ce in IPC::SyncMessage::GenerateReply(IPC::Message const*) ipc/ipc_sync_message.cc:99
    #2 0x9f6ec47 in Tuple4 ipc/ipc_message_utils.h:877
    #3 0x9f6ec47 in bool FrameHostMsg_RunJavaScriptMessage::DispatchDelayReply\u003Ccontent::RenderFrameHostImpl, void (content::RenderFrameHostImpl::*)(std::basic_string\u003Cunsigned short, base::string16_char_traits, std::allocator\u003Cunsigned short> > const&, std::basic_string\u003Cunsigned short, base::string16_char_traits, std::allocator\u003Cunsigned short> > const&, GURL const&, content::JavaScriptMessageType, IPC::Message*)>(IPC::Message const*, content::RenderFrameHostImpl*, void (content::RenderFrameHostImpl::*)(std::basic_string\u003Cunsigned short, base::string16_char_traits, std::allocator\u003Cunsigned short> > const&, std::basic_string\u003Cunsigned short, base::string16_char_traits, std::allocator\u003Cunsigned short> > const&, GURL const&, content::JavaScriptMessageType, IPC::Message*)) content/common/frame_messages.h:545
    #4 0x9f6b9d3 in content::RenderFrameHostImpl::OnMessageReceived(IPC::Message const&) content/browser/frame_host/render_frame_host_impl.cc:287
    #5 0xa1185d9 in content::RenderProcessHostImpl::OnMessageReceived(IPC::Message const&) content/browser/renderer_host/render_process_host_impl.cc:1346
    #6 0x5239eec in IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) ipc/ipc_channel_proxy.cc:269
    #7 0x3256fbf in Run base/callback.h:401
    #8 0x3256fbf in base::MessageLoop::RunTask(base::PendingTask const&) base/message_loop/message_loop.cc:450
    #9 0x3258e35 in DeferOrRunPendingTask base/message_loop/message_loop.cc:462
    #10 0x3258e35 in base::MessageLoop::DoWork() base/message_loop/message_loop.cc:576
    #11 0x32fab87 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_glib.cc:270
    #12 0x3290314 in base::RunLoop::Run() base/run_loop.cc:49
    #13 0xc55c305 in content::RunThisRunLoop(base::RunLoop*) content/public/test/test_utils.cc:103
    #14 0xc55e252 in operator-> content/public/test/test_utils.cc:165
    #15 0xc55e252 in content::WindowedNotificationObserver::Wait() content/public/test/test_utils.cc:225
    #16 0x31af7c1 in ui_test_utils::WaitForAppModalDialog() chrome/test/base/ui_test_utils.cc:342
    #17 0x19c1ed8 in BrowserTest_SadTabCancelsSubframeDialogs_Test::RunTestOnMainThread() chrome/browser/ui/browser_browsertest.cc:585
    #18 0x31996b8 in InProcessBrowserTest::RunTestOnMainThreadLoop() chrome/test/base/in_process_browser_test.cc:419


Original issue's description:
> Ensure that modal dialogs from subframes can be cleaned up correctly.
> 
> BUG=366510
> TEST=See bug comment 7 for repro steps.
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266638

TBR=avi@chromium.org,sky@chromium.org,creis@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=366510

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266669 0039d316-1c4b-4281-b951-d872f2087c98
parent 1e70daac
Hide whitespace changes
Inline Side-by-side
Showing with 10 additions and 51 deletions
chrome/browser/ui/browser_browsertest.cc
View file @ 257c32d6
......@@ -574,33 +574,6 @@ IN_PROC_BROWSER_TEST_F(BrowserTest, SadTabCancelsDialogs) {
ui_test_utils::NavigateToURL(browser(), url2);
}
// Make sure that dialogs opened by subframes are closed when the process dies.
// See http://crbug.com/366510.
IN_PROC_BROWSER_TEST_F(BrowserTest, SadTabCancelsSubframeDialogs) {
// Navigate to an iframe that opens an alert dialog.
WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
contents->GetMainFrame()->ExecuteJavaScript(
ASCIIToUTF16("window.location.href = 'data:text/html,"
"<iframe srcdoc=\"<script>alert(1)</script>\">'"));
AppModalDialog* alert = ui_test_utils::WaitForAppModalDialog();
EXPECT_TRUE(alert->IsValid());
AppModalDialogQueue* dialog_queue = AppModalDialogQueue::GetInstance();
EXPECT_TRUE(dialog_queue->HasActiveDialog());
// Crash the renderer process and ensure the dialog is gone.
content::RenderProcessHost* child_process = contents->GetRenderProcessHost();
content::RenderProcessHostWatcher crash_observer(
child_process,
content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
base::KillProcess(child_process->GetHandle(), 0, false);
crash_observer.Wait();
EXPECT_FALSE(dialog_queue->HasActiveDialog());
// Make sure subsequent navigations work.
GURL url2("data:text/html,foo");
ui_test_utils::NavigateToURL(browser(), url2);
}
// Test for crbug.com/22004. Reloading a page with a before unload handler and
// then canceling the dialog should not leave the throbber spinning.
IN_PROC_BROWSER_TEST_F(BrowserTest, ReloadThenCancelBeforeUnload) {
......
content/browser/renderer_host/render_view_host_impl.cc
View file @ 257c32d6
......@@ -1166,9 +1166,7 @@ void RenderViewHostImpl::OnRenderProcessGone(int status, int exit_code) {
render_view_termination_status_ =
static_cast<base::TerminationStatus>(status);
// Reset frame tree state associated with this process. This must happen
// before RenderViewTerminated because observers expect the subframes of any
// affected frames to be cleared first.
// Reset frame tree state associated with this process.
delegate_->GetFrameTree()->RenderProcessGone(this);
// Our base class RenderWidgetHost needs to reset some stuff.
......
content/browser/web_contents/web_contents_impl.cc
View file @ 257c32d6
......@@ -3089,8 +3089,7 @@ void WebContentsImpl::RunJavaScriptMessage(
default_prompt,
base::Bind(&WebContentsImpl::OnDialogClosed,
base::Unretained(this),
rfh->GetProcess()->GetID(),
rfh->GetRoutingID(),
rfh,
reply_msg,
false),
&suppress_this_message);
......@@ -3099,8 +3098,7 @@ void WebContentsImpl::RunJavaScriptMessage(
if (suppress_this_message) {
// If we are suppressing messages, just reply as if the user immediately
// pressed "Cancel", passing true to |dialog_was_suppressed|.
OnDialogClosed(rfh->GetProcess()->GetID(), rfh->GetRoutingID(), reply_msg,
true, false, base::string16());
OnDialogClosed(rfh, reply_msg, true, false, base::string16());
}
// OnDialogClosed (two lines up) may have caused deletion of this object (see
......@@ -3133,8 +3131,7 @@ void WebContentsImpl::RunBeforeUnloadConfirm(
dialog_manager_->RunBeforeUnloadDialog(
this, message, is_reload,
base::Bind(&WebContentsImpl::OnDialogClosed, base::Unretained(this),
rfh->GetProcess()->GetID(), rfh->GetRoutingID(), reply_msg,
false));
rfh, reply_msg, false));
}
WebContents* WebContentsImpl::GetAsWebContents() {
......@@ -3810,21 +3807,17 @@ bool WebContentsImpl::CreateRenderViewForInitialEmptyDocument() {
}
#endif
void WebContentsImpl::OnDialogClosed(int render_process_id,
int render_frame_id,
void WebContentsImpl::OnDialogClosed(RenderFrameHost* rfh,
IPC::Message* reply_msg,
bool dialog_was_suppressed,
bool success,
const base::string16& user_input) {
RenderFrameHostImpl* rfh = RenderFrameHostImpl::FromID(render_process_id,
render_frame_id);
last_dialog_suppressed_ = dialog_was_suppressed;
if (is_showing_before_unload_dialog_ && !success) {
// If a beforeunload dialog is canceled, we need to stop the throbber from
// spinning, since we forced it to start spinning in Navigate.
if (rfh)
DidStopLoading(rfh);
DidStopLoading(rfh);
controller_.DiscardNonCommittedEntries();
FOR_EACH_OBSERVER(WebContentsObserver, observers_,
......@@ -3832,10 +3825,8 @@ void WebContentsImpl::OnDialogClosed(int render_process_id,
}
is_showing_before_unload_dialog_ = false;
if (rfh) {
rfh->JavaScriptDialogClosed(reply_msg, success, user_input,
dialog_was_suppressed);
}
static_cast<RenderFrameHostImpl*>(rfh)->JavaScriptDialogClosed(
reply_msg, success, user_input, dialog_was_suppressed);
}
void WebContentsImpl::SetEncoding(const std::string& encoding) {
......
content/browser/web_contents/web_contents_impl.h
View file @ 257c32d6
......@@ -670,11 +670,8 @@ class CONTENT_EXPORT WebContentsImpl
// watching |web_contents|. No-op if there is no such observer.
void RemoveDestructionObserver(WebContentsImpl* web_contents);
// Callback function when showing JavaScript dialogs. Takes in a routing ID
// pair to identify the RenderFrameHost that opened the dialog, because it's
// possible for the RenderFrameHost to be deleted by the time this is called.
void OnDialogClosed(int render_process_id,
int render_frame_id,
// Callback function when showing JavaScript dialogs.
void OnDialogClosed(RenderFrameHost* rfh,
IPC::Message* reply_msg,
bool dialog_was_suppressed,
bool success,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment