Fix crash/hang on tab close + shutdown, and tutorial UI bugs (found in #416 review)

src/MainComponent.cpp

@@ -64,7 +64,8 @@ void MainComponent::paintOverChildren(Graphics& g)
             // Add extra highlights to the cutout path
             for (auto& rect : tutorialExtraHighlights)
             {
-                highlightPath.addRoundedRectangle(rect.toFloat(), 5.0f);
+                if (! rect.isEmpty())
+                    highlightPath.addRoundedRectangle(rect.toFloat(), 5.0f);
             }
 
             backgroundPath.setUsingNonZeroWinding(false);
@@ -73,11 +74,16 @@ void MainComponent::paintOverChildren(Graphics& g)
             g.fillPath(backgroundPath);
 
             g.setColour(Colours::white);
-            g.drawRoundedRectangle(tutorialHighlightRect.toFloat(), 5.0f, 2.0f);
+
+            // An empty rectangle is a region the current step has nothing to
+            // point at; outlining it would leave a stray mark in the corner
+            if (! tutorialHighlightRect.isEmpty())
+                g.drawRoundedRectangle(tutorialHighlightRect.toFloat(), 5.0f, 2.0f);
 
             for (auto& rect : tutorialExtraHighlights)
             {
-                g.drawRoundedRectangle(rect.toFloat(), 5.0f, 2.0f);
+                if (! rect.isEmpty())
+                    g.drawRoundedRectangle(rect.toFloat(), 5.0f, 2.0f);
             }
         }
     }
@@ -430,32 +436,63 @@ void MainComponent::setTutorialExtraHighlights(std::vector<Rectangle<int>> bound
 
 void MainComponent::ensureTutorialModelLoaded()
 {
+    // Loading is asynchronous, so without this guard every repeated call that
+    // arrives before the first load finishes - clicking Next again, say - would
+    // open yet another tab or start yet another load.
+    if (tutorialModelLoadInFlight)
+        return;
+
     auto* tab = getCurrentModelTab();
 
     if (tab == nullptr)
     {
+        // createNewTab() selects the tab it creates, which is what the tutorial
+        // steps compute their highlights against; leave it selected.
         tab = modelTabs.createNewTab();
-        modelTabs.setCurrentTabIndex(0);
-
-        if (welcomeWindow != nullptr)
-            tab->addChangeListener(welcomeWindow.get());
+        tutorialCreatedTab = tab;
+    }
 
-        if (tab != nullptr)
-            tab->loadDefaultModel();
+    if (tab->isModelLoaded())
         return;
-    }
 
-    if (! tab->isModelLoaded())
-        tab->loadDefaultModel();
+    tutorialModelLoadInFlight = true;
+
+    Component::SafePointer<MainComponent> safeThis(this);
+    tab->onNextModelLoadComplete(
+        [safeThis](ModelTab*, bool)
+        {
+            if (safeThis != nullptr)
+                safeThis->tutorialModelLoadInFlight = false;
+        });
+
+    tab->loadDefaultModel();
 }
 
 void MainComponent::resetTutorialAutoLoadedModel()
 {
-    if (auto* tab = getCurrentModelTab())
-    {
-        if (tab->isModelLoaded() && tab->getLoadedPath() == TutorialConstants::fallbackModelPath)
-            tab->resetState();
-    }
+    // Close the tab the tutorial opened on the user's behalf. Resetting it in
+    // place would leave a blank tab behind, since a model tab has no model
+    // selection of its own - models are chosen on the Home tab.
+    if (auto* tab = tutorialCreatedTab.getComponent())
+        modelTabs.closeTab(tab);
+
+    tutorialCreatedTab = nullptr;
+}
+
+void MainComponent::ensureMediaClipboardVisible()
+{
+    if (! showMediaClipboard)
+        viewMediaClipboardCallback();
+}
+
+Rectangle<int> MainComponent::getTabBarBounds()
+{
+    auto& tabBar = modelTabs.getTabbedButtonBar();
+
+    if (tabBar.getNumTabs() == 0)
+        return {};
+
+    return getLocalArea(&tabBar, tabBar.getLocalBounds());
 }
 
 Rectangle<int> MainComponent::getModelSelectBounds()
@@ -469,9 +506,14 @@ Rectangle<int> MainComponent::getModelSelectBounds()
     if (auto* tab = getCurrentModelTab())
     {
         auto bounds = tab->getModelSelectBounds();
-        return getLocalArea(tab, bounds);
+
+        if (! bounds.isEmpty())
+            return getLocalArea(tab, bounds);
     }
-    return {};
+
+    // Models are selected on the Home tab, so while a model tab is showing,
+    // point at the tab bar that leads back to it.
+    return getTabBarBounds();
 }
 
 Rectangle<int> MainComponent::getControlsBounds()
@@ -680,5 +722,10 @@ void MainComponent::changeListenerCallback(ChangeBroadcaster* source)
     if (source == &modelTabs)
     {
         updateWindowConstraints();
+
+        // Model tabs are created and closed while the tutorial is open, so it
+        // follows the container rather than subscribing to individual tabs.
+        if (welcomeWindow != nullptr)
+            welcomeWindow->notifyModelStateChanged();
     }
 }

src/MainComponent.h

@@ -79,13 +79,16 @@ class MainComponent : public Component,
     void setTutorialHighlight(Rectangle<int> bounds);
     void setTutorialExtraHighlights(std::vector<Rectangle<int>> bounds);
     void ensureTutorialModelLoaded();
+    bool isTutorialModelLoadInFlight() const { return tutorialModelLoadInFlight; }
     void resetTutorialAutoLoadedModel();
+    void ensureMediaClipboardVisible();
+
+    ModelTab* getCurrentModelTab() const;
+    ModelTab* getFirstModelTab() const;
 
-    ModelTab* getCurrentModelTab() const;
-    ModelTab* getFirstModelTab() const;
-
 
     // Bounds accessors for tutorial steps (public for WelcomeWindow)
+    Rectangle<int> getTabBarBounds();
     Rectangle<int> getModelSelectBounds();
     Rectangle<int> getControlsBounds();
     Rectangle<int> getInputTrackBounds();
@@ -129,7 +132,7 @@ class MainComponent : public Component,
 
     // Miscellaneous
     //void focusCallback();
-    void changeListenerCallback(ChangeBroadcaster* source) override;
+    void changeListenerCallback(ChangeBroadcaster* source) override;
 
     /* Interface */
 
@@ -158,12 +161,18 @@ class MainComponent : public Component,
     StatusAreaWidget statusAreaWidget;
     MediaClipboardWidget mediaClipboardWidget;
 
-    bool isTutorialActive = false;
-    Rectangle<int> tutorialHighlightRect;
-    std::vector<Rectangle<int>> tutorialExtraHighlights;
-    std::unique_ptr<WelcomeWindow> welcomeWindow;
-
-    SharedResourcePointer<SharedAPIKeys> sharedTokens;
+    bool isTutorialActive = false;
+    Rectangle<int> tutorialHighlightRect;
+    std::vector<Rectangle<int>> tutorialExtraHighlights;
+    std::unique_ptr<WelcomeWindow> welcomeWindow;
+
+    // Set while the tutorial's fallback model is loading, so that repeated
+    // requests to load it do not stack up. The tab the tutorial opened on the
+    // user's behalf is remembered so that it can be closed again at the end.
+    bool tutorialModelLoadInFlight = false;
+    Component::SafePointer<ModelTab> tutorialCreatedTab;
+
+    SharedResourcePointer<SharedAPIKeys> sharedTokens;
     SharedResourcePointer<StatusMessage> statusMessage;
 
     JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR(MainComponent)

src/Model.h

@@ -142,6 +142,10 @@ class Model
             return result;
         }
 
+        // Requests made while querying belong to this model as well, even though
+        // the client only replaces the current one once loading has succeeded
+        tempClient->setRequestRegistry(&requestRegistry);
+
         setStatus(ModelStatus::QUERYING_CONTROLS);
 
         // Initialize empty dictionary to hold query response
@@ -347,6 +351,13 @@ class Model
         return result;
     }
 
+    // Aborts any request this model currently has in flight, locally. Unlike
+    // cancel(), this needs neither the server nor a reachable network, and it
+    // unblocks the worker thread that is waiting on the connection. Once aborted
+    // the model will not open any further connections, so this is only for
+    // models that are being discarded (see ModelTab::abandon).
+    void abortActiveRequests() { requestRegistry.abortActiveRequests(); }
+
     OpResult cancel()
     {
         setStatus(ModelStatus::CANCELING);
@@ -617,6 +628,9 @@ class Model
 
     ModelStatus status;
 
+    // Declared before the client so that it is destroyed after it
+    RequestRegistry requestRegistry;
+
     std::unique_ptr<Client> client;
 
     String loadedPath;

src/ModelTab.h

@@ -72,7 +72,15 @@ class ModelTab : public Component, private ChangeListener, public ChangeBroadcas
     // Bounds accessors for tutorial steps
     Rectangle<int> getModelSelectBounds() const
     {
-        return modelSelectionWidget.getBounds().expanded(2, 2);
+        auto bounds = modelSelectionWidget.getBounds();
+
+        // The model browser lives on the Home tab, so this widget is laid out
+        // with an empty size here. Report nothing rather than a stray rectangle
+        // in the top left corner.
+        if (bounds.getWidth() > 0 && bounds.getHeight() > 0)
+            return bounds.expanded(2, 2);
+
+        return {};
     }
 
     Rectangle<int> getControlsBounds() const
@@ -117,6 +125,32 @@ class ModelTab : public Component, private ChangeListener, public ChangeBroadcas
 
     bool isModelLoaded() { return model->isLoaded(); }
 
+    // True while a model load or process request is still queued or running on
+    // one of this tab's thread pools. Used to defer destruction of the tab: if
+    // its ThreadPool is destroyed while a worker is blocked in a network call,
+    // JUCE force-kills the worker thread, which can corrupt the shared
+    // networking subsystem and hang all future requests.
+    bool hasPendingRequests() const
+    {
+        return loadingThreadPool.getNumJobs() > 0 || processingThreadPool.getNumJobs() > 0;
+    }
+
+    // Called once this tab has left the UI but cannot be destroyed yet because a
+    // request is still in flight. Aborts the connection locally so that the
+    // request settles within moments instead of whenever the server or the
+    // request timeout gets around to it - which may be long after the app has
+    // started shutting down, at which point delivering the result crashes. Any
+    // result that does still arrive is dropped, since the user closed this tab.
+    void abandon()
+    {
+        abandoned = true;
+
+        // Invalidate any in-flight jobs
+        ++currentProcessID;
+
+        model->abortActiveRequests();
+    }
+
     void resized() override
     {
         FlexBox tabArea;
@@ -426,6 +460,12 @@ class ModelTab : public Component, private ChangeListener, public ChangeBroadcas
                 MessageManager::callAsync(
                     [this, result]
                     {
+                        if (abandoned)
+                        {
+                            // Tab was closed while this load was in flight
+                            return;
+                        }
+
                         if (result.wasOk())
                         {
                             modelSelectionWidget.setSuccessfulState();
@@ -539,6 +579,12 @@ class ModelTab : public Component, private ChangeListener, public ChangeBroadcas
                 MessageManager::callAsync(
                     [this, result, outputFilesPtr, labelsPtr]
                     {
+                        if (abandoned)
+                        {
+                            // Tab was closed while this process was in flight
+                            return;
+                        }
+
                         std::function<void()> onExit = [this]
                         {
                             // Re-enable processing immediately
@@ -627,5 +673,6 @@ class ModelTab : public Component, private ChangeListener, public ChangeBroadcas
     ThreadPool processingThreadPool { 10 };
 
     std::atomic<uint64_t> currentProcessID { 0 };
+    std::atomic<bool> abandoned { false };
     std::function<void(ModelTab*, bool)> initialLoadCallback;
 };