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

Issue 6299667012780032: Issues #696,1231,1264,1265 - Improve handling in PassthroughApp (Closed)

Created:
Sept. 19, 2014, 2:41 p.m. by sergei
Modified:
Oct. 20, 2014, 7:43 a.m.
Reviewers:
Eric, Oleksandr
CC:
Felix Dahlke
Visibility:
Public.

Description

Since the changes for these issues are quite coupled and they all are mainly in PassthroughAPP we decided to put them in one codereview. #696 Error page is displayed instead of blocked ad - Solved by feeding IE the empty page. Sometimes it's a white rectangle, sometimes it's hidden. #1231 video doesn't play with ABP for IE - object-subrequest was not handled at all, now we detect it but not completely #1264 Figure out the best way to block subdomain requests - For subdocument requests generate the empty html page, see #696 #1265 Figure out a method to detect the type of the request correctly - We can obtain Accept header in BeginningTransaction and make a decision here. - We also detect object-subrequest requests. Changes: - add OBJECT-SUBREQUEST to CPluginFilter - 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. - [PluginWbPassThrough.cpp] 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 `handled` argument to our protocol Sink `OnStart` method. When `handled` is set to true the original (target) protocol::Start is not called. Before it was up to the return code, if it's successful then call it. But we need to return the successful code and don't need the call of the original protocol impl because it messes the workflow and crashes. - rename `WBPassthruSink::Read` to `WBPassthruSink::OnRead`, because it's sink and its methods are some kind of slots and the proper names are `OnSomeEvent`, there is even already `OnStart`. - get rid of `WBPassthruSink::m_lastDataReported` because it's not used anymore. Add `WBPassthruSink::m_currentPositionOfSentPage` which is used by `WBPassthruSink::OnRead`. - move `WBPassthruSink::m_shouldBlock` to `WBPassthru::m_shouldSupplyCustomContent`. When we send custom response we don't initiate any original protocol calls, thus we don't need to call its LockRequest/UnlockRequest. To support that logic WBPassthru::m_hasOriginalStartCalled is added. They (LockRequest/UnlockRequest) crash if original Start is not called. As well it cannot be in `WBPassthruSink` because it is destroyed before `UnlockRequest` is called. 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.

Patch Set 1 #

Total comments: 21

Patch Set 2 : Get rid of yoda style and rename blockedByABPPageSize local var #

Patch Set 3 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+262 lines, -164 lines) Patch
M src/plugin/AdblockPlusDomTraverser.cpp View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M src/plugin/PluginFilter.cpp View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M src/plugin/PluginTabBase.cpp View 1 2 1 chunk +0 lines, -3 lines 0 comments Download
M src/plugin/PluginWbPassThrough.h View 1 2 4 chunks +26 lines, -10 lines 0 comments Download
M src/plugin/PluginWbPassThrough.cpp View 1 2 8 chunks +215 lines, -139 lines 0 comments Download
M src/plugin/SinkPolicy.h View 1 chunk +1 line, -1 line 0 comments Download
M src/plugin/SinkPolicy.inl View 1 2 1 chunk +15 lines, -9 lines 0 comments Download

Messages

Total messages: 9
Oleksandr
http://codereview.adblockplus.org/6299667012780032/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/6299667012780032/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: : m_currentPositionOfSentPage(0) I wouldn't be entirely sure that IE ...
Sept. 21, 2014, 11:30 p.m. (2014-09-21 23:30:40 UTC) #1
sergei
http://codereview.adblockplus.org/6299667012780032/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/6299667012780032/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: : m_currentPositionOfSentPage(0) On 2014/09/21 23:30:40, Oleksandr wrote: > I ...
Sept. 22, 2014, 4:18 p.m. (2014-09-22 16:18:05 UTC) #2
Oleksandr
Awesome! LGTM http://codereview.adblockplus.org/6299667012780032/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp File src/plugin/PluginWbPassThrough.cpp (right): http://codereview.adblockplus.org/6299667012780032/diff/5629499534213120/src/plugin/PluginWbPassThrough.cpp#newcode26 src/plugin/PluginWbPassThrough.cpp:26: : m_currentPositionOfSentPage(0) Even though I can't provide ...
Oct. 3, 2014, 2:36 p.m. (2014-10-03 14:36:37 UTC) #3
Oleksandr
Also, doesn't this also fix #1238 (back button not working) issue? On 2014/10/03 14:36:37, Oleksandr ...
Oct. 4, 2014, 7:52 a.m. (2014-10-04 07:52:01 UTC) #4
sergei
On 2014/10/04 07:52:01, Oleksandr wrote: > Also, doesn't this also fix #1238 (back button not ...
Oct. 6, 2014, 8:40 a.m. (2014-10-06 08:40:47 UTC) #5
Oleksandr
Rebase LGTM. As agreed, let's push this and if there are comments from other reviewers ...
Oct. 17, 2014, 1:44 p.m. (2014-10-17 13:44:26 UTC) #6
sergei
On 2014/10/17 13:44:26, Oleksandr wrote: > Rebase LGTM. As agreed, let's push this and if ...
Oct. 17, 2014, 1:59 p.m. (2014-10-17 13:59:04 UTC) #7
Eric
The only concern I wanted to state at this point (after the code has been ...
Oct. 20, 2014, 4:17 a.m. (2014-10-20 04:17:29 UTC) #8
sergei
Oct. 20, 2014, 7:43 a.m. (2014-10-20 07:43:41 UTC) #9
On 2014/10/20 04:17:29, Eric wrote:
> The only concern I wanted to state at this point (after the code has been
> pushed) is that by modifying the SinkPolicy.* files we can no longer directly
> import future updates to PassthroughAPP.
> 
> Given that the code that was modified in those files was templated, it seems
we
> could keep compatibility with the external code by using a template
> specialization rather than modify the external code itself.

Thanks a lot, we can do it via template specialization, I've created #1484.

Powered by Google App Engine
This is Rietveld