Issue #1356 - Improve detection of the issuer of the request

This fixes the problem for IE9+. IE8 is left blocking a bit too much, unfortunately.

[Eric, 2014-10-08 17:37:53]

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
src/plugin/PluginClass.cpp:1139: 
I'm mildly concerned that this version of GetTab is using a linear search on
strings. On the other hand, maintaining a cache of results from calls to
GetBrowserURL() would be necessary to use a hash-map, and that means reliably
trapping all the ways it could change.

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
src/plugin/PluginClass.cpp:1147: for (it = s_threadInstances.begin(); it !=
s_threadInstances.end(); ++it)
We can use the range-for syntax here. It's supported by our version of Visual
Studio.

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
src/plugin/PluginClass.cpp:1152: return it->second->m_tab;
If we were using a sentry object to control the mutex, we wouldn't need a
separate call to Unlock() and returning in the middle of a loop wouldn't look
awkward. Instead, I'd assign to 'tab' and then 'break'.

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
File src/plugin/PluginWbPassThrough.cpp (right):

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
src/plugin/PluginWbPassThrough.cpp:227: if ((tab != 0) &&
(tab->IsFrameCached(src)))
Prefer nullptr to 0, but the idiom we're using (as a rule) is to treat pointers
(and pointer-like objects) as implicitly casting to true if non-null.

[sergei, 2014-10-09 13:46:22]

Because it does not exactly correspond to #1356 (see the comment in the code,
it's "not what we need exactly for the filters") I would make it as a part of
another issue.

Could you give an example (web page) where one can debug it, I could not
reproduce when the code enters into `if ((tab != 0) &&
(tab->IsFrameCached(src)))`?

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
src/plugin/PluginClass.cpp:1139: 
I think it should be fast enough with linear search for now. If we have some
performance issues I would prefer firstly to profile the plugin.

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
src/plugin/PluginClass.cpp:1152: return it->second->m_tab;
+1 for scoped lock
I also don't like return if it's not an error case, but here it's OK for me.

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
File src/plugin/PluginWbPassThrough.cpp (right):

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
src/plugin/PluginWbPassThrough.cpp:229: contentType =
CFilter::contentTypeSubdocument;
I would leave the comment somewhere
- that it's only for frames, fetched inside other frames or by third-party
stuff, when the tab is nullptr.
- we can accidentally get the wrong tab. If there is another tab (with the lower
thread id) which contains the same frame.
- the line below is not what we need exactly for the filters. For the case:
tab, root-page src = root.html
    iframe src = 1st-iframe.html
      iframe src = 2nd-iframe.html
the call should be
ShouldBlock(/*src*/2nd-iframe.html, ..., 1st-iframe.html)
but it will be
ShouldBlock(/*src*/2nd-iframe.html, ..., root.html)
Also it might be important to note a detail that IE can have several tabs in the
one process, see IE8 and Loosely-Coupled IE (LCIE).

http://codereview.adblockplus.org/5113230347206656/diff/5741031244955648/src/...
src/plugin/PluginWbPassThrough.cpp:231: if (client->ShouldBlock(boundDomain,
contentType, tab->GetDocumentUrl(), true))
Instead of `boundDomain` there should be `src`

[sergei, 2014-10-10 10:59:42]

I think CPluginClass::GetTab(const std::wstring& url) is incorrect and we don't
need it.

We lookup the tab which has GetBrowserUrl() equal to BINDSTRING_ROOTDOC_URL and
then use tab->GetDocumentUrl().
First of all as I understand from CPluginClass::GetBrowserUrl and from
http://msdn.microsoft.com/en-us/library/aa752130(v=vs.85).aspx , theoretically,
until we receive the first NavigateComplete2
`browser->get_LocationURL(&bstrURL)` can return an empty string. So we might
should fix the method, although that particular case seems not too important.
The important thing is that we should check whether the browser is busy or not
before get_LocationURL. Debugging it I got, that when it returns that the
browser is busy, browser->get_LocationURL(&bstrURL) returns the previous
location instead of expected (e.g, "about:Tabs" or http://www.bing.com/search...
while I'm already navigating another page) but m_tab->m_documentUrl (which is
set in CPluginClass::BeforeNavigate2,CPluginTabBase::OnNavigate) is the actual
url I've navigated to. So I would say that we should not use
CPluginClass::GetBrowserUrl here, and use CPluginTabBase::GetDocumentUrl, but in
that case we don't need to lookup it, because we have it already. As well see
#1472.

[Oleksandr, 2014-10-13 00:22:53]

Unfortunately since my previous tests were invalid, and as it has been pointed
out, this patch set doesn't really improve anything - I have to close this code
review.

On 2014/10/10 10:59:42, sergei wrote:
> I think CPluginClass::GetTab(const std::wstring& url) is incorrect and we
don't
> need it.
> 
> We lookup the tab which has GetBrowserUrl() equal to BINDSTRING_ROOTDOC_URL
and
> then use tab->GetDocumentUrl().
> First of all as I understand from CPluginClass::GetBrowserUrl and from
> http://msdn.microsoft.com/en-us/library/aa752130%28v=vs.85%29.aspx ,
theoretically,
> until we receive the first NavigateComplete2
> `browser->get_LocationURL(&bstrURL)` can return an empty string. So we might
> should fix the method, although that particular case seems not too important.
> The important thing is that we should check whether the browser is busy or not
> before get_LocationURL. Debugging it I got, that when it returns that the
> browser is busy, browser->get_LocationURL(&bstrURL) returns the previous
> location instead of expected (e.g, "about:Tabs" or
http://www.bing.com/search...
> while I'm already navigating another page) but m_tab->m_documentUrl (which is
> set in CPluginClass::BeforeNavigate2,CPluginTabBase::OnNavigate) is the actual
> url I've navigated to. So I would say that we should not use
> CPluginClass::GetBrowserUrl here, and use CPluginTabBase::GetDocumentUrl, but
in
> that case we don't need to lookup it, because we have it already. As well see
> #1472.