Issue #1173 - add exception handler to CPluginClass::Invoke

src/plugin/PluginClass.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginClass.h"
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #ifdef SUPPORT_FILTER
 #include "PluginFilter.h"
 #endif
 #include "PluginMimeFilterClient.h"
 #include "PluginClient.h"
@@ skipping 376 matching lines @@
     {
       s_instances.erase(this);
 
       std::map<DWORD,CPluginClass*>::iterator it = s_threadInstances.find(::GetCurrentThreadId());
       if (it != s_threadInstances.end())
       {
         s_threadInstances.erase(it);
       }
       if (s_instances.empty())
       {
-        // AUDIT: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr

[Oleksandr, 2014/08/21 14:49:56]

I think it's better to create an issue then to put this comment into the code.

[Eric, 2014/09/29 19:16:44]

Well, I don't know what the issue would be yet; I haven't spent enough time
analyzing the code to know everything that's involved here. Is it better to have
neither comment nor issue on this?

[Oleksandr, 2014/10/02 20:44:16]

How about TODO: then?

On 2014/09/29 19:16:44, Eric wrote:

[Eric, 2014/10/02 23:11:02]

OK.

+        // TODO: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr
         CPluginClientFactory::ReleaseMimeFilterClientInstance();
       }
     }
     s_criticalSectionLocal.Unlock();
 
     // Release browser interface
     s_criticalSectionBrowser.Lock();
     {
       m_webBrowser2.Release();
     }
@@ skipping 117 matching lines @@
       DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_STATUSBAR, "Class::Get statusbar state");
     }
   }
   DEBUG_GENERAL("ShowStatusBar end");
 }
 
 /*
  * #1163 This class is the implementation for method DISPID_BEFORENAVIGATE2 in CPluginClass::Invoke.
  * - It validates and convertes its own arguments, rather than unifying them in the Invoke body.
  * - It's declared void and not HRESULT, so DISPID_BEFORENAVIGATE2 can only return S_OK.
  */

[Oleksandr, 2014/08/21 14:49:56]

All the comments in this file that start with #1163 are redundant here, IMHO. I
don't any of them adds in any way to readability of the code.

[Eric, 2014/09/29 19:16:44]

These comments are intended to exist only while #1163 is being worked on,
because it's not an issue that can get solved with a single review. These
comments essentially record a work-in-progress.

I don't intend them to be permanent. They'll all go as #1163 gets finished.

[Oleksandr, 2014/10/02 20:44:16]

ok, as long as you'll be able to keep track of them :)

On 2014/09/29 19:16:44, Eric wrote:

[Eric, 2014/10/02 23:11:02]

No problem. That's what grep is for.

 void CPluginClass::BeforeNavigate2(DISPPARAMS* pDispParams)
 {
 
   if (pDispParams->cArgs < 7)
   {
     return;
   }
   //Register a mime filter if it's not registered yet
   if (s_mimeFilter == NULL)
   {
@@ skipping 79 matching lines @@
         UpdateStatusBar();
       }
     }
   }
   notificationMessage.Hide();
   DEBUG_GENERAL("Tab change end");
   return S_OK;
 }
 
 // This gets called whenever there's a browser event
+// ENTRY POINT
 STDMETHODIMP CPluginClass::Invoke(DISPID dispidMember, REFIID riid, LCID lcid, WORD wFlags, DISPPARAMS* pDispParams, VARIANT* pvarResult, EXCEPINFO* pExcepInfo, UINT* puArgErr)
 {
-  // Entry point exception handler

[Oleksandr, 2014/08/21 14:49:56]

Self-evident, IMHO

[Eric, 2014/09/29 19:16:44]

I mentioned this in another review, but I'd like to have some way of using
'grep' to find all the entry points. Suggestions welcome.

[Eric, 2014/10/02 23:11:02]

Per a suggestion in another review, I'm changing this simply to mark this
function as an entry point.

   try
   {
     WCHAR tmp[256];
     wsprintf(tmp, L"Invoke: %d\n", dispidMember);
     DEBUG_GENERAL(tmp);
     switch (dispidMember)
     {
     case DISPID_WINDOWSTATECHANGED:
       {
         // #1163 should validate and convert arguments here
@@ skipping 121 matching lines @@
       /*
        * Ordinarily a method not dispatched should return DISP_E_MEMBERNOTFOUND.
        * As a conservative initial change, we leave it behaving as before,
        *   which is to do nothing and return S_OK.
        */
       // do nothing
       break;
     }
   }
   catch(...)
   {

[Oleksandr, 2014/08/21 14:49:56]

Maybe let's log at least something first

[Eric, 2014/09/29 19:16:44]

We could. Got a suggestion? Right now we've got no general-purpose log message
for unknown exceptions. There's not a lot to say since we don't have an
exception object. Is just the function name sufficient?

[Oleksandr, 2014/10/02 20:44:16]

Yes, I think it would be sufficient. We could in theory also log thread and
process IDs, but I don't remember that ever being useful before for me, so we
can just skip it.

On 2014/09/29 19:16:44, Eric wrote:

[Eric, 2014/10/02 23:11:02]

OK. I just used DEBUG_GENERAL, lacking anything specifically for this case yet.

+    DEBUG_GENERAL( "Caught unknown exception in CPluginClass::Invoke" );
     return E_FAIL;
   }
   return S_OK;
 }
 
 bool CPluginClass::InitObject(bool bBHO)
 {
   DEBUG_GENERAL("InitObject");
   CPluginSettings* settings = CPluginSettings::GetInstance();
 
@@ skipping 68 matching lines @@
   }
 
 
   int ieVersion = CPluginClient::GetInstance()->GetIEVersion();
   // Create status pane
   if (bBHO && ieVersion > 6 && !CreateStatusBarPane())
   {
     return false;
   }
   
-  if (CPluginClient::GetInstance()->IsFirstRun())
+  s_criticalSectionLocal.Lock();
+  int versionCompRes = CPluginClient::GetInstance()->CompareVersions(CPluginClient::GetInstance()->GetPref(L"currentVersion", L"0.0"), L"1.2");
+
+  bool isFirstRun = CPluginClient::GetInstance()->IsFirstRun();
+  CPluginClient::GetInstance()->SetPref(L"currentVersion", std::wstring(IEPLUGIN_VERSION));
+  // This is the first time ABP was installed
+  // Or ABP was updated from the version that did not support Acceptable Ads (<1.2)
+  if (isFirstRun || versionCompRes < 0)
   {   
+    if (!isFirstRun)
+    {
+      CPluginClient::GetInstance()->SetPref(L"displayUpdatePage", true);
+    }
+

[Oleksandr, 2014/10/03 08:31:50]

I understand this was added here inadvertently, since the code was merged
already, right?

[Oleksandr, 2014/10/03 15:19:20]

hm. Trying doing a diff for patchset 1 vs a patchset 2 here. There's a bunch of
code added here in patchset 2.
On 2014/10/03 15:14:13, Eric wrote:

[Eric, 2014/10/03 15:37:52]

Ah. What you're seeing is an artifact of rebasing the code between the time the
first patch set was created and that of the second. 

The changes are from 0892c5dece26064d992adf5f25c8895ed7ca6e3f, which is a change
of yours after we forked a development branch a couple of months ago.

     // IE6 can't be accessed from another thread, execute in current thread
     if (ieVersion < 7)
     {
       FirstRunThread();
     }
     else
     {
       CreateThread(NULL, NULL, (LPTHREAD_START_ROUTINE)CPluginClass::FirstRunThread, NULL, NULL, NULL);
     }
-    if ((m_hPaneWnd == NULL) || (!IsStatusBarEnabled()))
+    if (((m_hPaneWnd == NULL) || !IsStatusBarEnabled()) && isFirstRun)
     {
       ShowStatusBar();
     }
 
-  }
+    // Enable acceptable ads by default
+    std::wstring aaUrl = CPluginClient::GetInstance()->GetPref(L"subscriptions_exceptionsurl", L"");
+    CPluginClient::GetInstance()->AddSubscription(aaUrl);
+  }
+  s_criticalSectionLocal.Unlock();
   return true;
 }
 
 bool CPluginClass::CreateStatusBarPane()
 {
   CriticalSection::Lock lock(m_csStatusBar);
 
   CPluginClient* client = CPluginClient::GetInstance();
 
   wchar_t szClassName[MAX_PATH];
@@ skipping 1108 matching lines @@
         }
       }
     }
 
     hTabWnd = ::GetWindow(hTabWnd, GW_HWNDNEXT);
   }
 
   return hTabWnd;
 
 }