Crash Deleting Windows since addition of isWindowOnCurrentVirtualDesktop

We’ve discovered an issue with a recent change in JUCE which is causing crashes when hiding AlertWindows (and presumably other windows with drop shadows).

Here’s the stack trace:

I think what’s happening is:
• The AlertWindow is being deleted
• Which calls updateShadows
• Which calls isWindowOnCurrentVirtualDesktop
• Which for some reason calls a synchronous paint call
• Which calls in to the partially deleted Component
• Which crashes

Is there a way to avoid the paint callback or updating the shadows during destruction?
It might well be one of the children of the AlertWindow being deleted that’s triggering the repaint of the parent.

I’m unable to reproduce the crash using the DialogsDemo and opening an alert window but I think I can see what’s going wrong. Can you see if the following patch fixes the crash in your case?

0001.patch (940 Bytes)

That didn’t seem to fix it.
What was the logic in that patch? I thought this was a JUCE window we’re dealing with so bailing out if it’s not wouldn’t have any effect?

We’re trying to mock up an example that shows it in the JUCE demo.

Ah ok, thanks for testing.

In the HWNDComponentPeer destructor the first line resets the HWND identifier so isJUCEWindow() will return false and messages won’t be processed for the window whilst it is being destroyed. My understanding was that the crash was due to messages arriving for the partially destroyed peer so adding that check would avoid calling IsWindowOnCurrentVirtualDesktop() in that situation.

An example with one of the JUCE demos would be really helpful in tracking this down, thanks!

The other case is different though, it doesn’t involve destruction of the window. It’s that seemingly the dpi change involves two messages, a setting change and then a dpi change. doSettingChange, through forceDisplayUpdate, ends up calling updateShadows, but then somehow IsWindowOnCurrentVirtualDesktop makes the dpi change be delivered, and handleDPIChanging resets the shadower while it was still being updated for the setting change before. I’ve been scratching my head for a couple hours but I can’t think of anything that would cover all cases…

Ok, trying this again… Can you try this patch? We can temporarily disable message handling whilst inside the IsWindowOnCurrentVirtualDesktop() call which I think should fix things.

0001.patch (2.8 KB)

Hmm, that seems to just kill the message loop.
We have some pretty old code on init that shows our splash screen and then after a new status text is set, pumps the message loop until the repaint happens.

I know this is all kinds of dodgy but it’s a bit difficult to change right now.
With this patch applied, the runDispatchLoopUntil just blocks forever. I’m guessing because the repaint call either never gets posted or gets purged when the ScopedMessageDisabler is constructed.

What was this IsWindowOnCurrentVirtualDesktop added for?

That’s odd, it should only prevent the window messages from being dispatched whilst the ScopedMessageDisabler is in scope so we will just ignore window messages dispatched during the IsWindowOnCurrentVirtualDesktop() call - is that not what you are seeing?

No, it just stops the message loop it looks like…

Here’s some code that replicates the issue. In the JUCE “Windows” Demo, in the showAllWindows() function replace the existing window calls to showAlertWindow() and define it as this:

{
    String title("ok");
    bool savePreview = true;

    const auto w = std::make_unique<AlertWindow>("Title", "Message", AlertWindow::InfoIcon, nullptr);

    w->addTextEditor("name", "Name");
    w->addTextBlock(TRANS("An optional description of the preset") + ":");
    w->addTextBlock(TRANS("Add any number of tags separated by commas") + ":");
    w->addTextEditor("tags", "example_tags");

    std::unique_ptr<ToggleButton> savePreviewButton;

    if (savePreview)
    {
        savePreviewButton = std::make_unique<ToggleButton>(TRANS("Save audio preview"));
        savePreviewButton->setVisible(true);
        savePreviewButton->setName("");
        savePreviewButton->setSize(150, 20);
        savePreviewButton->setToggleState(true, dontSendNotification);
        w->addCustomComponent(savePreviewButton.get());
    }

    w->addButton(TRANS("OK"), 1, KeyPress(KeyPress::returnKey));
    w->addButton(TRANS("Cancel"), 2, KeyPress(KeyPress::escapeKey));

    w->runModalLoop();
}

(It’s a bit odd but was snipped from some of our code).
You’ll also need to enabled modal loops.

After you press OK, you’ll see the crash.

Thanks. With that example and my above patch applied I’m no longer seeing the crash. I’m also not seeing the message loop hang - if I put the following lines after w->runModalLoop(); the DBG statement prints:

MessageManager::getInstance()->runDispatchLoopUntil (500);
DBG ("done");

It’s possible that the message loop issue is related to another bug in the DropShadower causing IsWindowOnCurrentVirtualDesktop() to be called on a timer continuously. Can you apply this patch containing the additional fix and see if that sorts it? Appreciate the debugging help!

0002.patch (4.8 KB)

So that fixes the crash but not the hang I’m seeing.
Here’s an example of that if you replace the Main.cpp file of the JUCE DemoRunner app. You’ll see once the repaint is called, it never actually triggers a repaint.

Main.cpp (8.6 KB)

I’m able to reproduce the hang using your code, but it’s present on both develop and master without any of my above changes so I don’t think those are the cause. It occurs without the IsWindowOnCurrentVirtualDesktop() changes too. Which version of JUCE are you using where the splash screen works as expected?

I was also seeing dropped repaints with the second patch. Just in case it helps, this is my try.

(Basically, isolating the call to IsWindowOnCurrentVirtualDesktop() in the timer callback prevents the ComponentListener callbacks from reentering windowProc. The rest is secondary, I just thought it wasn’t necessary to check for childrenChanged -maybe I’m missing something there.)

Ok, thanks for pointing that out. There was a small mistake in my example (too much hack and slash). The calls to waitForRepaint in the app startup code shouldn’t be there. That is called automatically by setStartupMessage.
Try with this:
Main.cpp (8.5 KB)

That works with b4bc2c8 (tip of develop) with your 0002.patch applied.

The problem is, this still doesn’t work in Waveform.
I have a feeling there’s something else going on during our startup process which is causing the repaint messages to be lost. Trying to debug that now.

I can’t figure out the root of this problem, working on my Windows VM debugging it is just taking too long. I have sidestepped the problem by setting setDropShadowEnabled (false); on our splash screen for now.

It does seem like there are situations where repaint messages are getting dropped though…

If you apply your patch to develop at least I should be able to get a release out.

repaints
Those lines are dropped repaints from the 2nd patch. IsWindowOnCurrentVirtualDesktop() is still called on every timer callback, I don’t see a way to avoid that.

My previous try still crashed, only more occasionally. The only fix I can think of is a WeakReference to this. This version is less invasive, just addressing this issue. I think it still shouldn’t crash in Dave’s case, as the ChildrenChanged callback doesn’t call IsWindowOnCurrentVirtualDesktop(). In the meanwhile I’ll be using the more thorough edit, which seems to work and is much cleaner.

Not sure if it’s related but I’m seeing occasional crashes here when comp is nullptr.

class DropShadower::ShadowWindow  : public Component
{
public:
    ShadowWindow (Component* comp, const DropShadow& ds)
        : target (comp), shadow (ds)
    {
        setVisible (true);
        setAccessible (false);
        setInterceptsMouseClicks (false, false);

        if (comp->isOnDesktop())

Do you have a stack trace? I can’t see how we would get into that situation since the construction of the ShadowWindow objects is done inside a block that checks the owner weak ref isn’t null.

I haven’t seen it again, maybe it was resolved by a clean and rebuild? I will post if I see it again.

I’ve pushed a fix for the original issue to develop here:

Please let me know if you’re still experiencing issues with this commit applied.