Add an extractor for DOM features to be used for client side phishing detection.

Add an extractor for DOM features to be used for client side phishing detection.

PhishingDOMFeatureExtractor iterates over the page elements and computes a
number of features.  To avoid blocking the renderer for too long, the extractor
may run in several chunks of works, posting a task to continue processing if
necessary.

This CL only includes the feature extraction itself.  I will add the logic to
cap the time per iteration in a follow-up CL.

BUG=none
TEST=PhishingDOMFeatureExtractorTest

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54082

[Brian Ryner, 2010-07-21 22:55:18]

Brett, someone mentioned that you'd be a good person to review the way I'm
setting up the unit test for this, so would be great if you could take a look at
that part of the CL.

[James Hawkins, 2010-07-21 23:12:19]

http://codereview.chromium.org/2878046/diff/1/6
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

http://codereview.chromium.org/2878046/diff/1/6#newcode26
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:26: // Link
related features
Two space indent for these.

http://codereview.chromium.org/2878046/diff/1/7
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h (right):

http://codereview.chromium.org/2878046/diff/1/7#newcode13
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:13: #include
<string>
Blank line after std includes.

http://codereview.chromium.org/2878046/diff/1/7#newcode100
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:100: // The view
that we will extract features from.
Note weak reference.

http://codereview.chromium.org/2878046/diff/1/7#newcode104
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:104: FeatureMap*
features_;
Who owns |features_|? If it's this object, should it be in a scoped_ptr?

http://codereview.chromium.org/2878046/diff/1/7#newcode108
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:108:
WebKit::WebFrame* cur_frame_;
Note weak reference.

http://codereview.chromium.org/2878046/diff/1/8
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc
(right):

http://codereview.chromium.org/2878046/diff/1/8#newcode9
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:9:
#include <string>
Blank line after std includes.

http://codereview.chromium.org/2878046/diff/1/8#newcode40
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:40: //
IPC::Channel::Listener implementation
Colon at end of comment.

http://codereview.chromium.org/2878046/diff/1/8#newcode49
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:49: //
RenderViewVisitor implementation
Colon at end of comment.

[Paweł Hajdan Jr., 2010-07-21 23:28:30]

Drive-by with a test comment. Please let me take another look before committing.

http://codereview.chromium.org/2878046/diff/1/8
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc
(right):

http://codereview.chromium.org/2878046/diff/1/8#newcode79
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:79:
CHECK(channel_->Connect());
Can we use ASSERT_TRUEs in place of CHECKs to avoid crashing entire unit_tests
binary in case of failure? Applies to all CHECKs in this file.

[brettw, 2010-07-22 21:04:10]

I don't see an obvious reason why the test setup is wrong, but I don't think I
would remember if there were any gotchas, I last touched that more than a year
ago.

http://codereview.chromium.org/2878046/diff/1/7
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h (right):

http://codereview.chromium.org/2878046/diff/1/7#newcode124
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:124: }  //
namespace safe_browsing
Blank line before this.

http://codereview.chromium.org/2878046/diff/1/8
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc
(right):

http://codereview.chromium.org/2878046/diff/1/8#newcode40
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:40: //
IPC::Channel::Listener implementation
On 2010/07/21 23:12:19, James Hawkins wrote:
> Colon at end of comment.

He means period

[Brian Ryner, 2010-07-23 21:21:08]

Have another look.

http://codereview.chromium.org/2878046/diff/1/6
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

http://codereview.chromium.org/2878046/diff/1/6#newcode26
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:26: // Link
related features
On 2010/07/21 23:12:19, James Hawkins wrote:
> Two space indent for these.

Done.

http://codereview.chromium.org/2878046/diff/1/7
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h (right):

http://codereview.chromium.org/2878046/diff/1/7#newcode13
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:13: #include
<string>
On 2010/07/21 23:12:19, James Hawkins wrote:
> Blank line after std includes.

Done.

http://codereview.chromium.org/2878046/diff/1/7#newcode100
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:100: // The view
that we will extract features from.
On 2010/07/21 23:12:19, James Hawkins wrote:
> Note weak reference.

Done.

http://codereview.chromium.org/2878046/diff/1/7#newcode104
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:104: FeatureMap*
features_;
On 2010/07/21 23:12:19, James Hawkins wrote:
> Who owns |features_|? If it's this object, should it be in a scoped_ptr?

It's the caller.  Added a comment to clarify.

http://codereview.chromium.org/2878046/diff/1/7#newcode108
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:108:
WebKit::WebFrame* cur_frame_;
On 2010/07/21 23:12:19, James Hawkins wrote:
> Note weak reference.

Done.

http://codereview.chromium.org/2878046/diff/1/7#newcode124
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:124: }  //
namespace safe_browsing
On 2010/07/22 21:04:10, brettw wrote:
> Blank line before this.

Done.

http://codereview.chromium.org/2878046/diff/1/8
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc
(right):

http://codereview.chromium.org/2878046/diff/1/8#newcode9
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:9:
#include <string>
On 2010/07/21 23:12:19, James Hawkins wrote:
> Blank line after std includes.

Done.

http://codereview.chromium.org/2878046/diff/1/8#newcode40
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:40: //
IPC::Channel::Listener implementation
On 2010/07/22 21:04:10, brettw wrote:
> On 2010/07/21 23:12:19, James Hawkins wrote:
> > Colon at end of comment.
> 
> He means period

Looking at similar code, I see some that uses colon and some with period.  I
opted for a period here.

http://codereview.chromium.org/2878046/diff/1/8#newcode49
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:49: //
RenderViewVisitor implementation
On 2010/07/21 23:12:19, James Hawkins wrote:
> Colon at end of comment.

Same here.

http://codereview.chromium.org/2878046/diff/1/8#newcode79
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:79:
CHECK(channel_->Connect());
On 2010/07/21 23:28:31, Paweł Hajdan Jr. wrote:
> Can we use ASSERT_TRUEs in place of CHECKs to avoid crashing entire unit_tests
> binary in case of failure? Applies to all CHECKs in this file.

Good point, done.

[James Hawkins, 2010-07-23 21:27:39]

LGTM

http://codereview.chromium.org/2878046/diff/9001/10006
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h (right):

http://codereview.chromium.org/2878046/diff/9001/10006#newcode105
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:105: FeatureMap*
features_;  // the caller keeps ownership of this
Capitalize The. Period at end of sentence.

[Paweł Hajdan Jr., 2010-07-23 22:30:51]

Code I commented in the drive-by LGTM.

[lzheng, 2010-07-26 17:51:28]

http://codereview.chromium.org/2878046/diff/9001/10003
File chrome/renderer/safe_browsing/features.cc (right):

http://codereview.chromium.org/2878046/diff/9001/10003#newcode21
chrome/renderer/safe_browsing/features.cc:21: bool
FeatureMap::AddRealFeature(const std::string& name, double value) {
Since only value [0~1.0] is expected, we should check the value to make sure it
is within that range and return false if not.

http://codereview.chromium.org/2878046/diff/9001/10007
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc
(right):

http://codereview.chromium.org/2878046/diff/9001/10007#newcode7
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:7:
#include <string.h>  // for memcpy()
nit: move this after <string>?

[Brian Ryner, 2010-07-27 00:20:05]

Have another look.

Thanks,
Brian

http://codereview.chromium.org/2878046/diff/9001/10003
File chrome/renderer/safe_browsing/features.cc (right):

http://codereview.chromium.org/2878046/diff/9001/10003#newcode21
chrome/renderer/safe_browsing/features.cc:21: bool
FeatureMap::AddRealFeature(const std::string& name, double value) {
On 2010/07/26 17:51:28, lzheng wrote:
> Since only value [0~1.0] is expected, we should check the value to make sure
it
> is within that range and return false if not.

Done.  I made this log to a histogram like the TooManyFeatures case, since if
this happens for users it indicates a bug in the code.

http://codereview.chromium.org/2878046/diff/9001/10006
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h (right):

http://codereview.chromium.org/2878046/diff/9001/10006#newcode105
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:105: FeatureMap*
features_;  // the caller keeps ownership of this
On 2010/07/23 21:27:42, James Hawkins wrote:
> Capitalize The. Period at end of sentence.

Done.

http://codereview.chromium.org/2878046/diff/9001/10007
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc
(right):

http://codereview.chromium.org/2878046/diff/9001/10007#newcode7
chrome/renderer/safe_browsing/phishing_dom_feature_extractor_unittest.cc:7:
#include <string.h>  // for memcpy()
On 2010/07/26 17:51:28, lzheng wrote:
> nit: move this after <string>?

As I understand it, C library headers should come before C++ library headers:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

[lzheng, 2010-07-27 00:41:13]

LGTM.

Please make sure other reviewers are fine.

Thanks!

[Brian Ryner, 2010-07-27 04:47:29]

On 2010/07/27 00:41:13, lzheng wrote:
> LGTM.
> 
> Please make sure other reviewers are fine.
> 
> Thanks!

Just made one more minor tweak to the comment for AddRealFeature to clarify that
it now returns false if the feature value is outside of the allowed range.

[marria, 2010-07-27 21:31:51]

lgtm

http://codereview.chromium.org/2878046/diff/22001/14007
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

http://codereview.chromium.org/2878046/diff/22001/14007#newcode49
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:49: :
external_links(0),
You could make this external_links and internal_links, for consistency with
action_other_domain/action_same_domain, img_other_domain/img_same_domain.  Up to
you though.

http://codereview.chromium.org/2878046/diff/22001/14008
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h (right):

http://codereview.chromium.org/2878046/diff/22001/14008#newcode91
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:91: // Given a
URL, checks whether the domain is the same as the domain of the
is _not_ the same, right?

[Brian Ryner, 2010-07-27 23:25:46]

Have one more look.  I also added a missing #include to the unittest, which
showed up once I synced my client to the trunk.

http://codereview.chromium.org/2878046/diff/22001/14007
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right):

http://codereview.chromium.org/2878046/diff/22001/14007#newcode49
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:49: :
external_links(0),
On 2010/07/27 21:31:51, marria wrote:
> You could make this external_links and internal_links, for consistency with
> action_other_domain/action_same_domain, img_other_domain/img_same_domain.  Up
to
> you though.

I think it's useful to have total_links since we compute two separate ratios
from it (external and secure links).  This makes internal_links redundant, so
I'll leave that part as-is.

However, I do like the idea of making things more consistent, so I switched the
image and action counts to be "other_domain" and "total", rather than
"other_domain" and "same_domain".  I think this makes things a little nicer and
simplifies InsertFeatures a bit.

http://codereview.chromium.org/2878046/diff/22001/14008
File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h (right):

http://codereview.chromium.org/2878046/diff/22001/14008#newcode91
chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:91: // Given a
URL, checks whether the domain is the same as the domain of the
On 2010/07/27 21:31:51, marria wrote:
> is _not_ the same, right?

Good catch, fixed.

[marria, 2010-07-28 00:48:09]

lgtm

[lzheng1, 2010-07-28 09:17:41]

LGTM

On Tue, Jul 27, 2010 at 5:48 PM, <marria@google.com> wrote:

> lgtm
>
>
> http://codereview.chromium.org/2878046/show
>