Issue #1234 - Remove CString local variable from CPluginTabBase::InjectABP

src/plugin/PluginTabBase.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include "PluginStdAfx.h"
-
-#include "PluginClient.h"
+#include "AdblockPlusClient.h"
+#include "PluginClientBase.h"
 #include "PluginSettings.h"
 #include "AdblockPlusDomTraverser.h"
-#include "PluginClass.h"
 #include "PluginTabBase.h"
-#include "PluginUtil.h"
 #include "IeVersion.h"
-#include <dispex.h>
 #include <Mshtmhst.h>
 
 CPluginTabBase::CPluginTabBase(CPluginClass* plugin)
   : m_plugin(plugin)
   , m_isActivated(false)
   , m_continueThreadRunning(true)
 {
   m_filter = std::auto_ptr<CPluginFilter>(new CPluginFilter());
   m_filter->hideFiltersLoadedEvent = CreateEvent(NULL, true, false, NULL);
 
@@ skipping 73 matching lines @@
   m_traverser->ClearCache();
 }
 
 namespace
 {
   /**
    * Determine if the HTML file is one of ours.
    * The criterion is that it appear in the "html/templates" folder within our installation.
    *
    * Warning: This function may fail if the argument is not a "file://" URL.
    * This is occasionally the case in circumstances yet to be characterized.

[Eric, 2015/05/20 15:56:32]

I see this happen under the debugger regularly, but not even completely
consistently then. The argument, instead of a file URL, is an ordinary file
system path, complete with Windows-style backslash directory separators.

I'm not sure its actually a defect in an ordinary installation.

[sergei, 2015/06/01 09:41:30]

I see that it's sometimes in MS-DOS/Windows-style when it's called while
reacting on DownloadComplete event and it seems to be always correct URI when
it's called from DocumentComplete. JIC, the most correct reaction on document
complete is implemented in issue-119
(http://codereview.adblockplus.org/6567422169448448/).

[Eric, 2015/06/10 18:30:36]

If you'd like to add text to the comment, please propose some. Since I'm not
sure it's even a defect, I wrote this as a warning of odd behavior in case this
bit of code is ever identified as defective.

    */
-  bool IsOurHtmlFile(std::wstring url)

[sergei, 2015/06/01 09:41:30]

const ref

[Eric, 2015/06/10 18:30:36]

Done.

+  bool IsOurHtmlFile(const std::wstring& url)
   {
     // Declared static because the value is derived from an installation directory, which won't change during run-time.
     static auto dir = FileUrl(HtmlFolderPath());
-    static auto dirLength = dir.length();

[sergei, 2015/06/01 09:41:30]

I would say we don't need `dirLength` variable.

[Eric, 2015/06/10 18:30:36]

Done.

-
-    std::wstring log = L"InjectABP. Current URL: ";
-    log += url;
-    log += L", template directory URL: ";
-    log += dir;
-    DEBUG_GENERAL(log);
-
+
+    DEBUG_GENERAL([&]() -> std::wstring {
+      std::wstring log = L"InjectABP. Current URL: ";
+      log += url;
+      log += L", template directory URL: ";
+      log += dir;
+      return log;
+    }());
+
+    /*
+     * The length check here is defensive, in case the document URL is truncated for some reason.
+     */
+    if (url.length() < 5)
+    {
+      // We can't match ".html" at the end of the URL if it's too short.
+      return false;
+    }
     auto urlCstr = url.c_str();

[sergei, 2015/06/01 09:41:30]

Actually, this variable is also not necessary.

[Eric, 2015/06/10 18:30:36]

Not strictly, no; it's a small optimization. But eliminating it means calling
c_str() twice down below.

     // Check the prefix to match our directory
     // Check the suffix to be an HTML file
-    return (_wcsnicmp(urlCstr, dir.c_str(), dirLength) == 0) &&
+    return (_wcsnicmp(urlCstr, dir.c_str(), dir.length()) == 0) &&
       (_wcsnicmp(urlCstr + url.length() - 5, L".html", 5) == 0);

[sergei, 2015/06/01 09:41:30]

It would be great to have a comment here explaining the reason the length of
`url` is always not less than 5.

[Eric, 2015/06/10 18:30:36]

We actually need an explicit check, which I've added.

I got myself confused before ('dir' is always of length at least 15) and didn't
have it in the first place.

   }
 }
 
 void CPluginTabBase::InjectABP(IWebBrowser2* browser)
 {
   CriticalSection::Lock lock(m_csInject);
   auto url = GetDocumentUrl();
   if (!IsOurHtmlFile(url))
   {
     DEBUG_GENERAL(L"InjectABP. Not injecting");
@@ skipping 229 matching lines @@
           LogQueue::LogPluginError(pluginError.GetErrorCode(), pluginError.GetErrorId(), pluginError.GetErrorSubid(), pluginError.GetErrorDescription(), true, pluginError.GetProcessId(), pluginError.GetThreadId());
         }
 
         // Non-hanging sleep
         Sleep(50);
       }
 
       tabLoopIteration++;
   }
 }