Fix ABP pages on refresh

src/plugin/PluginTabBase.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginClient.h"
 #include "PluginSettings.h"
 #ifdef SUPPORT_CONFIG
 #include "PluginConfig.h"
 #endif
 #include "PluginTab.h"
 #include "PluginDomTraverser.h"
 #include "PluginClass.h"
@@ skipping 91 matching lines @@
 
   std::wstring domainString = GetDocumentDomain();
   ResetEvent(m_filter->hideFiltersLoadedEvent);
   CreateThread(NULL, NULL, &FilterLoader, this, NULL, NULL);
 
 #ifdef SUPPORT_DOM_TRAVERSER
   m_traverser->ClearCache();
 #endif
 }
 
+void CPluginTabBase::InjectABP(IWebBrowser2* browser)
+{
+  CString url = GetDocumentUrl();
+  CString log;
+  log.Format(L"Current URL: %s, settings URL: %s", url, UserSettingsFileUrl().c_str());
+  DEBUG_GENERAL(log);
+  if (0 == url.CompareNoCase(CString(UserSettingsFileUrl().c_str())) ||
+      0 == url.CompareNoCase(CString(FirstRunPageFileUrl().c_str())) )
+  {
+    CComPtr<IDispatch> pDocDispatch;
+    browser->get_Document(&pDocDispatch);
+    CComQIPtr<IHTMLDocument2> pDoc2 = pDocDispatch;
+    if (pDoc2)

[Wladimir Palant, 2013/08/13 09:48:34]

Now that this is a separate function we can avoid the huge indentation here:

if (!pDoc2)
  return;

And similarly for all the other checks.

+    {
+      CComPtr<IHTMLWindow2> pWnd2;
+      pDoc2->get_parentWindow(&pWnd2);
+      if (pWnd2)
+      {
+        CComQIPtr<IDispatchEx> pWndEx = pWnd2;
+        if (pWndEx)
+        {
+          // Create "Settings" object in JavaScript.
+          // A method call of "Settings" in JavaScript, transfered to "Invoke" of m_pluginUserSettings
+          DISPID dispid;
+          HRESULT hr = pWndEx->GetDispID(L"Settings", fdexNameEnsure, &dispid);
+          if (SUCCEEDED(hr))
+          {
+            CComVariant var((IDispatch*)&m_pluginUserSettings);
+
+            DISPPARAMS params;
+            params.cArgs = 1;
+            params.cNamedArgs = 0;
+            params.rgvarg = &var;
+            params.rgdispidNamedArgs = 0;
+            hr = pWndEx->Invoke(dispid, IID_NULL, LOCALE_USER_DEFAULT, DISPATCH_PROPERTYPUT | DISPATCH_PROPERTYPUTREF, &params, 0, 0, 0);
+            if (FAILED(hr))
+            {
+              DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT_INVOKE, "CPluginTabBase::OnDocumentComplete - Failed to create Settings in JavaScript");

[Felix Dahlke, 2013/08/08 08:47:53]

It's CPluginTabBase::InjectABP now, isn't it?

[Oleksandr, 2013/08/08 14:08:47]

It is, yes.
On 2013/08/08 08:47:53, Felix H. Dahlke wrote:

+            }
+          }
+        }
+      }
+    }
+  }
+}
+
 void CPluginTabBase::OnDownloadComplete(IWebBrowser2* browser)
 {
 #ifdef SUPPORT_DOM_TRAVERSER
   if (!CPluginClient::GetInstance()->IsWhitelistedUrl(std::wstring(GetDocumentUrl())))
   {
     m_traverser->TraverseDocument(browser, GetDocumentDomain(), GetDocumentUrl());
   }
 #endif // SUPPORT_DOM_TRAVERSER
+
+  InjectABP(browser);
 }
 
-
 void CPluginTabBase::OnDocumentComplete(IWebBrowser2* browser, const CString& url, bool isDocumentBrowser)
 {
   CString documentUrl = GetDocumentUrl();
 
   if (isDocumentBrowser)
   {
     if (url != documentUrl)
     {
       SetDocumentUrl(url);
     }
-
-    CString log;
-    log.Format(L"Current URL: %s, settings URL: %s", url, UserSettingsFileUrl().c_str());
-    DEBUG_GENERAL(log);
-    if (0 == url.CompareNoCase(CString(UserSettingsFileUrl().c_str())) ||
-        0 == url.CompareNoCase(CString(FirstRunPageFileUrl().c_str())) )
-    {
-      CComPtr<IDispatch> pDocDispatch;
-      browser->get_Document(&pDocDispatch);
-      CComQIPtr<IHTMLDocument2> pDoc2 = pDocDispatch;
-      if (pDoc2)
-      {
-        CComPtr<IHTMLWindow2> pWnd2;
-        pDoc2->get_parentWindow(&pWnd2);
-        if (pWnd2)
-        {
-          CComQIPtr<IDispatchEx> pWndEx = pWnd2;
-          if (pWndEx)
-          {
-            // Create "Settings" object in JavaScript.
-            // A method call of "Settings" in JavaScript, transfered to "Invoke" of m_pluginUserSettings
-            DISPID dispid;
-            HRESULT hr = pWndEx->GetDispID(L"Settings", fdexNameEnsure, &dispid);
-            if (SUCCEEDED(hr))
-            {
-              CComVariant var((IDispatch*)&m_pluginUserSettings);
-
-              DISPPARAMS params;
-              params.cArgs = 1;
-              params.cNamedArgs = 0;
-              params.rgvarg = &var;
-              params.rgdispidNamedArgs = 0;
-              hr = pWndEx->Invoke(dispid, IID_NULL, LOCALE_USER_DEFAULT, DISPATCH_PROPERTYPUT | DISPATCH_PROPERTYPUTREF, &params, 0, 0, 0);
-              if (FAILED(hr))
-              {
-                DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT, PLUGIN_ERROR_CREATE_SETTINGS_JAVASCRIPT_INVOKE, "CPluginTabBase::OnDocumentComplete - Failed to create Settings in JavaScript");
-              }
-            }
-          }
-        }
-      }
-    }
+    InjectABP(browser);

[Felix Dahlke, 2013/08/08 08:47:53]

So OnDocumentComplete is only called once, not on refresh? Is OnDownloadComplete
called every time?

Is it possible that DISP_DOCUMENTCOMPLETE is actually happening, but we don't
call OnDocumentComplete because of the additional checks there?

[Oleksandr, 2013/08/08 14:08:47]

No, I have checked the events raised. It isn't fired. You can also check
comments here:
http://social.msdn.microsoft.com/Forums/ie/en-US/7a3a8d96-52f5-4ec0-a395-222c...

On 2013/08/08 08:47:53, Felix H. Dahlke wrote:

[Felix Dahlke, 2013/08/08 14:19:05]

So, initially, OnDocumentComplete is fired and OnDownloadComplete is not fired,
and on refresh OnDocumentComplete is not fired but OnDownloadComplete is fired?
Doesn't make any sense to me, but if that's how it is...

[Oleksandr, 2013/08/08 16:00:04]

No, I got it a bit wrong actually. Both events are fired in each case, but when
refresh is done, the URL property is ambiguous (get_LocationURL returns
about:blank on IE11, for example). So we can't count on it in that case. 
On the other hand, when not refreshing but just navigating - DOWNLOADCOMPLETE
gets the URL that is in "incorrect" format. Like "c:\Program Files\..", not
file:///c:/Program Files. We could just use one event and different URLs of
course, but I think this is more consistent.

On 2013/08/08 14:19:05, Felix H. Dahlke wrote:

[Felix Dahlke, 2013/08/08 16:20:59]

That's fine by me, but isn't InjectABP being called twice per page load now?
That we should avoid.

[Oleksandr, 2013/08/09 07:07:48]

InjectABP can get called even more then twice per page load (it is fired on
every time a document is downloaded, so all frames included). If the URL being
navigated to fits the URL where ABP should be injected - the injection will run.
It will always succeed (GetDispID and Invoke), but based on the current state of
the page the actual Settings object will appear only in one of
OnDownloadComplete or OnDocumentComplete events. It is not quite clear to me why
the behavior is as such (Document's readyState is "complete" in both cases), but
it works. Even if somehow injection ran twice, it wouldn't be a problem since it
would just make sure the same object is created on page's javascript context.

On 2013/08/08 16:20:59, Felix H. Dahlke wrote:

   }
 
 #ifdef SUPPORT_DOM_TRAVERSER
   if (url.Left(6) != "res://")
   {
     // Get document
     CComPtr<IDispatch> pDocDispatch;
     HRESULT hr = browser->get_Document(&pDocDispatch);
     if (FAILED(hr) || !pDocDispatch)
     {
@@ skipping 174 matching lines @@
 
         // Non-hanging sleep
         Sleep(50);
       }
 
       tabLoopIteration++;
   }
 
   return 0;
 }