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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by sergei
Modified:
4 years, 9 months ago
Reviewers:
Oleksandr, Eric
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 ...
4 years, 10 months ago (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 ...
4 years, 10 months ago (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 ...
4 years, 9 months ago (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 ...
4 years, 9 months ago (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 ...
4 years, 9 months ago (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 ...
4 years, 9 months ago (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 ...
4 years, 9 months ago (2014-10-17 13:59:04 UTC) #7
Eric
The only concern I wanted to state at this point (after the code has been ...
4 years, 9 months ago (2014-10-20 04:17:29 UTC) #8
sergei
4 years, 9 months ago (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.
Sign in to reply to this message.

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