Merge pull request #12969 from jherico/fix/web_crash

Attempt to shutdown web surfaces more consistently

interface/src/Application.cpp

@@ -4741,7 +4741,7 @@ void Application::updateLOD(float deltaTime) const {
     }
 }
 
-void Application::pushPostUpdateLambda(void* key, std::function<void()> func) {
+void Application::pushPostUpdateLambda(void* key, const std::function<void()>& func) {
     std::unique_lock<std::mutex> guard(_postUpdateLambdasLock);
     _postUpdateLambdas[key] = func;
 }
@@ -7351,7 +7351,7 @@ void Application::windowMinimizedChanged(bool minimized) {
     }
 }
 
-void Application::postLambdaEvent(std::function<void()> f) {
+void Application::postLambdaEvent(const std::function<void()>& f) {
     if (this->thread() == QThread::currentThread()) {
         f();
     } else {
@@ -7359,6 +7359,15 @@ void Application::postLambdaEvent(std::function<void()> f) {
     }
 }
 
+void Application::sendLambdaEvent(const std::function<void()>& f) {
+    if (this->thread() == QThread::currentThread()) {
+        f();
+    } else {
+        LambdaEvent event(f);
+        QCoreApplication::sendEvent(this, &event);
+    } 
+}
+
 void Application::initPlugins(const QStringList& arguments) {
     QCommandLineOption display("display", "Preferred displays", "displays");
     QCommandLineOption disableDisplays("disable-displays", "Displays to disable", "displays");

interface/src/Application.h

@@ -136,7 +136,8 @@ public:
     Application(int& argc, char** argv, QElapsedTimer& startup_time, bool runningMarkerExisted);
     ~Application();
 
-    void postLambdaEvent(std::function<void()> f) override;
+    void postLambdaEvent(const std::function<void()>& f) override;
+    void sendLambdaEvent(const std::function<void()>& f) override;
 
     QString getPreviousScriptLocation();
     void setPreviousScriptLocation(const QString& previousScriptLocation);
@@ -240,7 +241,7 @@ public:
 
     qint64 getCurrentSessionRuntime() const { return _sessionRunTimer.elapsed(); }
 
-    bool isAboutToQuit() const override { return _aboutToQuit; }
+    bool isAboutToQuit() const { return _aboutToQuit; }
     bool isPhysicsEnabled() const { return _physicsEnabled; }
 
     // the isHMDMode is true whenever we use the interface from an HMD and not a standard flat display
@@ -264,7 +265,7 @@ public:
     render::EnginePointer getRenderEngine() override { return _renderEngine; }
     gpu::ContextPointer getGPUContext() const { return _gpuContext; }
 
-    virtual void pushPostUpdateLambda(void* key, std::function<void()> func) override;
+    virtual void pushPostUpdateLambda(void* key, const std::function<void()>& func) override;
 
     void updateMyAvatarLookAtPosition();
 

interface/src/ui/overlays/Web3DOverlay.cpp

@@ -65,14 +65,10 @@ const QString Web3DOverlay::TYPE = "web3d";
 const QString Web3DOverlay::QML = "Web3DOverlay.qml";
 
 static auto qmlSurfaceDeleter = [](OffscreenQmlSurface* surface) {
-    AbstractViewStateInterface::instance()->postLambdaEvent([surface] {
+    AbstractViewStateInterface::instance()->sendLambdaEvent([surface] {
-        if (AbstractViewStateInterface::instance()->isAboutToQuit()) {
+        // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown
-            // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown
+        // if the application has already stopped its event loop, delete must be explicit
-            // if the application has already stopped its event loop, delete must be explicit
+        delete surface;
-            delete surface;
-        } else {
-            surface->deleteLater();
-        }
     });
 };
 

libraries/entities-renderer/src/RenderableWebEntityItem.cpp

@@ -25,7 +25,7 @@
 #include <EntityScriptingInterface.h>
 
 #include "EntitiesRendererLogging.h"
-
+#include <NetworkingConstants.h>
 
 using namespace render;
 using namespace render::entities;
@@ -45,6 +45,7 @@ static int DEFAULT_MAX_FPS = 10;
 static int YOUTUBE_MAX_FPS = 30;
 
 static QTouchDevice _touchDevice;
+static const char* URL_PROPERTY = "url";
 
 WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& urlString) {
     if (urlString.isEmpty()) {
@@ -52,7 +53,7 @@ WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString&
     }
 
     const QUrl url(urlString);
-    if (url.scheme() == "http" || url.scheme() == "https" ||
+    if (url.scheme() == URL_SCHEME_HTTP || url.scheme() == URL_SCHEME_HTTPS ||
         urlString.toLower().endsWith(".htm") || urlString.toLower().endsWith(".html")) {
         return ContentType::HtmlContent;
     }
@@ -164,6 +165,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene
         if (urlChanged) {
             if (newContentType != ContentType::HtmlContent || currentContentType != ContentType::HtmlContent) {
                 destroyWebSurface();
+                // If we destroyed the surface, the URL change will be implicitly handled by the re-creation
+                urlChanged = false;
             }
 
             withWriteLock([&] {
@@ -185,8 +188,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene
             return;
         }
         
-        if (urlChanged) {
+        if (urlChanged && _contentType == ContentType::HtmlContent) {
-            _webSurface->getRootItem()->setProperty("url", _lastSourceUrl);
+            _webSurface->getRootItem()->setProperty(URL_PROPERTY, _lastSourceUrl);
         }
 
         if (_contextPosition != entity->getWorldPosition()) {
@@ -254,6 +257,14 @@ bool WebEntityRenderer::hasWebSurface() {
     return (bool)_webSurface && _webSurface->getRootItem();
 }
 
+static const auto WebSurfaceDeleter = [](OffscreenQmlSurface* webSurface) {
+    AbstractViewStateInterface::instance()->sendLambdaEvent([webSurface] {
+        // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown
+        // if the application has already stopped its event loop, delete must be explicit
+        delete webSurface;
+    });
+};
+
 bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) {
     if (_currentWebCount >= MAX_CONCURRENT_WEB_VIEWS) {
         qWarning() << "Too many concurrent web views to create new view";
@@ -261,20 +272,9 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) {
     }
 
     ++_currentWebCount;
-    auto deleter = [](OffscreenQmlSurface* webSurface) {
-        AbstractViewStateInterface::instance()->postLambdaEvent([webSurface] {
-            if (AbstractViewStateInterface::instance()->isAboutToQuit()) {
-                // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown
-                // if the application has already stopped its event loop, delete must be explicit
-                delete webSurface;
-            } else {
-                webSurface->deleteLater();
-            }
-        });
-    };
 
     // FIXME use the surface cache instead of explicit creation
-    _webSurface = QSharedPointer<OffscreenQmlSurface>(new OffscreenQmlSurface(), deleter);
+    _webSurface = QSharedPointer<OffscreenQmlSurface>(new OffscreenQmlSurface(), WebSurfaceDeleter);
     // FIXME, the max FPS could be better managed by being dynamic (based on the number of current surfaces
     // and the current rendering load)
     _webSurface->setMaxFps(DEFAULT_MAX_FPS);
@@ -302,7 +302,7 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) {
             _webSurface->setMaxFps(DEFAULT_MAX_FPS);
         }
         _webSurface->load("controls/WebEntityView.qml", [this](QQmlContext* context, QObject* item) {
-            item->setProperty("url", _lastSourceUrl);
+            item->setProperty(URL_PROPERTY, _lastSourceUrl);
         });
     } else if (_contentType == ContentType::QmlContent) {
         _webSurface->load(_lastSourceUrl, [this](QQmlContext* context, QObject* item) {
@@ -327,6 +327,11 @@ void WebEntityRenderer::destroyWebSurface() {
     if (webSurface) {
         --_currentWebCount;
         QQuickItem* rootItem = webSurface->getRootItem();
+        // Explicitly set the web URL to an empty string, in an effort to get a 
+        // faster shutdown of any chromium processes interacting with audio
+        if (rootItem && _contentType == ContentType::HtmlContent) {
+            rootItem->setProperty(URL_PROPERTY, "");
+        }
 
         if (rootItem && rootItem->objectName() == "tabletRoot") {
             auto tabletScriptingInterface = DependencyManager::get<TabletScriptingInterface>();

libraries/qml/src/qml/OffscreenSurface.cpp

@@ -66,14 +66,10 @@ OffscreenSurface::OffscreenSurface()
 }
 
 OffscreenSurface::~OffscreenSurface() {
-    disconnect(qApp);
+    delete _sharedObject;
-    _sharedObject->destroy();
 }
 
 bool OffscreenSurface::fetchTexture(TextureAndFence& textureAndFence) {
-    if (!_sharedObject) {
-        return false;
-    }
     hifi::qml::impl::TextureAndFence typedTextureAndFence;
     bool result = _sharedObject->fetchTexture(typedTextureAndFence);
     textureAndFence = typedTextureAndFence;

libraries/qml/src/qml/impl/RenderEventHandler.cpp

@@ -49,8 +49,8 @@ RenderEventHandler::RenderEventHandler(SharedObject* shared, QThread* targetThre
         qFatal("Unable to create new offscreen GL context");
     }
 
-    moveToThread(targetThread);
     _canvas.moveToThreadWithContext(targetThread);
+    moveToThread(targetThread);
 }
 
 void RenderEventHandler::onInitalize() {
@@ -160,11 +160,8 @@ void RenderEventHandler::onQuit() {
     }
 
     _shared->shutdownRendering(_canvas, _currentSize);
-    // Release the reference to the shared object.  This will allow it to 
+    _canvas.doneCurrent();
-    // be destroyed (should happen on it's own thread).
+    _canvas.moveToThreadWithContext(qApp->thread());
-    _shared->deleteLater();
+    moveToThread(qApp->thread());
-
-    deleteLater();
-
     QThread::currentThread()->quit();
 }

libraries/qml/src/qml/impl/SharedObject.cpp

@@ -72,26 +72,35 @@ SharedObject::SharedObject() {
     QObject::connect(qApp, &QCoreApplication::aboutToQuit, this, &SharedObject::onAboutToQuit);
 }
 
+
 SharedObject::~SharedObject() {
-    if (_quickWindow) {
+    // After destroy returns, the rendering thread should be gone
-        _quickWindow->destroy();
+    destroy();
-        _quickWindow = nullptr;
+
+    // _renderTimer is created with `this` as the parent, so need no explicit destruction
+
+    // Destroy the event hand
+    if (_renderObject) {
+        delete _renderObject;
+        _renderObject = nullptr;
     }
 
     if (_renderControl) {
-        _renderControl->deleteLater();
+        delete _renderControl;
         _renderControl = nullptr;
     }
 
-    if (_renderThread) {
+    if (_quickWindow) {
-        _renderThread->quit();
+        _quickWindow->destroy();
-        _renderThread->deleteLater();
+        delete _quickWindow;
+        _quickWindow = nullptr;
     }
 
-    if (_rootItem) {
+    // _rootItem is parented to the quickWindow, so needs no explicit destruction
-        _rootItem->deleteLater();
+    //if (_rootItem) {
-        _rootItem = nullptr;
+    //    delete _rootItem;
-    }
+    //    _rootItem = nullptr;
+    //}
 
     releaseEngine(_qmlContext->engine());
 }
@@ -119,6 +128,10 @@ void SharedObject::create(OffscreenSurface* surface) {
 }
 
 void SharedObject::setRootItem(QQuickItem* rootItem) {
+    if (_quit) {
+        return;
+    }
+
     _rootItem = rootItem;
     _rootItem->setSize(_quickWindow->size());
 
@@ -127,7 +140,6 @@ void SharedObject::setRootItem(QQuickItem* rootItem) {
     _renderThread->setObjectName(objectName());
     _renderThread->start();
 
-
     // Create event handler for the render thread
     _renderObject = new RenderEventHandler(this, _renderThread);
     QCoreApplication::postEvent(this, new OffscreenEvent(OffscreenEvent::Initialize));
@@ -137,35 +149,43 @@ void SharedObject::setRootItem(QQuickItem* rootItem) {
 }
 
 void SharedObject::destroy() {
+    // `destroy` is idempotent, it can be called multiple times without issues
     if (_quit) {
         return;
     }
 
     if (!_rootItem) {
-        deleteLater();
         return;
     }
 
-
     _paused = true;
     if (_renderTimer) {
+        _renderTimer->stop();
         QObject::disconnect(_renderTimer);
-        _renderTimer->deleteLater();
     }
 
-    QObject::disconnect(_renderControl);
+    if (_renderControl) {
+        QObject::disconnect(_renderControl);
+    }
+
     QObject::disconnect(qApp);
 
     {
         QMutexLocker lock(&_mutex);
         _quit = true;
-        QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority);
+        if (_renderObject) {
+            QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority);
+        }
     }
     // Block until the rendering thread has stopped
     // FIXME this is undesirable because this is blocking the main thread,
     // but I haven't found a reliable way to do this only at application 
     // shutdown
-    _renderThread->wait();
+    if (_renderThread) {
+        _renderThread->wait();
+        delete _renderThread;
+        _renderThread = nullptr;
+    }
 }
 
 

libraries/render-utils/src/AbstractViewStateInterface.h

@@ -37,15 +37,25 @@ public:
 
     virtual glm::vec3 getAvatarPosition() const = 0;
 
-    virtual bool isAboutToQuit() const = 0;
+    // Unfortunately, having this here is a bad idea.  Lots of objects connect to 
-    virtual void postLambdaEvent(std::function<void()> f) = 0;
+    // the aboutToQuit signal, and it's impossible to know the order in which 
+    // the receivers will be called, so this might return false negatives
+    //virtual bool isAboutToQuit() const = 0;
+
+    // Queue code to execute on the main thread. 
+    // If called from the main thread, the lambda will execute synchronously
+    virtual void postLambdaEvent(const std::function<void()>& f) = 0;
+    // Synchronously execute code on the main thread.  This function will
+    // not return until the code is executed, regardles of which thread it 
+    // is called from
+    virtual void sendLambdaEvent(const std::function<void()>& f) = 0;
 
     virtual qreal getDevicePixelRatio() = 0;
 
     virtual render::ScenePointer getMain3DScene() = 0;
     virtual render::EnginePointer getRenderEngine() = 0;
 
-    virtual void pushPostUpdateLambda(void* key, std::function<void()> func) = 0;
+    virtual void pushPostUpdateLambda(void* key, const std::function<void()>& func) = 0;
 
     virtual bool isHMDMode() const = 0;
 
@@ -54,5 +64,4 @@ public:
     static void setInstance(AbstractViewStateInterface* instance);
 };
 
-
+#endif  // hifi_AbstractViewStateInterface_h
-#endif // hifi_AbstractViewStateInterface_h

tests/render-perf/src/main.cpp

@@ -453,8 +453,8 @@ protected:
         return vec3();
     }
 
-    bool isAboutToQuit() const override { return false; }
+    void postLambdaEvent(const std::function<void()>& f) override {}
-    void postLambdaEvent(std::function<void()> f) override {}
+    void sendLambdaEvent(const std::function<void()>& f) override {}
 
     qreal getDevicePixelRatio() override {
         return 1.0f;
@@ -469,7 +469,7 @@ protected:
     }
 
     std::map<void*, std::function<void()>> _postUpdateLambdas;
-    void pushPostUpdateLambda(void* key, std::function<void()> func) override {
+    void pushPostUpdateLambda(void* key, const std::function<void()>& func) override {
         _postUpdateLambdas[key] = func;
     }