Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4974480757620736: Issue #1356 - Improve detection of the issuer of the request (Closed)

Created:
Oct. 14, 2014, 11:23 p.m. by Oleksandr
Modified:
Nov. 5, 2014, 4:51 p.m.
Reviewers:
sergei, Felix Dahlke, Eric
Visibility:
Public.

Description

This builds on http://codereview.adblockplus.org/6390087684194304/, and will very likely have tons of conflicts when merging. Which is why I submit this as an overview of the idea in general. There will most likely be another patch with exact code to be committed after we push the patches from the above code review. The idea is that we can get the real Referer header, which is available on BeginningTransaction AFTER the original BeginningTransaction is called. Doing that leaves no reason to have our ShouldBlock check running in OnStart, so all that code is moved to BeginningTransaction as well. It works and it finally fixes the issue.

Patch Set 1 #

Total comments: 16

Patch Set 2 : Update to the repositorory tip #

Total comments: 11

Patch Set 3 : Cleanup #

Total comments: 2

Patch Set 4 : Revert the incorrect cleanup #

Total comments: 16

Patch Set 5 : Refactoring lambda and ExtractHttpHeaders functions. Coding style cleanup. #

Total comments: 5

Patch Set 6 : Revert refactoring of ExtractHttpHeaders. Keeping it simple. Typo fixes. #

Total comments: 3

Patch Set 7 : Coding style changes #

Total comments: 6

Patch Set 8 : Reorder stuff in BeginningTransaction for clarity. #

Patch Set 9 : Rename the parameter #

Unified diffs Side-by-side diffs Delta from patch set Stats (+118 lines, -207 lines) Patch
M src/plugin/PluginFilter.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/PluginWbPassThrough.h View 1 2 3 4 2 chunks +1 line, -1 line 0 comments Download
M src/plugin/PluginWbPassThrough.cpp View 1 2 3 4 5 6 7 8 6 chunks +116 lines, -204 lines 0 comments Download
M src/plugin/SinkPolicy.inl View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 23
Oleksandr
Oct. 14, 2014, 11:34 p.m. (2014-10-14 23:34:41 UTC) #1
Eric
Looks OK. Merging this directly does indeed look difficult, given the number of changes since ...
Oct. 15, 2014, 5:02 p.m. (2014-10-15 17:02:32 UTC) #2
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode255 src/plugin/PluginWbPassThrough.cpp:255: auto acceptHeader = [&]() -> std::string On 2014/10/15 17:02:33, ...
Oct. 24, 2014, 9:59 a.m. (2014-10-24 09:59:18 UTC) #3
sergei
- http://www.netzwelt.de still does not work as expected. - BTW, as a privacy feature we ...
Oct. 28, 2014, 2:08 p.m. (2014-10-28 14:08:45 UTC) #4
sergei
On 2014/10/28 14:08:45, sergei wrote: > - http://www.netzwelt.de still does not work as expected. Awesome, ...
Oct. 28, 2014, 2:19 p.m. (2014-10-28 14:19:52 UTC) #5
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode284 src/plugin/PluginWbPassThrough.cpp:284: m_boundDomain = ExtractHTTPHeader(std::wstring(*pszAdditionalHeaders), L"Referer:").c_str(); On 2014/10/28 14:08:46, sergei wrote: ...
Oct. 30, 2014, 11:40 p.m. (2014-10-30 23:40:48 UTC) #6
sergei
http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode284 src/plugin/PluginWbPassThrough.cpp:284: m_boundDomain = ExtractHTTPHeader(std::wstring(*pszAdditionalHeaders), L"Referer:").c_str(); On 2014/10/30 23:40:48, Oleksandr wrote: ...
Oct. 31, 2014, 12:47 p.m. (2014-10-31 12:47:57 UTC) #7
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5700735861784576/src/plugin/SinkPolicy.inl File src/plugin/SinkPolicy.inl (left): http://codereview.adblockplus.org/4974480757620736/diff/5700735861784576/src/plugin/SinkPolicy.inl#oldcode387 src/plugin/SinkPolicy.inl:387: if (E_ABORT == hr && pSink->m_blockedInTransaction) On 2014/10/31 12:47:57, ...
Oct. 31, 2014, 3:22 p.m. (2014-10-31 15:22:07 UTC) #8
sergei
LGTM
Oct. 31, 2014, 3:40 p.m. (2014-10-31 15:40:54 UTC) #9
Felix Dahlke
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/plugin/PluginWbPassThrough.cpp#newcode203 src/plugin/PluginWbPassThrough.cpp:203: std::wstring src = szURL; Shouldn't this be stored as ...
Nov. 3, 2014, 4:58 a.m. (2014-11-03 04:58:16 UTC) #10
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/plugin/PluginWbPassThrough.cpp#newcode203 src/plugin/PluginWbPassThrough.cpp:203: std::wstring src = szURL; On 2014/11/03 04:58:16, Felix H. ...
Nov. 3, 2014, 11:37 a.m. (2014-11-03 11:37:37 UTC) #11
sergei
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h File src/shared/Utils.h (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h#newcode54 src/shared/Utils.h:54: T ExtractHTTPHeader(const T& allHeaders, const T& targetHeaderName, const T& ...
Nov. 3, 2014, 2:17 p.m. (2014-11-03 14:17:46 UTC) #12
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h File src/shared/Utils.h (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h#newcode54 src/shared/Utils.h:54: T ExtractHTTPHeader(const T& allHeaders, const T& targetHeaderName, const T& ...
Nov. 4, 2014, 12:02 p.m. (2014-11-04 12:02:42 UTC) #13
Felix Dahlke
http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h File src/shared/Utils.h (right): http://codereview.adblockplus.org/4974480757620736/diff/5681717746597888/src/shared/Utils.h#newcode54 src/shared/Utils.h:54: T ExtractHTTPHeader(const T& allHeaders, const T& targetHeaderName, const T& ...
Nov. 4, 2014, 4:13 p.m. (2014-11-04 16:13:25 UTC) #14
Oleksandr
Nov. 4, 2014, 11:47 p.m. (2014-11-04 23:47:03 UTC) #15
sergei
http://codereview.adblockplus.org/4974480757620736/diff/5678807906254848/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5678807906254848/src/plugin/PluginWbPassThrough.cpp#newcode39 src/plugin/PluginWbPassThrough.cpp:39: std::string ExtractHttpAcceptHeader(IInternetProtocol* internetProtocol) Would you shift this function one ...
Nov. 5, 2014, 12:19 a.m. (2014-11-05 00:19:36 UTC) #16
Oleksandr
Nov. 5, 2014, 12:27 a.m. (2014-11-05 00:27:23 UTC) #17
sergei
On 2014/11/05 00:27:23, Oleksandr wrote: LGTM
Nov. 5, 2014, 7:17 a.m. (2014-11-05 07:17:15 UTC) #18
Felix Dahlke
http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName); I still find it unintuitive ...
Nov. 5, 2014, 9:04 a.m. (2014-11-05 09:04:52 UTC) #19
Oleksandr
http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName); On 2014/11/05 09:04:52, Felix H. ...
Nov. 5, 2014, 10:01 a.m. (2014-11-05 10:01:57 UTC) #20
Felix Dahlke
LGTM!
Nov. 5, 2014, 10:03 a.m. (2014-11-05 10:03:42 UTC) #21
sergei
http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: auto targetHeaderBeginsAt = allHeaders.find(targetHeaderName); On 2014/11/05 10:01:58, Oleksandr wrote: ...
Nov. 5, 2014, 10:28 a.m. (2014-11-05 10:28:59 UTC) #22
Oleksandr
Nov. 5, 2014, 4:42 p.m. (2014-11-05 16:42:49 UTC) #23
Renamed the parameter. Closing this and pushing.

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

http://codereview.adblockplus.org/4974480757620736/diff/5630721452408832/src/...
src/plugin/PluginWbPassThrough.cpp:26: auto targetHeaderBeginsAt =
allHeaders.find(targetHeaderName);
On 2014/11/05 10:28:59, sergei wrote:
> On 2014/11/05 10:01:58, Oleksandr wrote:
> > On 2014/11/05 09:04:52, Felix H. Dahlke wrote:
> > > I still find it unintuitive that the colon is part of "header name" here.
> Why
> > > not just append the colon here?
> > > 
> > > More importantly, header names are generally case insensitive, are we sure
> > both
> > > HTTP_QUERY_RAW_HEADERS_CRLF and pszAdditionalHeaders will always return
them
> > in
> > > a way our case sensitive comparison expects?
> > 
> > 1. Colon is part of header name because this is a templated function and
> > otherwise we'll end up with stuff CreateConstString (see Patchset 5). This
is
> to
> > keep it simple.
> > 2. The headers we get from IE are constructed by IE (not some third party
> code).
> > So it is always in the same format. Besides, if we wanted to convert
> everything
> > to lower case ::tolower wouldn't work (since it's meant to be used for chars
> > only, not wchars. So we'll make things unnecessarily complex again.
> 
> 1. T is std::string or std::wstring. The variant with template specialization
> looks like overengineering here. May be such functions like
> `ConvertToResult<From, To>::Type ConvertTo<To>(const From&)` could help but we
> don't have them as well as we can use something like `static_cast<typename
> T::value_type>(':')`, but it still smells. What about
> `targetHeaderNameWithColon`?
> 2. It works now, but I also think it's important. I would say that content
type
> should also be case-insensitive.
> BTW, the headers are not always constructed by IE. `BeginningTransaction` is
> provided by the client and the client sets or modifies the header.  (as I
> pointed before we can actually use it to implement additional features like
> hiding the real user agent). A real example is flash addon, as I see it adds
its
> own header which we can use for better object-subrequest detection as well
> (should be as another issue).
> I think that we can discuss the details how it should be implemented in the
> issue tracker. I would like to have it as a good-first-bug, it's an excellent
> challenging task which can wait for now I think ;)

Created https://issues.adblockplus.org/ticket/1529 on this.

Powered by Google App Engine
This is Rietveld