Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Revert of Ensure that modal dialogs from subframes can be cleaned up …
Browse files Browse the repository at this point in the history 
…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

[email protected],[email protected],[email protected]
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
  • Loading branch information
[email protected] committed Apr 28, 2014
1 parent 1e70daa commit 257c32d
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 51 deletions.
27 changes: 0 additions & 27 deletions chrome/browser/ui/browser_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 1 addition & 3 deletions content/browser/renderer_host/render_view_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
23 changes: 7 additions & 16 deletions content/browser/web_contents/web_contents_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -3810,32 +3807,26 @@ 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_,
BeforeUnloadDialogCancelled());
}

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) {
Expand Down
7 changes: 2 additions & 5 deletions content/browser/web_contents/web_contents_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 257c32d

Please sign in to comment.