Issue 23127017: Consider the frame structure in Matches()

Issue 23127017: Consider the frame structure in Matches() (Closed)

Created:
Oct. 29, 2013, 3:13 p.m. by Felix Dahlke

Modified:
Nov. 15, 2013, 8:18 a.m.

Reviewers:
Wladimir Palant

Visibility:
Public.

Description

Consider the frame structure in Matches()

Patch Set 2 : Proper exception rule handling #

Total comments: 10

Patch Set 3 : Better test coverage #

Created: Nov. 15, 2013, 8:15 a.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+138 lines, -0 lines)			Patch
M	include/AdblockPlus/FilterEngine.h	View	1	2 chunks	+6 lines, -0 lines	0 comments	Download
M	src/FilterEngine.cpp	View	1 2	1 chunk	+30 lines, -0 lines	0 comments	Download
M	test/FilterEngine.cpp	View	1 2	1 chunk	+102 lines, -0 lines	0 comments	Download

Messages

Total messages: 5

Expand All Messages | Collapse All Messages

Wladimir Palant

http://codereview.adblockplus.org/23127017/diff/1/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/23127017/diff/1/src/FilterEngine.cpp#newcode240 src/FilterEngine.cpp:240: } No, this approach is wrong - and it ...

Oct. 30, 2013, 7:39 a.m. (2013-10-30 07:39:56 UTC) #2

http://codereview.adblockplus.org/23127017/diff/1/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

http://codereview.adblockplus.org/23127017/diff/1/src/FilterEngine.cpp#newcod...
src/FilterEngine.cpp:240: }
No, this approach is wrong - and it seems that we completely forgot to consider
exception rules properly in libadblockplus. There are exceptions that apply to
the particular request URL - and there are those that apply to the frame of that
URL. We need to check the latter first because they will override any blocking
rules for the URL. The approach should be the following:

For each documentUrl in documentUrl call API.checkFilterMatch. The type
parameter should be set to DOCUMENT and documentUrl parameter should be set to
the previous document URL (the frame that's one level higher). For the top-level
frame the URL and document URL parameters will be identical. If any of the
checks return an exception rule then that match should be returned.

Finally, API.checkFilterMatch should be called with the current parameters - the
URL of the request and the URL of the immediate frame that requested it (the
last one in the list I guess). Whatever we get from that call should be
returned.

Feel free to have a look at the checkRequest() function in
adblockpluschrome/webRequest.js if you need inspiration. Note that it makes
sense to have the "old" Matches method call this one with a single-element
vector.

http://codereview.adblockplus.org/23127017/diff/1/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

http://codereview.adblockplus.org/23127017/diff/1/test/FilterEngine.cpp#newco...
test/FilterEngine.cpp:208:
filterEngine->GetFilter("@@adbanner.gif$domain=example.org")->AddToList();
@@||example.org^$document is the exception rule you want to test with, it should
apply if http://example.com/ is anywhere in the frame hierarchy.
@@adbanner.gif$domain=example.org should only apply if the request is being
initiated by the example.org frame immediately, feel free to test this as well.
You probably want to test @@||example.org^document,domain=example.com as well -
that should only match if example.org is a subframe of a document originating
from example.com. And a test for the document domain of the top-level frame
won't hurt either.

Felix Dahlke

Uploaded a new patch, this one should handle exception rules properly. http://codereview.adblockplus.org/23127017/diff/1/src/FilterEngine.cpp File src/FilterEngine.cpp (right): ...

Nov. 3, 2013, 3:51 a.m. (2013-11-03 03:51:15 UTC) #3

Uploaded a new patch, this one should handle exception rules properly.

http://codereview.adblockplus.org/23127017/diff/1/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

http://codereview.adblockplus.org/23127017/diff/1/src/FilterEngine.cpp#newcod...
src/FilterEngine.cpp:240: }
On 2013/10/30 07:39:56, Wladimir Palant wrote:
> No, this approach is wrong - and it seems that we completely forgot to
consider
> exception rules properly in libadblockplus. There are exceptions that apply to
> the particular request URL - and there are those that apply to the frame of
that
> URL. We need to check the latter first because they will override any blocking
> rules for the URL. The approach should be the following:
> 
> For each documentUrl in documentUrl call API.checkFilterMatch. The type
> parameter should be set to DOCUMENT and documentUrl parameter should be set to
> the previous document URL (the frame that's one level higher). For the
top-level
> frame the URL and document URL parameters will be identical. If any of the
> checks return an exception rule then that match should be returned.
> 
> Finally, API.checkFilterMatch should be called with the current parameters -
the
> URL of the request and the URL of the immediate frame that requested it (the
> last one in the list I guess). Whatever we get from that call should be
> returned.
> 
> Feel free to have a look at the checkRequest() function in
> adblockpluschrome/webRequest.js if you need inspiration. Note that it makes
> sense to have the "old" Matches method call this one with a single-element
> vector.

On 2013/10/30 07:39:56, Wladimir Palant wrote:
> No, this approach is wrong - and it seems that we completely forgot to
consider
> exception rules properly in libadblockplus.

I was actually wondering about this after seeing pretty much that logic in
Policy.processNode in ABP for Firefox. Thought it was already taken care of at a
deeper level though.

So that's kind of a big improvement then: We can now support domain-based
whitelisting in Android :)

Wladimir Palant

LGTM with the comments addresses (mostly concerning test coverage). http://codereview.adblockplus.org/23127017/diff/60001/src/FilterEngine.cpp File src/FilterEngine.cpp (right): http://codereview.adblockplus.org/23127017/diff/60001/src/FilterEngine.cpp#newcode241 src/FilterEngine.cpp:241: ...

Nov. 4, 2013, 7:17 a.m. (2013-11-04 07:17:28 UTC) #4

Felix Dahlke

Nov. 15, 2013, 8:18 a.m. (2013-11-15 08:18:19 UTC) #5

Addressed all issues and pushed this.

http://codereview.adblockplus.org/23127017/diff/60001/src/FilterEngine.cpp
File src/FilterEngine.cpp (right):

http://codereview.adblockplus.org/23127017/diff/60001/src/FilterEngine.cpp#ne...
src/FilterEngine.cpp:241: return CheckFilterMatch(url, contentType,
documentUrls.back());
On 2013/11/04 07:17:28, Wladimir Palant wrote:
> Nit: use lastDocumentUrl instead of documentUrls.back()?

Done.

http://codereview.adblockplus.org/23127017/diff/60001/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

http://codereview.adblockplus.org/23127017/diff/60001/test/FilterEngine.cpp#n...
test/FilterEngine.cpp:208:
filterEngine->GetFilter("@@||example.org$document")->AddToList();
On 2013/11/04 07:17:28, Wladimir Palant wrote:
> Nit: This should be @@||example.org^$document (^ is a separator placeholder -
it
> makes sure the domain name actually ends here).

Done.

http://codereview.adblockplus.org/23127017/diff/60001/test/FilterEngine.cpp#n...
test/FilterEngine.cpp:242: documentUrls2);
On 2013/11/04 07:17:28, Wladimir Palant wrote:
> How about an additional test where the document URLs are http://example.org/
and
> http://ads.com/frame/ (in reverse order)? This should return a blocking filter
> and didn't with your original implementation.

Done.

http://codereview.adblockplus.org/23127017/diff/60001/test/FilterEngine.cpp#n...
test/FilterEngine.cpp:250:
filterEngine->GetFilter("@@||example.org$document")->AddToList();
On 2013/11/04 07:17:28, Wladimir Palant wrote:
> Nit: As above, this should be @@||example.org^$document. However, I would
prefer
> testing @@||example.org^$document,domain=ads.com to make sure that the parent
> domain is also checked properly.

Done.

http://codereview.adblockplus.org/23127017/diff/60001/test/FilterEngine.cpp#n...
test/FilterEngine.cpp:268: ASSERT_EQ(AdblockPlus::Filter::TYPE_EXCEPTION,
match2->GetType());
On 2013/11/04 07:17:28, Wladimir Palant wrote:
> I would add a few more tests here:
> 
> * http://example.org/ only - should return TYPE_BLOCKING.
> * http://example.org/, http://ads.com/frame/ - should return TYPE_BLOCKING.
> * http://ads.com/frame/, http://example.org/, http://example.com/ - should
> return TYPE_EXCEPTION.

Done.

Expand All Messages | Collapse All Messages