Don't leave trash web processes when changing web entity urls

libraries/entities-renderer/src/RenderableWebEntityItem.cpp

@@ -46,6 +46,19 @@ static int YOUTUBE_MAX_FPS = 30;
 
 static QTouchDevice _touchDevice;
 
+WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& urlString) {
+    if (urlString.isEmpty()) {
+        return ContentType::NoContent;
+    }
+
+    const QUrl url(urlString);
+    if (url.scheme() == "http" || url.scheme() == "https" ||
+        urlString.toLower().endsWith(".htm") || urlString.toLower().endsWith(".html")) {
+        return ContentType::HtmlContent;
+    }
+    return ContentType::QmlContent;
+}
+
 WebEntityRenderer::WebEntityRenderer(const EntityItemPointer& entity) : Parent(entity) {
     static std::once_flag once;
     std::call_once(once, [&]{
@@ -123,13 +136,45 @@ void WebEntityRenderer::onTimeout() {
 }
 
 void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene, Transaction& transaction, const TypedEntityPointer& entity) {
-    withWriteLock([&] {
-        // This work must be done on the main thread
-        if (!hasWebSurface()) {
-            // If we couldn't create a new web surface, exit
-            if (!buildWebSurface(entity)) {
-                return;
+    // If the content type has changed, or the old content type was QML, we need to 
+    // destroy the existing surface (because surfaces don't support changing the root 
+    // object, so subsequent loads of content just overlap the existing content
+    bool urlChanged = false;
+    {
+        auto newSourceUrl = entity->getSourceUrl();
+        auto newContentType = getContentType(newSourceUrl);
+        auto currentContentType = ContentType::NoContent;
+        withReadLock([&] {
+            urlChanged = _lastSourceUrl != newSourceUrl;
+            currentContentType = _contentType;
+        });
+
+        if (urlChanged) {
+            if (newContentType != ContentType::HtmlContent || currentContentType != ContentType::HtmlContent) {
+                destroyWebSurface();
             }
+
+            withWriteLock([&] {
+                _lastSourceUrl = newSourceUrl;
+                _contentType = newContentType;
+            });
+        }
+    }
+
+
+    withWriteLock([&] {
+        if (_contentType == ContentType::NoContent) {
+            return;
+        }
+
+        // This work must be done on the main thread
+        // If we couldn't create a new web surface, exit
+        if (!hasWebSurface() && !buildWebSurface(entity)) {
+            return;
+        }
+        
+        if (urlChanged) {
+            _webSurface->getRootItem()->setProperty("url", _lastSourceUrl);
         }
 
         if (_contextPosition != entity->getWorldPosition()) {
@@ -138,11 +183,6 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene
             _webSurface->getSurfaceContext()->setContextProperty("globalPosition", vec3toVariant(_contextPosition));
         }
 
-        if (_lastSourceUrl != entity->getSourceUrl()) {
-            _lastSourceUrl = entity->getSourceUrl();
-            loadSourceURL();
-        }
-
         _lastDPI = entity->getDPI();
         _lastLocked = entity->getLocked();
 
@@ -232,9 +272,6 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) {
         // Let us interact with the keyboard
         surfaceContext->setContextProperty("tabletInterface", DependencyManager::get<TabletScriptingInterface>().data());
     });
-    _fadeStartTime = usecTimestampNow();
-    loadSourceURL();
-    _webSurface->resume();
 
     // forward web events to EntityScriptingInterface
     auto entities = DependencyManager::get<EntityScriptingInterface>();
@@ -243,6 +280,29 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) {
         emit entities->webEventReceived(entityItemID, message);
     });
 
+    if (_contentType == ContentType::HtmlContent) {
+        // We special case YouTube URLs since we know they are videos that we should play with at least 30 FPS.
+        // FIXME this doesn't handle redirects or shortened URLs, consider using a signaling method from the 
+        // web entity
+        if (QUrl(_lastSourceUrl).host().endsWith("youtube.com", Qt::CaseInsensitive)) {
+            _webSurface->setMaxFps(YOUTUBE_MAX_FPS);
+        } else {
+            _webSurface->setMaxFps(DEFAULT_MAX_FPS);
+        }
+        _webSurface->load("controls/WebEntityView.qml", [this](QQmlContext* context, QObject* item) {
+            item->setProperty("url", _lastSourceUrl);
+        });
+    } else if (_contentType == ContentType::QmlContent) {
+        _webSurface->load(_lastSourceUrl, [this](QQmlContext* context, QObject* item) {
+            if (item && item->objectName() == "tabletRoot") {
+                auto tabletScriptingInterface = DependencyManager::get<TabletScriptingInterface>();
+                tabletScriptingInterface->setQmlTabletRoot("com.highfidelity.interface.tablet.system", _webSurface.data());
+            }
+        });
+    }
+    _fadeStartTime = usecTimestampNow();
+    _webSurface->resume();
+
     return true;
 }
 
@@ -289,32 +349,6 @@ glm::vec2 WebEntityRenderer::getWindowSize(const TypedEntityPointer& entity) con
     return dims;
 }
 
-void WebEntityRenderer::loadSourceURL() {
-    const QUrl sourceUrl(_lastSourceUrl);
-    if (sourceUrl.scheme() == "http" || sourceUrl.scheme() == "https" ||
-        _lastSourceUrl.toLower().endsWith(".htm") || _lastSourceUrl.toLower().endsWith(".html")) {
-        _contentType = htmlContent;
-
-        // We special case YouTube URLs since we know they are videos that we should play with at least 30 FPS.
-        if (sourceUrl.host().endsWith("youtube.com", Qt::CaseInsensitive)) {
-            _webSurface->setMaxFps(YOUTUBE_MAX_FPS);
-        } else {
-            _webSurface->setMaxFps(DEFAULT_MAX_FPS);
-        }
-
-        _webSurface->load("controls/WebEntityView.qml", [this](QQmlContext* context, QObject* item) {
-            item->setProperty("url", _lastSourceUrl);
-        });
-    } else {
-        _contentType = qmlContent;
-        _webSurface->load(_lastSourceUrl);
-        if (_webSurface->getRootItem() && _webSurface->getRootItem()->objectName() == "tabletRoot") {
-            auto tabletScriptingInterface = DependencyManager::get<TabletScriptingInterface>();
-            tabletScriptingInterface->setQmlTabletRoot("com.highfidelity.interface.tablet.system", _webSurface.data());
-        }
-    }
-}
-
 void WebEntityRenderer::hoverEnterEntity(const PointerEvent& event) {
     if (!_lastLocked && _webSurface) {
         PointerEvent webEvent = event;

libraries/entities-renderer/src/RenderableWebEntityItem.h

@@ -47,15 +47,19 @@ private:
     bool buildWebSurface(const TypedEntityPointer& entity);
     void destroyWebSurface();
     bool hasWebSurface();
-    void loadSourceURL();
     glm::vec2 getWindowSize(const TypedEntityPointer& entity) const;
 
+
     int _geometryId{ 0 };
-    enum contentType {
-        htmlContent,
-        qmlContent
+    enum class ContentType {
+        NoContent,
+        HtmlContent,
+        QmlContent
     };
-    contentType _contentType;
+
+    static ContentType getContentType(const QString& urlString);
+
+    ContentType _contentType{ ContentType::NoContent };
     QSharedPointer<OffscreenQmlSurface> _webSurface;
     glm::vec3 _contextPosition;
     gpu::TexturePointer _texture;