Stop using nested message loop for alert() and other JS dialogs

content/renderer/render_thread_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_thread_impl.h"
 
 #include <algorithm>
 #include <limits>
 #include <map>
 #include <utility>
@@ skipping 596 matching lines @@
 #endif
 
   lazy_tls.Pointer()->Set(this);
 
   // Register this object as the main thread.
   ChildProcess::current()->set_main_thread(this);
 
   InitializeWebKit(resource_task_queue);
 
   // In single process the single process is all there is.
-  notify_webkit_of_modal_loop_ = true;
   webkit_shared_timer_suspended_ = false;
   widget_count_ = 0;
   hidden_widget_count_ = 0;
   idle_notification_delay_in_ms_ = kInitialIdleHandlerDelayMs;
   idle_notifications_to_skip_ = 0;
   layout_test_mode_ = false;
 
   appcache_dispatcher_.reset(
       new AppCacheDispatcher(Get(), new AppCacheFrontendImpl()));
   dom_storage_dispatcher_.reset(new DomStorageDispatcher());
@@ skipping 313 matching lines @@
 
   lazy_tls.Pointer()->Set(NULL);
 }
 
 bool RenderThreadImpl::Send(IPC::Message* msg) {
   // Certain synchronous messages cannot always be processed synchronously by
   // the browser, e.g., putting up UI and waiting for the user. This could cause
   // a complete hang of Chrome if a windowed plugin is trying to communicate
   // with the renderer thread since the browser's UI thread could be stuck
   // (within a Windows API call) trying to synchronously communicate with the
   // plugin.  The remedy is to pump messages on this thread while the browser

[no sievers, 2016/05/18 20:11:24]

So how does the described scenario not break with removing the 'remedy' in this
patch?

[Changwan Ryu, 2016/05/19 00:44:56]

I'm not entirely removing the remedy in this patch. This function still allows
synchronous message as a parameter.

However, the PPAPI case has already been deprecated, so I've updated the comment
to be more generic.

[Changwan Ryu, 2016/05/19 01:00:31]

Correction: s/PPAPI/NPAPI

   // is processing this request. This creates an opportunity for re-entrancy

[no sievers, 2016/05/18 20:11:24]

This comment (or part of it) doesn't apply anymore with this change right?

[Changwan Ryu, 2016/05/19 00:44:56]

It still holds for some cases such as PrintWebViewHelper and FlashMessageLoop.

   // into WebKit, so we need to take care to disable callbacks, timers, and
   // pending network loads that could trigger such callbacks.
   bool pumping_events = false;
   if (msg->is_sync()) {
     if (msg->is_caller_pumping_messages()) {
       pumping_events = true;
     }
   }
 
-  bool notify_webkit_of_modal_loop = true;  // default value
-  std::swap(notify_webkit_of_modal_loop, notify_webkit_of_modal_loop_);
-
   if (pumping_events) {
     renderer_scheduler_->SuspendTimerQueue();
-
-    if (notify_webkit_of_modal_loop)
-      WebView::willEnterModalLoop();
+    WebView::willEnterModalLoop();

[no sievers, 2016/05/18 20:11:24]

...and related to that: why do we still need to tell webkit if we won't dispatch
nested messages anymore?

[Changwan Ryu, 2016/05/19 00:44:56]

PrintWebViewHelper and FlashMessageLoop may need to pause resource dispatching
until their sync messages get replies. JS dialogues were exceptions where we did
not have to notify webkit.

[Lei Zhang, 2016/05/19 00:50:28]

window.print() is still a thing and it is synchronous.

[Changwan Ryu, 2016/05/19 01:00:31]

Thanks for the correction. I mean alert(), beforeunload(), confirm(), and
prompt(). window.print() goes through PrintWebViewHelper and still pumps
messages.

   }
 
   bool rv = ChildThreadImpl::Send(msg);
 
   if (pumping_events) {
-    if (notify_webkit_of_modal_loop)
-      WebView::didExitModalLoop();
-
+    WebView::didExitModalLoop();
     renderer_scheduler_->ResumeTimerQueue();
   }
 
   return rv;
 }
 
 IPC::SyncChannel* RenderThreadImpl::GetChannel() {
   return channel();
 }
 
@@ skipping 625 matching lines @@
 scoped_refptr<base::SingleThreadTaskRunner>
 RenderThreadImpl::GetIOThreadTaskRunner() {
   return io_thread_task_runner_;
 }
 
 std::unique_ptr<base::SharedMemory> RenderThreadImpl::AllocateSharedMemory(
     size_t size) {
   return HostAllocateSharedMemoryBuffer(size);
 }
 
-void RenderThreadImpl::DoNotNotifyWebKitOfModalLoop() {
-  notify_webkit_of_modal_loop_ = false;
-}
-
 void RenderThreadImpl::OnChannelError() {
   // In single-process mode, the renderer can't be restarted after shutdown.
   // So, if we get a channel error, crash the whole process right now to get a
   // more informative stack, since we will otherwise just crash later when we
   // try to restart it.
   CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSingleProcess));
   ChildThreadImpl::OnChannelError();
 }
 
@@ skipping 409 matching lines @@
 }
 
 void RenderThreadImpl::PendingRenderFrameConnect::OnConnectionError() {
   size_t erased =
       RenderThreadImpl::current()->pending_render_frame_connects_.erase(
           routing_id_);
   DCHECK_EQ(1u, erased);
 }
 
 }  // namespace content