Crash in ActiveX browser plugin wrapper code

General JUCE discussion
1

Hi Jules!

The version of Juce is latest, pulled from Git repo.

While developing a Juce-based plugin for Internet Explorer I’ve found that plugin crashes when the plugin is unloaded from memory.
Debugging in WinDbg showed the place of a crash (symbols was forcefully loaded for unloaded .dll):

0:018> kb
ChildEBP RetAddr  Args to Child              
078cf818 5a28cf4d 00000032 0406a9d8 078cf874 npOurPlugin!juce::WaitableEvent::wait+0x21 
078cf828 5a2949c0 00000032 7bf12eee 078cf8c8 npOurPlugin!juce::Thread::wait+0x1d 
078cf874 5a28c5fe 7bf12e22 078cf8c8 078cf8d0 npOurPlugin!juce::InternalTimerThread::run+0x180 
078cf8b8 5a28c78c 0406a9d8 078cf8d8 5a341d77 npOurPlugin!juce::Thread::threadEntryPoint+0xee 
078cf8c4 5a341d77 0406a9d8 00000000 00000000 npOurPlugin!juce::juce_threadEntryPoint+0xc 
078cf8d8 5a4b6c73 0406a9d8 7bf12f8e 00000000 npOurPlugin!juce::threadEntryProc+0x47 
078cf914 5a4b6c14 0406c278 078cf92c 7701d0e9 npOurPlugin!_callthreadstartex+0x53 
078cf920 7701d0e9 0406c278 078cf96c 772b19bb npOurPlugin!_threadstartex+0xa4 
078cf92c 772b19bb 0406c038 7100f0c1 00000000 kernel32!BaseThreadInitThunk+0xe
078cf96c 772b198e 5a4b6b70 0406c038 00000000 ntdll!__RtlUserThreadStart+0x23
078cf984 00000000 5a4b6b70 0406c038 00000000 ntdll!_RtlUserThreadStart+0x1b

Additional investigations showed that it was caused by incorrect negative values of numActivePlugins global variable in juce_ActiveX_GlueCode.cpp.
In web page we use following widely-known logic to determine if the plugin is installed. It is written in VBScript:

    <script language='VBScript'>
      function DetectOurPluginActiveX
        on error resume next
        dim tControl
        dim res
        res = 0
        set tControl = CreateObject("www.OurPlugin.com.OurPlugin.1")
        if IsObject(tControl) then
          res = 1
        end if
        tControl = 0
        DetectOurPluginActiveX = res
      end function
    </script>

Following steps leads to crash:

[list]
[] VBScript code creates ActiveX object without calling of SetSite() method (or called with incorrect parameter, and thus numActivePlugins is decremented in deleteHolderComp()), and therefore createHolderComp() is not called[/]
[] Created by VBScript object is destroyed. numActivePlugins becomes less than 0.[/]
[] New ActiveX object is created by tag, with correctly called SetSite(). But juce GUI /nonGui is not initialized, because “if (numActivePlugins++ == 0)” condition in createHolderComp() is not passed through.[/]
[] On dll unloading condition “if (–numActivePlugins == 0)” is not passed through for the same reason[/][/list]

In result we have a crash after DllCanUnloadNow() and DllMain with DLL_PROCESS_DETACH was performed.

I’ve also successfully reproduced this issue with browser plugin demo from Juce sources.

Here’s my simple solution (in the form of git diff):

--- a/juce/extras/browser plugins/wrapper/juce_ActiveX_GlueCode.cpp	
+++ b/juce/extras/browser plugins/wrapper/juce_ActiveX_GlueCode.cpp	
@@ -571,7 +571,8 @@ static const String getExeVersion (const String& exeFileName, const String& fiel
     return resultString;
 }
 
-static int numActivePlugins = 0;
+CriticalSection numActivePluginsLock;
+volatile static int numActivePlugins = 0;
 
 class JuceActiveXObject     : public IUnknown,
                               public IDispatch,
@@ -633,6 +634,7 @@ public:
 
     HRESULT __stdcall SetSite (IUnknown* newSite)
     {
+        DBG("SetSite(" + String((uint64)newSite) + ")");
         if (newSite != site)
         {
             if (site != 0)
@@ -670,6 +672,8 @@ public:
 
     void createHolderComp()
     {
+        ScopedLock sl(numActivePluginsLock);
+
         if (numActivePlugins++ == 0)
         {
             log ("initialiseJuce_GUI()");
@@ -680,10 +684,14 @@ public:
 
         if (holderComp == 0)
             holderComp = new AXBrowserPluginHolderComponent();
+
+        DBG("numActivePlugins after createHolderComp(): " + String(numActivePlugins));
     }
 
     void deleteHolderComp()
     {
+        ScopedLock sl(numActivePluginsLock);
+
         deleteAndZero (holderComp);
 
         if (--numActivePlugins == 0)
@@ -691,6 +699,13 @@ public:
             log ("shutdownJuce_GUI()");
             shutdownJuce_GUI();
         }
+        else if(numActivePlugins < 0)
+        {
+            // preserve positive values
+            ++numActivePlugins;
+        }
+
+        DBG("numActivePlugins after deleteHolderComp(): " + String(numActivePlugins));
     }
 
     HRESULT __stdcall GetSite (REFIID riid, void **ppvSite)
@@ -859,7 +874,9 @@ STDAPI DllGetClassObject (REFCLSID rclsid, REFIID riid, LPVOID* ppv)
 STDAPI DllCanUnloadNow()
 {
     #pragma EXPORTED_FUNCTION
-    return S_OK;
+    ScopedLock sl(numActivePluginsLock);
+    log("DllCanUnloadNow()");
+    return numActivePlugins ? S_FALSE : S_OK;
 }
 
 //==============================================================================
2

Thanks for that detailed report!

Not sure about your solution though… It certainly looks like that counter variable will go wrong if SetSite is called with a null pointer, but surely all that’d be needed to fix it would be a couple of small tweaks to these methods:

[code] void createHolderComp()
{
if (holderComp == 0)
{
if (numActivePlugins++ == 0)
{
log (“initialiseJuce_GUI()”);
initialiseJuce_GUI();

            browserVersionDesc = "Internet Explorer " + getExeVersion (getExePath(), "FileVersion");
        }

        holderComp = new AXBrowserPluginHolderComponent();
    }
}

void deleteHolderComp()
{
    if (holderComp == 0)
    {
        deleteAndZero (holderComp);

        if (--numActivePlugins == 0)
        {
            log ("shutdownJuce_GUI()");
            shutdownJuce_GUI();
        }
    }
}

[/code]

?

And did you add the lock because you actually experienced threading problems? I’d have expected all the calls to be marshalled onto the same thread…?

3

Yeah, your fix would be much simpler. One note: this line in deleteHolderComp()

if (holderComp == 0)

should be

if (holderComp != 0)

Regarding locks, I just tried to ensure that it would be no problems if SetSite() and DllCanUnloadNow() are called from different threads.
I’ve just checked this out, and they are serialized to one thread indeed. So your fix will be enough.

Thanks.

4

Ah yes, thanks for pointing out my deliberate mistake there - I just added that to see if you were paying attention, of course… :oops: