Issue #3383 - Rewrite and simplify browser-site handling in CPluginClass

src/plugin/PluginClass.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 72 matching lines @@
     }
 
     int Width() const
     {
       return right - left;
     }
   };
 }
 
 CPluginClass::CPluginClass()
   : m_webBrowser2(nullptr)

[sergei, 2015/11/30 15:52:08]

We don't need it because ATL::CComPtr default constructor initializes internal
pointer to nullptr.

[Eric, 2015/11/30 16:31:51]

I'd rather have it explicit as a lightweight form of documentation. The lifetime
of that object is inherently problematic if you're not deeply familiar with how
IE uses SetSite.

[sergei, 2015/12/07 10:49:08]

ATL::CComPtr initializes it to nullptr in the constructor, what does this
initialization document here in addition and how is it related to the lifetime
of some object? I'm also not sure which object is referred to above.

[Eric, 2015/12/07 14:04:12]

The very-lightweight documentation is that every value that 'm_webBrowser2'
takes appears explicitly in the code, including the initial value of zero.
It's related to the life cycle (not really the life span) of 'CPluginClass'.

FYI, this is how I think about this issue. Knowing what I know now, if I were
implementing this from scratch, I would move much of what's currently in
'CPluginClass' to a new class that would be created/destroyed in 'SetSite()'.
(I'm not planning on introducing such a change.) The way I see it, there is,
effectively, a second class with a slightly narrower lifetime whose constructor
has one argument of type 'IWebBrowser2'. But there's a disconnect between (1) a
C++ constructor with one argument and (2) the 'SetSite' implementation pattern.
These could be bridged explicitly, but doing so at this point would be quite an
extensive change.

[sergei, 2015/12/08 09:41:59]

I think it will go away in a future and I don't understand the reason to
introduce it to get rid of it later.

[sergei, 2015/12/08 09:41:59]

It sounds not convincing. Why are we not doing it everywhere?
Sounds reasonable but it's not related to initialization of class members with
the same values which they get from the default constructor.

 {
   //Use this line to debug memory leaks
   //    _CrtDumpMemoryLeaks();
 
   m_isAdvised = false;
   m_hTabWnd = NULL;
   m_hStatusBarWnd = NULL;
   m_hPaneWnd = NULL;
   m_nPaneWidth = 0;
   m_pWndProcStatus = NULL;
@@ skipping 11 matching lines @@
   delete m_tab;
 }
 
 HWND CPluginClass::GetBrowserHWND() const
 {
   if (!m_webBrowser2)
   {
     DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserHWND - Reached with m_webBrowser2 == nullptr");
     return nullptr;
   }
-  SHANDLE_PTR hBrowserWndHandle;

[sergei, 2015/11/30 15:52:09]

it would be good to initialize it to NULL.

[Eric, 2015/11/30 16:31:51]

Welcome to MicrosoftLand. SHANDLE_PTR is not a pointer type; it's 'long'.

We're not testing its value. Since we're not returning 'nullptr' on error (see
below), we're only returning its value if 'get_HWND()' succeeds, and in that
case we can rely upon its value.

I'd say we don't need to bother to initialize it, particularly since
initializing it with 'nullptr', what looks on the surface like it would be the
obvious choice, doesn't compile.

[Oleksandr, 2015/12/03 11:51:58]

We don't check its value now, but we might in future. So I don't see why we
would remove the initialization here.

[Eric, 2015/12/03 14:24:54]

Initialized it to zero, which matches its type.

As a rule, though, if we're going to do something in the future, we can do its
prerequisites in the future also.

[sergei, 2015/12/07 10:49:08]

Agree, except following some good practices, like here.

Beside the fact that it's always good to initialize variables, I would like to
say it's a good practice to always initialize the value which is returned
through a pointer, like here or like e.g. void** in QueryInterface, because it
allows to find a bug during the development cycle. Such pointers are often held
by some smart-pointer wrappers, so the callee can in addition verify that the
value stored at the pointer value is not a valid value to avoid resource leakage
by overriding it.
Although one can argue that it's not our case because we are calling Microsoft's
method (which in particular does not fail or trigger any assert depending on the
argument value) and we are not going to implement tests for our plugin, I still
think that it's worth doing because at least it simplifies the readability
because one don't have to think and look into whether it's our implementation or
can somehow fail later and of course it does not cause any questions during the
codereview.

[sergei, 2015/12/07 10:49:08]

Although zero is OK, NULL looks better here.

[Eric, 2015/12/07 14:04:12]

I disagree. Sometimes NULL is defined as '(void*)0'. I don't want to incur any
cognitive overhead thinking about what it ought to be. It's Microsoft's
interface defect; we have to live with it.

[Eric, 2015/12/07 14:04:12]

Almost always.  But even saying that is saying too much here, because there's no
remaining issue of initialization.

+  SHANDLE_PTR hBrowserWndHandle = 0;
   HRESULT hr = m_webBrowser2->get_HWND(&hBrowserWndHandle);
   if (FAILED(hr))
   {
-    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Class::GetBrowserHWND - failed")

[sergei, 2015/11/30 15:52:08]

Actually, I think we should return nullptr in case of error, shouldn't we?

[Eric, 2015/11/30 16:31:51]

Done.

That's a good idea.

+    DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_UI, PLUGIN_ERROR_UI_GET_BROWSER_WINDOW, "Class::GetBrowserHWND - failed");
+    return nullptr;
   }
   return (HWND)hBrowserWndHandle;
 }
 
 bool CPluginClass::IsRootBrowser(IWebBrowser2* otherBrowser)
 {
   return m_webBrowser2.IsEqualObject(otherBrowser);
 }
 
 CComQIPtr<IWebBrowser2> CPluginClass::GetAsyncBrowser()
@@ skipping 11 matching lines @@
 
 std::wstring CPluginClass::GetBrowserUrl() const
 {
   std::wstring url;
   if (m_webBrowser2)
   {
     CComBSTR bstrURL;
     if (SUCCEEDED(m_webBrowser2->get_LocationURL(&bstrURL)) && bstrURL)
     {
       url = std::wstring(bstrURL, SysStringLen(bstrURL));
-      UnescapeUrl(url);
     }
   }
   else
   {
     DEBUG_ERROR_LOG(0, 0, 0, "CPluginClass::GetBrowserUrl - Reached with m_webBrowser2 == nullptr");
     url = m_tab->GetDocumentUrl();
   }
   return url;
 }
 
@@ skipping 118 matching lines @@
         {
           s_threadInstances.erase(it);
         }
         if (s_instances.empty())
         {
           // TODO: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr
           CPluginClientFactory::ReleaseMimeFilterClientInstance();
         }
       }
       s_criticalSectionLocal.Unlock();
+
+      m_webBrowser2 = nullptr;
 
       DEBUG_GENERAL("================================================================================\nNEW TAB UI - END\n================================================================================")
 
       ::CoUninitialize();
     }
 
   }
   catch (...)
   {
   }
@@ skipping 129 matching lines @@
     ATL::CComQIPtr<IWebBrowser2> webBrowser = frameBrowserDisp;
     if (!webBrowser)
     {
       return;
     }
     if (!urlVariant || urlVariant->vt != VT_BSTR)
     {
       return;
     }
     std::wstring url(urlVariant->bstrVal, SysStringLen(urlVariant->bstrVal));
-    UnescapeUrl(url);
 
     // If webbrowser2 is equal to top level browser (as set in SetSite), we are
     // navigating new page
     CPluginClient* client = CPluginClient::GetInstance();
     if (url.find(L"javascript") == 0)
     {
     }
     else if (IsRootBrowser(webBrowser))
     {
       m_tab->OnNavigate(url);
@@ skipping 41 matching lines @@
 {
   try
   {
     DEBUG_NAVI(L"Navi::Document Complete");
     ATL::CComQIPtr<IWebBrowser2> webBrowser2 = frameBrowserDisp;
     if (!webBrowser2)
     {
       return;
     }
     std::wstring frameSrc = GetLocationUrl(*webBrowser2);
-    UnescapeUrl(frameSrc);
     m_tab->OnDocumentComplete(webBrowser2, frameSrc, IsRootBrowser(webBrowser2));
   }
   catch (...)
   {
   }
 }
 
 // Entry point
 void STDMETHODCALLTYPE CPluginClass::OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask)
 {
@@ skipping 560 matching lines @@
           if (FAILED(hr))
           {
             DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION, PLUGIN_ERROR_NAVIGATION_SETTINGS, "Navigation::Failed")
           }
         }
       }
       break;
     }
   case ID_MENU_DISABLE_ON_SITE:
     {
-      CPluginSettings* settings = CPluginSettings::GetInstance();
       std::wstring urlString = GetTab()->GetDocumentUrl();
       std::string filterText = client->GetWhitelistingFilter(urlString);
       if (!filterText.empty())
       {
         client->RemoveFilter(filterText);
       }
       else
       {
-        settings->AddWhiteListedDomain(ToCString(client->GetHostFromUrl(urlString)));
+        CPluginSettings::GetInstance()->AddWhiteListedDomain(client->GetHostFromUrl(urlString));
       }
     }
   default:
     break;
   }
 
   // Invalidate and redraw the control
   UpdateStatusBar();
 }
 
@@ skipping 522 matching lines @@
   s_criticalSectionLocal.Unlock();
 
   return icon;
 }
 
 ATOM CPluginClass::GetAtomPaneClass()
 {
   return s_atomPaneClass;
 }