pinkshirt / firebreath

Automatically exported from code.google.com/p/firebreath
0 stars 0 forks source link

New activeXObject() crash on start in IE #145

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
After fixing #issue 142 on the main branch, there is new crash issue. 

What steps will reproduce the problem?
1. create activeX control using new activexObject() as in #issue142
2. open and close the web page quickly several times
3. it will accidentally crash the IE (100% repeatable, try 4-5 times)

What version of FireBreath are you using? On what operating system and
browsers?
Latest git version.

What version of your compiler or IDE are you using?

Please provide any additional information below.

I am not sure about the cause, but I have debugged to the crash location in 
source code. 

The constructor of WinMessageWindow throws an exception. 

    if (!(clsAtom = ::RegisterClassEx(&wc))) {
            err = ::GetLastError();    
        }
    }
    // Step 2: Creating the Window
    HWND messageWin = CreateWindowEx(
        WS_OVERLAPPED,
        (LPCWSTR)clsAtom,
        wszWinName,
        0,
        0, 0, 0, 0,
        HWND_MESSAGE, NULL, gInstance, NULL);
    if (!messageWin) {
        err = ::GetLastError();
---->        throw std::runtime_error("Could not create Message Window");
    }
    m_hWnd = messageWin;
}

-----------------------
The err code returned by RegisterClassEx is 87(invalid parameter)
Up one stack level, it is in the function clientSiteSet
-----------------------
  STDMETHODIMP CFBControl<pFbCLSID, pMT,ICurObjInterface,piid,plibid>::SetSite(IUnknown *pUnkSite)
        {
            HRESULT hr = IObjectWithSiteImpl<CFBControl<pFbCLSID,pMT,ICurObjInterface,piid,plibid> >::SetSite(pUnkSite);
            if (!pUnkSite) {
                m_webBrowser.Release();
                m_serviceProvider.Release();
                if (m_host)
                    m_host->shutdown();
                m_host.reset();
                return hr;
            }

            m_serviceProvider = pUnkSite;
            if (!m_serviceProvider)
                return E_FAIL;
            m_serviceProvider->QueryService(SID_SWebBrowserApp, IID_IWebBrowser2, reinterpret_cast<void**>(&m_webBrowser));

            if (m_webBrowser) {
                m_propNotify = m_spClientSite;
            }

            // There will be no window this time!
----here ->            clientSiteSet();
            setReady();
            return S_OK;
        }
---------------------------
I guess we need to catch this exception somewhere or figure out why. 

Thanks,
Xizhi
--------------------------
the problem does not seem to exist prior to fixing #142

Original issue reported on code.google.com by xizhi...@gmail.com on 16 Feb 2011 at 3:06

GoogleCodeExporter commented 9 years ago
O, i am wrong in the previous post about error code, the actual return code of 
registerwindowex is ERROR_CLASS_ALREADY_EXISTS(1410). So the reason is obvious. 

Original comment by xizhi...@gmail.com on 16 Feb 2011 at 3:38

GoogleCodeExporter commented 9 years ago
interesting; I'll have to see if I can track this down.  Do you have a test 
page that makes it easy to reproduce?

Original comment by taxilian on 16 Feb 2011 at 4:28

GoogleCodeExporter commented 9 years ago
I think i know why. 
The microsoft doc 
(http://msdn.microsoft.com/en-us/library/ms633574%28v=vs.85%29.aspx)
says: 

The executable or DLL that registered the class is the owner of the class. The 
system determines class ownership from the hInstance member of the WNDCLASSEX 
structure passed to the RegisterClassEx function when the class is registered. 
For DLLs, the hInstance member must be the handle to the .dll instance.

The class is not destroyed when the .dll that owns it is unloaded. Therefore, 
if the system calls the window procedure for a window of that class, it will 
cause an access violation, because the .dll containing the window procedure is 
no longer in memory. The process must destroy all windows using the class 
before the .dll is unloaded and call the UnregisterClass function. 
--------------------------

Original comment by xizhi...@gmail.com on 16 Feb 2011 at 7:25

GoogleCodeExporter commented 9 years ago
The point is that for some reason, the window class is not unregistered. 
I have submitted a patch to fix this issue. 
---------------------------

  if (!(clsAtom = ::RegisterClassEx(&wc))) {
            err = ::GetLastError();    
---here catch the error and do not throw->          if(err != 
ERROR_CLASS_ALREADY_EXISTS) // fixed an IE Dll unload problem  #issue 145
            {
                throw std::runtime_error("Could not register window class");
            }
        }
    }
    // Step 2: Creating the Window
    HWND messageWin = CreateWindowEx(
        WS_OVERLAPPED,
----here do not use atom, use string instead ->        wszClassName, // 
(LPCWSTR)clsAtom, this fixed a bug when clsAtom is not available due to 
ERROR_CLASS_ALREADY_EXISTS.
        wszWinName,
        0,
        0, 0, 0, 0,
        HWND_MESSAGE, NULL, gInstance, NULL);

Original comment by xizhi...@gmail.com on 16 Feb 2011 at 7:36

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
I have tested the above fix code and the problem is gone. 
The cause perhaps is that when you force close an IE window tab, the dll is 
unloaded, but the registered window class is not. 

Original comment by xizhi...@gmail.com on 16 Feb 2011 at 7:40

GoogleCodeExporter commented 9 years ago
Fixed as you suggested.  Thanks!  This will be in RC1.

Original comment by richarda...@gmail.com on 17 Feb 2011 at 10:52