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

src/plugin/PluginTabBase.cpp

Index: src/plugin/PluginTabBase.cpp
===================================================================
--- a/src/plugin/PluginTabBase.cpp
+++ b/src/plugin/PluginTabBase.cpp
@@ -157,9 +157,12 @@
     // 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.

+
     DEBUG_GENERAL([&]() -> std::wstring {
       std::wstring log = L"InjectABP. Current URL: ";
-      log += 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 += L", template directory URL: ";
       log += dir;
       return log;
@@ -168,14 +171,15 @@
     /*
      * The length check here is defensive, in case the document URL is truncated for some reason.
      */
-    if (url.length() < 5)
+    if (urlCanonicalized.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();
+    auto urlCstr = urlCanonicalized.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);
   }