Merge pull request #12993 from highfidelity/revert-12983-fix/web_crash_66

Revert "HOTFIX version of PR 12969: Attempt to shutdown web surfaces more consistently"

interface/src/Application.cpp

@@ -4743,7 +4743,7 @@ void Application::updateLOD(float deltaTime) const {
     }
 }
 
-void Application::pushPostUpdateLambda(void* key, const std::function<void()>& func) {
+void Application::pushPostUpdateLambda(void* key, std::function<void()> func) {
     std::unique_lock<std::mutex> guard(_postUpdateLambdasLock);
     _postUpdateLambdas[key] = func;
 }
@@ -7346,7 +7346,7 @@ void Application::windowMinimizedChanged(bool minimized) {
     }
 }
 
-void Application::postLambdaEvent(const std::function<void()>& f) {
+void Application::postLambdaEvent(std::function<void()> f) {
     if (this->thread() == QThread::currentThread()) {
         f();
     } else {
@@ -7354,15 +7354,6 @@ void Application::postLambdaEvent(const 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,8 +136,7 @@ public:
     Application(int& argc, char** argv, QElapsedTimer& startup_time, bool runningMarkerExisted);
     ~Application();
 
-    void postLambdaEvent(const std::function<void()>& f) override;
+    void postLambdaEvent(std::function<void()> f) override;
-    void sendLambdaEvent(const std::function<void()>& f) override;
 
     QString getPreviousScriptLocation();
     void setPreviousScriptLocation(const QString& previousScriptLocation);
@@ -240,7 +239,7 @@ public:
 
     qint64 getCurrentSessionRuntime() const { return _sessionRunTimer.elapsed(); }
 
-    bool isAboutToQuit() const { return _aboutToQuit; }
+    bool isAboutToQuit() const override { 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 +263,7 @@ public:
     render::EnginePointer getRenderEngine() override { return _renderEngine; }
     gpu::ContextPointer getGPUContext() const { return _gpuContext; }
 
-    virtual void pushPostUpdateLambda(void* key, const std::function<void()>& func) override;
+    virtual void pushPostUpdateLambda(void* key, std::function<void()> func) override;
 
     void updateMyAvatarLookAtPosition();
 

interface/src/ui/overlays/Web3DOverlay.cpp

@@ -65,10 +65,14 @@ const QString Web3DOverlay::TYPE = "web3d";
 const QString Web3DOverlay::QML = "Web3DOverlay.qml";
 
 static auto qmlSurfaceDeleter = [](OffscreenQmlSurface* surface) {
-    AbstractViewStateInterface::instance()->sendLambdaEvent([surface] {
+    AbstractViewStateInterface::instance()->postLambdaEvent([surface] {
+        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 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,7 +45,6 @@ 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()) {
@@ -53,7 +52,7 @@ WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString&
     }
 
     const QUrl url(urlString);
-    if (url.scheme() == URL_SCHEME_HTTP || url.scheme() == URL_SCHEME_HTTPS ||
+    if (url.scheme() == "http" || url.scheme() == "https" ||
         urlString.toLower().endsWith(".htm") || urlString.toLower().endsWith(".html")) {
         return ContentType::HtmlContent;
     }
@@ -165,8 +164,6 @@ 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([&] {
@@ -188,8 +185,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene
             return;
         }
         
-        if (urlChanged && _contentType == ContentType::HtmlContent) {
+        if (urlChanged) {
-            _webSurface->getRootItem()->setProperty(URL_PROPERTY, _lastSourceUrl);
+            _webSurface->getRootItem()->setProperty("url", _lastSourceUrl);
         }
 
         if (_contextPosition != entity->getWorldPosition()) {
@@ -257,14 +254,6 @@ 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";
@@ -272,9 +261,20 @@ 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(), WebSurfaceDeleter);
+    _webSurface = QSharedPointer<OffscreenQmlSurface>(new OffscreenQmlSurface(), deleter);
     // 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_PROPERTY, _lastSourceUrl);
+            item->setProperty("url", _lastSourceUrl);
         });
     } else if (_contentType == ContentType::QmlContent) {
         _webSurface->load(_lastSourceUrl, [this](QQmlContext* context, QObject* item) {
@@ -327,11 +327,6 @@ 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

@@ -64,10 +64,14 @@ OffscreenSurface::OffscreenSurface()
 }
 
 OffscreenSurface::~OffscreenSurface() {
-    delete _sharedObject;
+    disconnect(qApp);
+    _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");
     }
 
-    _canvas.moveToThreadWithContext(targetThread);
     moveToThread(targetThread);
+    _canvas.moveToThreadWithContext(targetThread);
 }
 
 void RenderEventHandler::onInitalize() {
@@ -160,8 +160,11 @@ void RenderEventHandler::onQuit() {
     }
 
     _shared->shutdownRendering(_canvas, _currentSize);
-    _canvas.doneCurrent();
+    // Release the reference to the shared object.  This will allow it to 
-    _canvas.moveToThreadWithContext(qApp->thread());
+    // be destroyed (should happen on it's own thread).
-    moveToThread(qApp->thread());
+    _shared->deleteLater();
+
+    deleteLater();
+
     QThread::currentThread()->quit();
 }

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

@@ -72,35 +72,26 @@ SharedObject::SharedObject() {
     QObject::connect(qApp, &QCoreApplication::aboutToQuit, this, &SharedObject::onAboutToQuit);
 }
 
-
 SharedObject::~SharedObject() {
-    // After destroy returns, the rendering thread should be gone
-    destroy();
-
-    // _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) {
-        delete _renderControl;
-        _renderControl = nullptr;
-    }
-
     if (_quickWindow) {
         _quickWindow->destroy();
-        delete _quickWindow;
         _quickWindow = nullptr;
     }
 
-    // _rootItem is parented to the quickWindow, so needs no explicit destruction
+    if (_renderControl) {
-    //if (_rootItem) {
+        _renderControl->deleteLater();
-    //    delete _rootItem;
+        _renderControl = nullptr;
-    //    _rootItem = nullptr;
+    }
-    //}
+
+    if (_renderThread) {
+        _renderThread->quit();
+        _renderThread->deleteLater();
+    }
+
+    if (_rootItem) {
+        _rootItem->deleteLater();
+        _rootItem = nullptr;
+    }
 
     releaseEngine(_qmlContext->engine());
 }
@@ -125,10 +116,6 @@ void SharedObject::create(OffscreenSurface* surface) {
 }
 
 void SharedObject::setRootItem(QQuickItem* rootItem) {
-    if (_quit) {
-        return;
-    }
-
     _rootItem = rootItem;
     _rootItem->setSize(_quickWindow->size());
 
@@ -137,6 +124,7 @@ 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));
@@ -146,43 +134,35 @@ 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();
     }
 
-    if (_renderControl) {
     QObject::disconnect(_renderControl);
-    }
-
     QObject::disconnect(qApp);
 
     {
         QMutexLocker lock(&_mutex);
         _quit = true;
-        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
-    if (_renderThread) {
     _renderThread->wait();
-        delete _renderThread;
-        _renderThread = nullptr;
-    }
 }
 
 

libraries/render-utils/src/AbstractViewStateInterface.h

@@ -37,25 +37,15 @@ public:
 
     virtual glm::vec3 getAvatarPosition() const = 0;
 
-    // Unfortunately, having this here is a bad idea.  Lots of objects connect to 
+    virtual bool isAboutToQuit() const = 0;
-    // the aboutToQuit signal, and it's impossible to know the order in which 
+    virtual void postLambdaEvent(std::function<void()> f) = 0;
-    // 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, const std::function<void()>& func) = 0;
+    virtual void pushPostUpdateLambda(void* key, std::function<void()> func) = 0;
 
     virtual bool isHMDMode() const = 0;
 
@@ -64,4 +54,5 @@ public:
     static void setInstance(AbstractViewStateInterface* instance);
 };
 
+
 #endif // hifi_AbstractViewStateInterface_h

tests/render-perf/src/main.cpp

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