Issue 3237 - Whitelisted AFS ad is not shown on netzwelt.de

src/plugin/PluginWbPassThrough.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
@@ skipping 124 matching lines @@
   }
 }
 
 WBPassthruSink::WBPassthruSink()
   : m_currentPositionOfSentPage(0)
   , m_contentType(ContentType::CONTENT_TYPE_OTHER)
   , m_isCustomResponse(false)
 {
 }
 
 ContentType WBPassthruSink::GetContentTypeFromMimeType(const std::wstring& mimeType)

[Eric, 2015/12/01 20:32:56]

Here's the relevant section on the Accept header from the standard
    http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html
It specifies an algorithm to determine precedence of the various media types.
Unfortunately, that precedence is not the order they appear in the media type
list. Nevertheless, it would seem that the highest precedence media types says
more about the request type than the lower ones. The present algorithm doesn't
distinguish precedence. On the other hand, I don't know what IE actually
generates to know if this is important.

That all said, the present algorithm looks throughout the whole media type list,
ignoring anything like precedence. If IE is generating these lists sanely, then
the first media type in the list would be the highest precedence one. If that's
the case, we'd want to loop through the media types and check each in turn. 

I can guess that IE doesn't send Accept-Charset or Accept-Language when it's
requesting images or objects. (I haven't tried to find out if this is so.) We
might be able to get better distinctions if we look at these headers in addition
to Accept by itself.

Here's a recent, mostly-authoritative (it's from IANA) list of known media
types.
    http://www.iana.org/assignments/media-types/media-types.xhtml
Clearly we don't need all of these in the code, but perusing it might be useful.

[Oleksandr, 2015/12/15 02:10:16]

It is not known really if the order has any effect on the possible type of the
request. It looks like it might, but it is a little far fetched, I think.
There's also very little gain had we implemented the precedence algorithm
anyway.
It does send Accept-Language for images as well.

[Eric, 2015/12/15 03:18:18]

Given the data you've gathered, it seems that IE isn't doing anything
sophisticated here. It seems like it might just have some string constants it
uses.

[Eric, 2015/12/15 03:18:18]

While not strictly necessary, there are a few things that would improve the
code, as long as we're touching it here. They're listed in the order I care.
- Add a comment here listing the media type strings found in testing. That's
some useful information that doesn't deserve to get buried in a comment in a
review.
- "MimeType" should be renamed "MediaTypeList". The present name is somewhat
confusing unless you track down where it's called from.
- I'd use "InferContentType" rather than "Get...". This is a heuristic, not just
a parse-and-fetch operation like its name seems to indicate.
- This function could be in an anonymous namespace. It doesn't need to be a
static class member.

I'm not going to hold the review up if these aren't present, but I would like to
see them, particularly the first, since it directly related to the defect being
fixed in the present change set.

[sergei, 2015/12/15 15:29:31]

The same from my side.

 {
+  if ((mimeType.find(L"text/html") != std::wstring::npos) ||
+      (mimeType.find(L"application/xhtml+xml") != std::wstring::npos))
+  {
+    return ContentType::CONTENT_TYPE_SUBDOCUMENT;
+  }
   if (mimeType.find(L"image/") != std::wstring::npos)
   {
-        // Ambivalent mime type suggests both image and other types. 
-        // Heuristically this is more likely a SUBDOCUMENT request.
-        if ((mimeType.find(L"text/") != std::wstring::npos) ||

[sergei, 2015/12/01 10:53:59]

In IE11 on my machine Accept header for "subdocument" seems "text/html,
application/xhtml+xml, image/jxr, */*" pretty stable. Just wonder, when and what
are there some different "text/???" and "application/???"? May be it  makes
sense to restrict the rule here.

What are the values on IE{8-10}?

[Eric, 2015/12/01 20:32:57]

We don't have anything like a crawler for IE, but it doesn't seem that it would
be all that difficult to log the media type list here, crawl manually, and
gather a collection of the media type lists that IE actually generates. 

[Oleksandr, 2015/12/15 02:10:16]

I have only been able to log 2 types here:
text/html, application/xhtml+xml, image/jxr, */*
image/png, image/svg+xml, image/jxr, image/*;q=0.8, */*;q=0.5

In IE9 the image/jxr part is missing, but other then that the header looks very
similar. So I agree, we can be precise here.

[Eric, 2015/12/15 03:18:18]

These are the strings I'd like to see in a comment.

-                (mimeType.find(L"application/") != std::wstring::npos))
-        {
-                return ContentType::CONTENT_TYPE_SUBDOCUMENT;
-        }
     return ContentType::CONTENT_TYPE_IMAGE;
   }
   if (mimeType.find(L"text/css") != std::wstring::npos)
   {
     return ContentType::CONTENT_TYPE_STYLESHEET;
   }
   if ((mimeType.find(L"application/javascript") != std::wstring::npos) || (mimeType.find(L"application/json") != std::wstring::npos))
   {
     return ContentType::CONTENT_TYPE_SCRIPT;
   }
   if (mimeType.find(L"application/x-shockwave-flash") != std::wstring::npos)
   {
     return ContentType::CONTENT_TYPE_OBJECT;
-  }
-  if (mimeType.find(L"text/html") != std::wstring::npos)

[Eric, 2015/12/01 20:32:56]

Rather than testing for "text/html" when there's a known image, you could just
move this test up to the top and test for it first.

I would also guess that anything including "application/xhtml+xml"
(XML-serialized HTML5) is also a document. I don't know, though, if IE generates
that header in requests.

[Oleksandr, 2015/12/15 02:10:16]

Seems like it does, based on my tests. Done.

-  {
-    return ContentType::CONTENT_TYPE_SUBDOCUMENT;
   }
   // It is important to have this check last, since it is rather generic, and might overlay text/html, for example
   if (mimeType.find(L"xml") != std::wstring::npos)
   {
     return ContentType::CONTENT_TYPE_XMLHTTPREQUEST;
   }
 
   return ContentType::CONTENT_TYPE_OTHER;
 }
 
@@ skipping 36 matching lines @@
   return contentType;
 }
 
 ////////////////////////////////////////////////////////////////////////////////////////
 //WBPassthruSink
 //Monitor and/or cancel every request and responde
 //WB makes, including images, sounds, scripts, etc
 ////////////////////////////////////////////////////////////////////////////////////////
 HRESULT WBPassthruSink::OnStart(LPCWSTR szUrl, IInternetProtocolSink *pOIProtSink,
                                 IInternetBindInfo *pOIBindInfo, DWORD grfPI, HANDLE_PTR dwReserved,
-                                IInternetProtocol* pTargetProtocol, bool& handled)
+                                IInternetProtocol* pTargetProtocol)
 {
   m_pTargetProtocol = pTargetProtocol;
   return BaseClass::OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, pTargetProtocol);
 }
 
 HRESULT WBPassthruSink::OnRead(void* pv, ULONG cb, ULONG* pcbRead)
 {
   if (!pv || !pcbRead)
   {
     return E_POINTER;
@@ skipping 200 matching lines @@
   }
 
   return OnStart(szUrl, pOIProtSink, pOIBindInfo, grfPI, dwReserved, m_spInternetProtocol);
 }
 
 STDMETHODIMP WBPassthru::Read(/* [in, out] */ void *pv,/* [in] */ ULONG cb,/* [out] */ ULONG *pcbRead)
 {
   WBPassthruSink* pSink = GetSink();
   return pSink->OnRead(pv, cb, pcbRead);
 }