Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1230)

Issue 6390087684194304: [superseded] Issue 1265 - Improve detection of mime type of the expected response (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by sergei
Modified:
4 years, 9 months ago
Visibility:
Public.

Description

This code review is superseded by http://codereview.adblockplus.org/6299667012780032/ . It's not removed so far to don't loose comments. Sometimes the mime type is not available in `WBPassthruSink::OnStart` and it's not possible to derive any information from the url. For such cases one can obtain `Accept` header in `BeginningTransaction` and make a decision here, if it's blocked we should return E_ABORT. It's possible via querying for `IWinInetHttpInfo` from the target protocol and calling `QueryInfo(HTTP_QUERY_RAW_HEADERS_CRLF | HTTP_QUERY_FLAG_REQUEST_HEADERS...)`. The latter call fails when it's done from our `OnStart` method, it seems that original target `protocol::Start` prepares the request and headers and then calls `BeginningTransaction`. To do it we need to store the url of the issuer of the request (`WBPassthruSink::m_boundDomain`) because it's needed by `CPluginFilter::ShouldBlock` in `BeginningTransaction`. As well as make `WBPassthruSink::m_contentType` which is used in `BeginningTransaction`, when the value is `CFilter::EContentType::contentTypeAny` it means that we could not derive the "type of request" in `OnStart` and we should try it here. Clean some code and a make couple of fixes: - If cannot derive mime type from URL then use `CFilter::EContentType::contentTypeAny` without subtracting `CFilter::EContentType::contentTypeSubdocument`. It's indeed in many cases `subdocument`. - add `CPluginDebug::DebugResultIgnoring(type, src, domain);` into `CPluginFilter::ShouldBlock`. There was already a call `CPluginDebug::DebugResultBlocking`, so this call is for consistency. On another hand that pair of calls is removed from `WBPassthruSink`, because it only duplicates blocked entries in the log.

Patch Set 1 #

Total comments: 14

Patch Set 2 : restore assuming XHR request type when xml is queried #

Patch Set 3 : restore assuming XHR request type when xml is queried #

Patch Set 4 : Fix missed support of other blocked types #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -102 lines) Patch
M src/plugin/AdblockPlusClient.h View 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/AdblockPlusClient.cpp View 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/AdblockPlusDomTraverser.cpp View 1 chunk +1 line, -2 lines 0 comments Download
M src/plugin/PluginClass.cpp View 1 chunk +0 lines, -2 lines 0 comments Download
M src/plugin/PluginFilter.cpp View 1 chunk +3 lines, -0 lines 0 comments Download
M src/plugin/PluginTabBase.cpp View 1 chunk +0 lines, -3 lines 0 comments Download
M src/plugin/PluginWbPassThrough.h View 1 1 chunk +6 lines, -3 lines 0 comments Download
M src/plugin/PluginWbPassThrough.cpp View 1 2 3 9 chunks +89 lines, -85 lines 4 comments Download
M src/plugin/SinkPolicy.inl View 2 chunks +3 lines, -5 lines 0 comments Download

Messages

Total messages: 4
sergei
4 years, 10 months ago (2014-09-11 08:31:15 UTC) #1
Oleksandr
http://codereview.adblockplus.org/6390087684194304/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (left): http://codereview.adblockplus.org/6390087684194304/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#oldcode225 src/plugin/PluginWbPassThrough.cpp:225: else type = "OTHER"; We will still need a ...
4 years, 10 months ago (2014-09-11 08:58:54 UTC) #2
sergei
http://codereview.adblockplus.org/6390087684194304/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (left): http://codereview.adblockplus.org/6390087684194304/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#oldcode225 src/plugin/PluginWbPassThrough.cpp:225: else type = "OTHER"; On 2014/09/11 08:58:54, Oleksandr wrote: ...
4 years, 10 months ago (2014-09-11 09:53:25 UTC) #3
Oleksandr
4 years, 9 months ago (2014-10-01 09:15:39 UTC) #4
http://codereview.adblockplus.org/6390087684194304/diff/5766466041282560/src/...
File src/plugin/PluginWbPassThrough.cpp (right):

http://codereview.adblockplus.org/6390087684194304/diff/5766466041282560/src/...
src/plugin/PluginWbPassThrough.cpp:194: if (nullptr == tab)
I personally don't like yoda conditions, and I think we have it in our coding
style not to use them. 

if (tab == nullptr) looks better, imho.

http://codereview.adblockplus.org/6390087684194304/diff/5766466041282560/src/...
src/plugin/PluginWbPassThrough.cpp:201: && client->ShouldBlock(src,
m_contentType, m_boundDomain, true))
Comment about yoda conditions applies here as well. And further below for all
the m_contentType checks.

http://codereview.adblockplus.org/6390087684194304/diff/5766466041282560/src/...
src/plugin/PluginWbPassThrough.cpp:231: if
(CFilter::EContentType::contentTypeXmlHttpRequest == m_contentType)
Looks like we don't need a special case for contentTypeXmlHttpRequest do we? The
'if' below will do the same anyway.

http://codereview.adblockplus.org/6390087684194304/diff/5766466041282560/src/...
src/plugin/PluginWbPassThrough.cpp:240:
m_spInternetProtocolSink->ReportResult(INET_E_REDIRECTING, 301, L"data:");
I don't think redirecting all requests to 'data:' is a good idea. This should be
done only for predefined types (contentTypeObject and
contentTypeObjectSubrequest, I think). Otherwise IE might present a download
prompt to the user, if we block CSS, for example. Redirecting to
res://mshtml.dll/blank.htm is a safer option here from my experience.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5