Issue 4395 - Setting page is loaded non-populated on some systems

src/plugin/PluginTabBase.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 137 matching lines @@
   /**
    * 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.
    */
   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());
-
-    dir = EscapeUrl(CanonicalizeUrl(dir));

[Eric, 2016/09/07 16:50:52]

There's no need to treat 'dir' as arising from a URL. It comes from a call to
system API `GetModuleFileName` and will always be in standard Window path
syntax. So at the very least calling `CanonicalizeUrl` here isn't necessary.

Besides, there's no reason to rewrite a static variable each time. If you really
want to compare in escaped URL form, just call `EscapeURL` in the
definition/initialization statement in line 158.

And in addition, I don't see why we need to compare escaped strings in the first
place. We can conver the path in the URL to canonical Windows form and compare
those. See comment in the definition of `CanonicalizeUrl` for how to do that. Or
just use the function we used to have that did that. We removed then function
`UnescapeUrl` previously because it was interfering with the blocking algorithm.
That function could be used here. It was removed in change set
5531a44442cd29020a384dc99bdddc08656e560a, and it was already fully tested.

[Oleksandr, 2016/09/08 02:51:15]

The thinking here is that Since dir arrives from a system call we cannot assume
it to be in any format, really. I think to be safe having Canonicalization here
would be fine.

-    std::wstring urlCanonicalized = EscapeUrl(CanonicalizeUrl(url));

[Eric, 2016/09/07 16:50:52]

As a point of English usage, `urlCanonical` (with "canonical" as an adjective)
is a little better. We don't need to care how it became canonical.

[Oleksandr, 2016/09/08 02:51:15]

Done.

+    static auto dir = CanonicalizeUrl(FileUrl(HtmlFolderPath()));
+
+    std::wstring urlCanonical = CanonicalizeUrl(url);
 
     DEBUG_GENERAL([&]() -> std::wstring {
       std::wstring log = L"InjectABP. Current URL: ";
-      log += urlCanonicalized;

[Eric, 2016/09/07 16:50:52]

It would be better here to use the argument `url` unmodified so that the log
contains what came in from the outside.

[Oleksandr, 2016/09/08 02:51:15]

During debugging I actually find it more convenient to have the processed URL
here. Just to see what exactly is being compared to what.

+      log += urlCanonical;
       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 (urlCanonicalized.length() < 5)
+    if (urlCanonical.length() < 5)
     {
       // We can't match ".html" at the end of the URL if it's too short.
       return false;
     }
-    auto urlCstr = urlCanonicalized.c_str();
+    auto urlCstr = urlCanonical.c_str();
     // Check the prefix to match our directory
     // Check the suffix to be an HTML file
-    // Compare escaped version and return
     return (_wcsnicmp(urlCstr, dir.c_str(), dir.length()) == 0) &&
-      (_wcsnicmp(urlCstr + url.length() - 5, L".html", 5) == 0);
+      (_wcsnicmp(urlCstr + urlCanonical.length() - 5, L".html", 5) == 0);
   }
 }
 
 void CPluginTab::InjectABP(IWebBrowser2* browser)
 {
   CriticalSection::Lock lock(m_csInject);
   auto url = GetDocumentUrl();
   if (!IsOurHtmlFile(url))
   {
     DEBUG_GENERAL(L"InjectABP. Not injecting");
@@ skipping 424 matching lines @@
           LogQueue::LogPluginError(pluginError.GetErrorCode(), pluginError.GetErrorId(), pluginError.GetErrorSubid(), pluginError.GetErrorDescription(), true, pluginError.GetProcessId(), pluginError.GetThreadId());
         }
 
         // Non-hanging sleep
         Sleep(50);
       }
 
       tabLoopIteration++;
   }
 }