Fix deadlocks on WebEngineContext destruction

This patch aims to handle 3 deadlocks on exit:

(1) Do not attempt to restart Viz thread on shutdown,
    this will make deadlock with no separate gpu thread,
    since force shut down of FrameSinkManager requires
    interaction with gpu process.

(2) QCoreApplication, QGuiApplication, QApplication
    behave differently when doing qt post routine,
    ~QGuiApplication calls the post routine handler after
    closing event dispatcher, which will on windows stop processing
    timer events, which we need to pump message loop when
    shuting down viz. Do not use QEventLoop and switch
    to active pulling. The proper solution is to fix QGuiApplication
    destructor to call post routine first, but this change  might
    have side effects on already existing user code.

(3) Since 7f1649b438329e we delete root frame sink asynchronously,
    which will in gpu thread running in separate thread create a deadlock.
    Viz requires gpu to destruct root frame sink, however if main process
    tries to close gpu process this will in turn try close viz, but viz calls
    back gpu now since the root frame sink is not destroyed.
    Use the same solution as in (1).

Change-Id: Ic6bc904bdac90ee01a5c5b9398a2e2746be3bbd8
Reviewed-by: Jüri Valdmann <juri.valdmann@qt.io>

3 files changed, 42 insertions, 30 deletions

diff --git a/src/core/web_engine_context.cpp b/src/core/web_engine_context.cpp
index c34d3c612..286842a20 100644
--- a/src/core/web_engine_context.cpp
+++ b/src/core/web_engine_context.cpp
@@ -49,6 +49,9 @@
 #include "base/task/sequence_manager/thread_controller_with_message_pump_impl.h"
 #include "base/threading/thread_restrictions.h"
 #include "cc/base/switches.h"
+#include "content/gpu/gpu_child_thread.h"
+#include "content/browser/compositor/surface_utils.h"
+#include "components/viz/host/host_frame_sink_manager.h"
 #if QT_CONFIG(webengine_printing_and_pdf)
 #include "chrome/browser/printing/print_job_manager.h"
 #include "components/printing/browser/features.h"
@@ -211,6 +214,26 @@ bool usingSoftwareDynamicGL()
 #endif
 }
 
+static bool waitForViz = false;
+static void completeVizCleanup()
+{
+    waitForViz = false;
+}
+
+static void cleanupVizProcess()
+{
+    auto gpuChildThread = content::GpuChildThread::instance();
+    if (!gpuChildThread)
+        return;
+    auto vizMain = gpuChildThread->viz_main();
+    auto vizCompositorThreadRunner = vizMain->viz_compositor_thread_runner();
+    if (!vizCompositorThreadRunner)
+        return;
+    waitForViz = true;
+    content::GetHostFrameSinkManager()->SetConnectionLostCallback(base::DoNothing());
+    vizCompositorThreadRunner->CleanupForShutdown(base::BindOnce(&completeVizCleanup));
+}
+
 scoped_refptr<QtWebEngineCore::WebEngineContext> WebEngineContext::m_handle;
 bool WebEngineContext::m_destroyed = false;
 
@@ -257,14 +280,21 @@ void WebEngineContext::destroy()
     if (m_devtoolsServer)
         m_devtoolsServer->stop();
 
-    // Normally the GPU thread is shut down when the GpuProcessHost is destroyed
-    // on IO thread (triggered by ~BrowserMainRunner). But by that time the UI
-    // task runner is not working anymore so we need to do this earlier.
-    destroyGpuProcess(m_threadedGpu);
 
     base::MessagePump::Delegate *delegate =
             static_cast<base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl *>(
                 m_runLoop->delegate_);
+    // Normally the GPU thread is shut down when the GpuProcessHost is destroyed
+    // on IO thread (triggered by ~BrowserMainRunner). But by that time the UI
+    // task runner is not working anymore so we need to do this earlier.
+    if (features::IsVizDisplayCompositorEnabled()) {
+        cleanupVizProcess();
+        while (waitForViz) {
+            while (delegate->DoWork()){}
+            QThread::msleep(50);
+        }
+    }
+    destroyGpuProcess();
     // Flush the UI message loop before quitting.
     while (delegate->DoWork()) { }
 
@@ -412,8 +442,7 @@ static void appendToFeatureSwitch(base::CommandLine *commandLine, const char *fe
 }
 
 WebEngineContext::WebEngineContext()
-    : m_threadedGpu(true)
-    , m_mainDelegate(new ContentMainDelegateQt)
+    : m_mainDelegate(new ContentMainDelegateQt)
     , m_globalQObject(new QObject())
 {
 #if defined(Q_OS_MACOS)
@@ -515,13 +544,13 @@ WebEngineContext::WebEngineContext()
     if (isDesktopGLOrSoftware || isGLES2Context)
         parsedCommandLine->AppendSwitch(switches::kDisableES3GLContext);
 #endif
-
+    bool threadedGpu = false;
 #ifndef QT_NO_OPENGL
-    m_threadedGpu = QOpenGLContext::supportsThreadedOpenGL();
+    threadedGpu = QOpenGLContext::supportsThreadedOpenGL();
 #endif
-    m_threadedGpu = m_threadedGpu && !qEnvironmentVariableIsSet(kDisableInProcGpuThread);
+    threadedGpu = threadedGpu && !qEnvironmentVariableIsSet(kDisableInProcGpuThread);
 
-    bool enableViz = ((m_threadedGpu && !parsedCommandLine->HasSwitch("disable-viz-display-compositor"))
+    bool enableViz = ((threadedGpu && !parsedCommandLine->HasSwitch("disable-viz-display-compositor"))
                       || parsedCommandLine->HasSwitch("enable-viz-display-compositor"));
     parsedCommandLine->RemoveSwitch("disable-viz-display-compositor");
     parsedCommandLine->RemoveSwitch("enable-viz-display-compositor");
@@ -660,7 +689,7 @@ WebEngineContext::WebEngineContext()
         parsedCommandLine->AppendSwitch(switches::kDisableGpu);
     }
 
-    registerMainThreadFactories(m_threadedGpu);
+    registerMainThreadFactories(threadedGpu);
 
     SetContentClient(new ContentClientQt);
 
diff --git a/src/core/web_engine_context.h b/src/core/web_engine_context.h
index edced9b42..ac0536596 100644
--- a/src/core/web_engine_context.h
+++ b/src/core/web_engine_context.h
@@ -129,9 +129,8 @@ private:
     ~WebEngineContext();
 
     static void registerMainThreadFactories(bool threaded);
-    static void destroyGpuProcess(bool threaded);
+    static void destroyGpuProcess();
 
-    bool m_threadedGpu;
     std::unique_ptr<base::RunLoop> m_runLoop;
     std::unique_ptr<ContentMainDelegateQt> m_mainDelegate;
     std::unique_ptr<content::ContentMainRunner> m_contentRunner;
diff --git a/src/core/web_engine_context_threads.cpp b/src/core/web_engine_context_threads.cpp
index f0f055676..c7440dd3f 100644
--- a/src/core/web_engine_context_threads.cpp
+++ b/src/core/web_engine_context_threads.cpp
@@ -92,20 +92,6 @@ struct GpuThreadControllerQt : content::GpuThreadController
         s_gpuProcess->set_main_thread(childThread);
     }
 
-    static void cleanupVizProcess()
-    {
-        auto gpuChildThread = content::GpuChildThread::instance();
-        if (!gpuChildThread)
-            return;
-        auto vizMain = gpuChildThread->viz_main();
-        auto vizCompositorThreadRunner = vizMain->viz_compositor_thread_runner();
-        if (!vizCompositorThreadRunner)
-            return;
-        QEventLoop loop;
-        vizCompositorThreadRunner->CleanupForShutdown(base::BindOnce(&QEventLoop::quit, base::Unretained(&loop)));
-        loop.exec();
-    }
-
     static void destroyGpuProcess()
     {
         DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -133,10 +119,8 @@ static std::unique_ptr<content::GpuThreadController> createGpuThreadController(
 }
 
 // static
-void WebEngineContext::destroyGpuProcess(bool threaded)
+void WebEngineContext::destroyGpuProcess()
 {
-    if (!threaded)
-        GpuThreadControllerQt::cleanupVizProcess();
     GpuThreadControllerQt::destroyGpuProcess();
 }