diff options
11 files changed, 483 insertions, 39 deletions
diff --git a/chrome/chrome_renderer.gypi b/chrome/chrome_renderer.gypi index 38e122f..cf5219e 100644 --- a/chrome/chrome_renderer.gypi +++ b/chrome/chrome_renderer.gypi @@ -199,6 +199,8 @@ 'renderer/safe_browsing/feature_extractor_clock.h', 'renderer/safe_browsing/features.cc', 'renderer/safe_browsing/features.h', + 'renderer/safe_browsing/phishing_classifier.cc', + 'renderer/safe_browsing/phishing_classifier.h', 'renderer/safe_browsing/phishing_dom_feature_extractor.cc', 'renderer/safe_browsing/phishing_dom_feature_extractor.h', 'renderer/safe_browsing/phishing_term_feature_extractor.cc', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index acdbca3..8638e0e 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1368,6 +1368,7 @@ 'renderer/renderer_about_handler_unittest.cc', 'renderer/renderer_main_unittest.cc', 'renderer/safe_browsing/features_unittest.cc', + 'renderer/safe_browsing/mock_feature_extractor_clock.h', 'renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc', 'renderer/safe_browsing/phishing_url_feature_extractor_unittest.cc', 'renderer/safe_browsing/scorer_unittest.cc', @@ -1795,6 +1796,8 @@ 'renderer/pepper_devices_browsertest.cc', 'renderer/render_view_browsertest.cc', 'renderer/render_view_browsertest_mac.mm', + 'renderer/safe_browsing/mock_feature_extractor_clock.h', + 'renderer/safe_browsing/phishing_classifier_browsertest.cc', 'renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc', 'renderer/safe_browsing/render_view_fake_resources_test.cc', 'renderer/safe_browsing/render_view_fake_resources_test.h', diff --git a/chrome/renderer/safe_browsing/mock_feature_extractor_clock.h b/chrome/renderer/safe_browsing/mock_feature_extractor_clock.h new file mode 100644 index 0000000..a783732 --- /dev/null +++ b/chrome/renderer/safe_browsing/mock_feature_extractor_clock.h @@ -0,0 +1,28 @@ +// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. +// +// A mock implementation of FeatureExtractorClock for testing. + +#ifndef CHROME_RENDERER_SAFE_BROWSING_MOCK_FEATURE_EXTRACTOR_CLOCK_H_ +#define CHROME_RENDERER_SAFE_BROWSING_MOCK_FEATURE_EXTRACTOR_CLOCK_H_ + +#include "chrome/renderer/safe_browsing/feature_extractor_clock.h" +#include "testing/gmock/include/gmock/gmock.h" + +namespace safe_browsing { + +class MockFeatureExtractorClock : public FeatureExtractorClock { + public: + MockFeatureExtractorClock() : FeatureExtractorClock() {} + virtual ~MockFeatureExtractorClock() {} + + MOCK_METHOD0(Now, base::TimeTicks()); + + private: + DISALLOW_COPY_AND_ASSIGN(MockFeatureExtractorClock); +}; + +} // namespace safe_browsing + +#endif // CHROME_RENDERER_SAFE_BROWSING_MOCK_FEATURE_EXTRACTOR_CLOCK_H_ diff --git a/chrome/renderer/safe_browsing/phishing_classifier.cc b/chrome/renderer/safe_browsing/phishing_classifier.cc new file mode 100644 index 0000000..32fa48e --- /dev/null +++ b/chrome/renderer/safe_browsing/phishing_classifier.cc @@ -0,0 +1,168 @@ +// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/renderer/safe_browsing/phishing_classifier.h" + +#include <string> + +#include "base/callback.h" +#include "base/compiler_specific.h" +#include "base/logging.h" +#include "base/sha2.h" +#include "chrome/renderer/render_view.h" +#include "chrome/renderer/safe_browsing/feature_extractor_clock.h" +#include "chrome/renderer/safe_browsing/features.h" +#include "chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h" +#include "chrome/renderer/safe_browsing/phishing_term_feature_extractor.h" +#include "chrome/renderer/safe_browsing/phishing_url_feature_extractor.h" +#include "chrome/renderer/safe_browsing/scorer.h" +#include "googleurl/src/gurl.h" +#include "third_party/WebKit/WebKit/chromium/public/WebFrame.h" +#include "third_party/WebKit/WebKit/chromium/public/WebURL.h" +#include "third_party/WebKit/WebKit/chromium/public/WebView.h" + +namespace safe_browsing { + +const double PhishingClassifier::kInvalidScore = -1.0; +const double PhishingClassifier::kPhishyThreshold = 0.5; + +PhishingClassifier::PhishingClassifier(RenderView* render_view, + const Scorer* scorer, + FeatureExtractorClock* clock) + : render_view_(render_view), + scorer_(scorer), + clock_(clock), + ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) { + url_extractor_.reset(new PhishingUrlFeatureExtractor); + dom_extractor_.reset( + new PhishingDOMFeatureExtractor(render_view_, clock_.get())); + term_extractor_.reset(new PhishingTermFeatureExtractor( + &scorer_->page_terms(), + &scorer_->page_words(), + scorer_->max_words_per_term(), + clock_.get())); + + Clear(); +} + +PhishingClassifier::~PhishingClassifier() { + // The RenderView should have called CancelPendingClassification() before + // we are destroyed. + CheckNoPendingClassification(); +} + +void PhishingClassifier::BeginClassification(const string16* page_text, + DoneCallback* done_callback) { + // The RenderView should have called CancelPendingClassification() before + // starting a new classification, so DCHECK this. + CheckNoPendingClassification(); + // However, in an opt build, we will go ahead and clean up the pending + // classification so that we can start in a known state. + CancelPendingClassification(); + + page_text_ = page_text; + done_callback_.reset(done_callback); + + // For consistency, we always want to invoke the DoneCallback + // asynchronously, rather than directly from this method. To ensure that + // this is the case, post a task to begin feature extraction on the next + // iteration of the message loop. + MessageLoop::current()->PostTask( + FROM_HERE, + method_factory_.NewRunnableMethod( + &PhishingClassifier::BeginFeatureExtraction)); +} + +void PhishingClassifier::BeginFeatureExtraction() { + WebKit::WebView* web_view = render_view_->webview(); + if (!web_view) { + RunFailureCallback(); + return; + } + + WebKit::WebFrame* frame = web_view->mainFrame(); + if (!frame) { + RunFailureCallback(); + return; + } + + features_.reset(new FeatureMap); + if (!url_extractor_->ExtractFeatures(GURL(frame->url()), features_.get())) { + RunFailureCallback(); + return; + } + + // DOM feature extraction can take awhile, so it runs asynchronously + // in several chunks of work and invokes the callback when finished. + dom_extractor_->ExtractFeatures( + features_.get(), + NewCallback(this, &PhishingClassifier::DOMExtractionFinished)); +} + +void PhishingClassifier::CancelPendingClassification() { + // Note that cancelling the feature extractors is simply a no-op if they + // were not running. + dom_extractor_->CancelPendingExtraction(); + term_extractor_->CancelPendingExtraction(); + method_factory_.RevokeAll(); + Clear(); +} + +void PhishingClassifier::DOMExtractionFinished(bool success) { + if (success) { + // Term feature extraction can take awhile, so it runs asynchronously + // in several chunks of work and invokes the callback when finished. + term_extractor_->ExtractFeatures( + page_text_, + features_.get(), + NewCallback(this, &PhishingClassifier::TermExtractionFinished)); + } else { + RunFailureCallback(); + } +} + +void PhishingClassifier::TermExtractionFinished(bool success) { + if (success) { + // Hash all of the features so that they match the model, then compute + // the score. + FeatureMap hashed_features; + for (base::hash_map<std::string, double>::const_iterator it = + features_->features().begin(); + it != features_->features().end(); ++it) { + DCHECK(hashed_features.AddRealFeature(base::SHA256HashString(it->first), + it->second)); + } + + double score = scorer_->ComputeScore(hashed_features); + RunCallback(score >= kPhishyThreshold, score); + } else { + RunFailureCallback(); + } +} + +void PhishingClassifier::CheckNoPendingClassification() { + DCHECK(!done_callback_.get()); + DCHECK(!page_text_); + if (done_callback_.get() || page_text_) { + LOG(ERROR) << "Classification in progress, missing call to " + << "CancelPendingClassification"; + } +} + +void PhishingClassifier::RunCallback(bool phishy, double phishy_score) { + done_callback_->Run(phishy, phishy_score); + Clear(); +} + +void PhishingClassifier::RunFailureCallback() { + RunCallback(false /* not phishy */, kInvalidScore); +} + +void PhishingClassifier::Clear() { + page_text_ = NULL; + done_callback_.reset(NULL); + features_.reset(NULL); +} + +} // namespace safe_browsing diff --git a/chrome/renderer/safe_browsing/phishing_classifier.h b/chrome/renderer/safe_browsing/phishing_classifier.h new file mode 100644 index 0000000..b460c9a --- /dev/null +++ b/chrome/renderer/safe_browsing/phishing_classifier.h @@ -0,0 +1,124 @@ +// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. +// +// This class handles the process of extracting all of the features from a +// page and computing a phishyness score. For more details, see +// phishing_*_feature_extractor.h, scorer.h, and client_model.proto. + +#ifndef CHROME_RENDERER_SAFE_BROWSING_PHISHING_CLASSIFIER_H_ +#define CHROME_RENDERER_SAFE_BROWSING_PHISHING_CLASSIFIER_H_ + +#include "base/basictypes.h" +#include "base/callback.h" +#include "base/scoped_ptr.h" +#include "base/string16.h" +#include "base/task.h" + +class RenderView; + +namespace safe_browsing { +class FeatureExtractorClock; +class FeatureMap; +class PhishingDOMFeatureExtractor; +class PhishingTermFeatureExtractor; +class PhishingUrlFeatureExtractor; +class Scorer; + +class PhishingClassifier { + public: + // Callback to be run when phishing classification finishes. If the first + // argument is true, the page is considered phishy by the client-side model, + // and the browser should ping back to get a final verdict. The second + // argument gives the phishyness score which is used in the pingback, + // or kInvalidScore if classification failed. + typedef Callback2<bool /* phishy */, double /* phishy_score */>::Type + DoneCallback; + + static const double kInvalidScore; + + // Creates a new PhishingClassifier object that will operate on + // |render_view|. |scorer| will be used for computing the final score, and + // must live at least as long as the PhishingClassifier. |clock| is used to + // time feature extractor operations, and the PhishingClassifier takes + // ownership of this object. + PhishingClassifier(RenderView* render_view, + const Scorer* scorer, + FeatureExtractorClock* clock); + ~PhishingClassifier(); + + // Called by the RenderView when a page has finished loading. This begins + // the feature extraction and scoring process. |page_text| should contain + // the plain text of a web page, including any subframes, as returned by + // RenderView::CaptureText(). |page_text| is owned by the caller, and must + // not be destroyed until either |done_callback| is run or + // CancelPendingClassification() is called. + // + // To avoid blocking the render thread for too long, phishing classification + // may run in several chunks of work, posting a task to the current + // MessageLoop to continue processing. Once the scoring process is complete, + // |done_callback| is run on the current thread. PhishingClassifier takes + // ownership of the callback. + void BeginClassification(const string16* page_text, DoneCallback* callback); + + // Called by the RenderView (on the render thread) when a page is unloading + // or the RenderView is being destroyed. This cancels any extraction that + // is in progress. + void CancelPendingClassification(); + + private: + // Any score equal to or above this value is considered phishy. + static const double kPhishyThreshold; + + // Begins the feature extraction process, by extracting URL features and + // beginning DOM feature extraction. + void BeginFeatureExtraction(); + + // Callback to be run when DOM feature extraction is complete. + // If it was successful, begins term feature extraction, otherwise + // runs the DoneCallback with a non-phishy verdict. + void DOMExtractionFinished(bool success); + + // Callback to be run when term feature extraction is complete. + // If it was successful, computes a score and runs the DoneCallback. + // If extraction was unsuccessful, runs the DoneCallback with a + // non-phishy verdict. + void TermExtractionFinished(bool success); + + // Helper to verify that there is no pending phishing classification. Dies + // in debug builds if the state is not as expected. This is a no-op in + // release builds. + void CheckNoPendingClassification(); + + // Helper method to run the DoneCallback and clear the state. + void RunCallback(bool phishy, double phishy_score); + + // Helper to run the DoneCallback when feature extraction has failed. + // This always signals a non-phishy verdict for the page, with kInvalidScore. + void RunFailureCallback(); + + // Clears the current state of the PhishingClassifier. + void Clear(); + + RenderView* render_view_; // owns us + const Scorer* scorer_; // owned by the caller + scoped_ptr<FeatureExtractorClock> clock_; + scoped_ptr<PhishingUrlFeatureExtractor> url_extractor_; + scoped_ptr<PhishingDOMFeatureExtractor> dom_extractor_; + scoped_ptr<PhishingTermFeatureExtractor> term_extractor_; + + // State for any in-progress extraction. + scoped_ptr<FeatureMap> features_; + const string16* page_text_; // owned by the caller + scoped_ptr<DoneCallback> done_callback_; + + // Used to create BeginFeatureExtraction tasks. + // These tasks are revoked if classification is cancelled. + ScopedRunnableMethodFactory<PhishingClassifier> method_factory_; + + DISALLOW_COPY_AND_ASSIGN(PhishingClassifier); +}; + +} // namespace safe_browsing + +#endif // CHROME_RENDERER_SAFE_BROWSING_PHISHING_CLASSIFIER_H_ diff --git a/chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc b/chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc new file mode 100644 index 0000000..e201dae --- /dev/null +++ b/chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc @@ -0,0 +1,134 @@ +// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. +// +// Note that although this is not a "browser" test, it runs as part of +// browser_tests. This is because WebKit does not work properly if it is +// shutdown and re-initialized. Since browser_tests runs each test in a +// new process, this avoids the problem. + +#include "chrome/renderer/safe_browsing/phishing_classifier.h" + +#include <string> + +#include "base/scoped_ptr.h" +#include "base/sha2.h" +#include "base/string16.h" +#include "base/utf_string_conversions.h" +#include "chrome/common/url_constants.h" +#include "chrome/renderer/safe_browsing/client_model.pb.h" +#include "chrome/renderer/safe_browsing/features.h" +#include "chrome/renderer/safe_browsing/mock_feature_extractor_clock.h" +#include "chrome/renderer/safe_browsing/render_view_fake_resources_test.h" +#include "chrome/renderer/safe_browsing/scorer.h" +#include "testing/gmock/include/gmock/gmock.h" + +namespace safe_browsing { + +class PhishingClassifierTest : public RenderViewFakeResourcesTest { + protected: + virtual void SetUp() { + // Set up WebKit and the RenderView. + RenderViewFakeResourcesTest::SetUp(); + + // Construct a model to test with. We include one feature from each of + // the feature extractors, which allows us to verify that they all ran. + ClientSideModel model; + model.add_hashes(base::SHA256HashString(features::kUrlTldToken + + std::string("net"))); + model.add_hashes(base::SHA256HashString(features::kPageLinkDomain + + std::string("phishing.com"))); + model.add_hashes(base::SHA256HashString(features::kPageTerm + + std::string("login"))); + model.add_hashes(base::SHA256HashString("login")); + + // Add a default rule with a non-phishy weight. + ClientSideModel::Rule* rule = model.add_rule(); + rule->set_weight(-1.0); + + // To give a phishy score, the total weight needs to be >= 0 + // (0.5 when converted to a probability). This will only happen + // if all of the listed features are present. + rule = model.add_rule(); + rule->add_feature(0); + rule->add_feature(1); + rule->add_feature(2); + rule->set_weight(1.0); + + model.add_page_term(3); + model.add_page_word(3); + model.set_max_words_per_term(1); + + clock_ = new MockFeatureExtractorClock; + scorer_.reset(Scorer::Create(model.SerializeAsString())); + ASSERT_TRUE(scorer_.get()); + classifier_.reset(new PhishingClassifier(view_, scorer_.get(), clock_)); + } + + virtual void TearDown() { + RenderViewFakeResourcesTest::TearDown(); + } + + // Helper method to start phishing classification and wait for it to + // complete. Returns the success value from the PhishingClassifier's + // DoneCallback, and fills in phishy_score with the score. + bool RunPhishingClassifier(const string16* page_text, double* phishy_score) { + success_ = false; + *phishy_score = PhishingClassifier::kInvalidScore; + + classifier_->BeginClassification( + page_text, + NewCallback(this, &PhishingClassifierTest::ClassificationFinished)); + message_loop_.Run(); + + *phishy_score = phishy_score_; + return success_; + } + + // Completion callback for classification. + void ClassificationFinished(bool success, double phishy_score) { + success_ = success; + phishy_score_ = phishy_score; + message_loop_.Quit(); + } + + scoped_ptr<Scorer> scorer_; + scoped_ptr<PhishingClassifier> classifier_; + MockFeatureExtractorClock* clock_; // owned by classifier_ + + // These members hold the status from the most recent call to the + // ClassificationFinished callback. + bool success_; + double phishy_score_; +}; + +TEST_F(PhishingClassifierTest, TestClassification) { + // This test doesn't exercise the extraction timing. + EXPECT_CALL(*clock_, Now()) + .WillRepeatedly(::testing::Return(base::TimeTicks::Now())); + + responses_["http://host.net/"] = + "<html><body><a href=\"http://phishing.com/\">login</a></body></html>"; + LoadURL("http://host.net/"); + + string16 page_text = ASCIIToUTF16("login"); + double phishy_score; + EXPECT_TRUE(RunPhishingClassifier(&page_text, &phishy_score)); + EXPECT_DOUBLE_EQ(0.5, phishy_score); + + // Change the link domain to something non-phishy. + responses_["http://host.net/"] = + "<html><body><a href=\"http://safe.com/\">login</a></body></html>"; + LoadURL("http://host.net/"); + + EXPECT_FALSE(RunPhishingClassifier(&page_text, &phishy_score)); + EXPECT_GE(phishy_score, 0.0); + EXPECT_LT(phishy_score, 0.5); + + // Extraction should fail for this case, since there is no host. + LoadURL(chrome::kAboutBlankURL); + EXPECT_FALSE(RunPhishingClassifier(&page_text, &phishy_score)); + EXPECT_EQ(phishy_score, PhishingClassifier::kInvalidScore); +} + +} // namespace safe_browsing diff --git a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc index db17e1d..a709f7c 100644 --- a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc +++ b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc @@ -421,7 +421,6 @@ bool PhishingDOMFeatureExtractor::IsExternalDomain(const GURL& url, void PhishingDOMFeatureExtractor::InsertFeatures() { DCHECK(page_feature_state_.get()); - features_->Clear(); if (page_feature_state_->total_links > 0) { // Add a feature for the fraction of times the page links to an external diff --git a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h index 2b72d46..e8fc68a 100644 --- a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h +++ b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h @@ -38,8 +38,8 @@ class PhishingDOMFeatureExtractor { // Creates a PhishingDOMFeatureExtractor for the specified RenderView. // The PhishingDOMFeatureExtrator should be destroyed prior to destroying // the RenderView. |clock| is used for timing feature extractor operations, - // and may be mocked for testing. PhishingDOMFeatureExtractor takes - // ownership of the clock. + // and may be mocked for testing. The caller maintains ownership of the + // clock. PhishingDOMFeatureExtractor(RenderView* render_view, FeatureExtractorClock* clock); ~PhishingDOMFeatureExtractor(); @@ -119,8 +119,8 @@ class PhishingDOMFeatureExtractor { // Non-owned pointer to the view that we will extract features from. RenderView* render_view_; - // Owned pointer to our clock. - scoped_ptr<FeatureExtractorClock> clock_; + // Non-owned pointer to our clock. + FeatureExtractorClock* clock_; // The output parameters from the most recent call to ExtractFeatures(). FeatureMap* features_; // The caller keeps ownership of this. diff --git a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc index b05ec5f..4f23249 100644 --- a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc +++ b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc @@ -12,8 +12,8 @@ #include "base/callback.h" #include "base/message_loop.h" #include "base/time.h" -#include "chrome/renderer/safe_browsing/feature_extractor_clock.h" #include "chrome/renderer/safe_browsing/features.h" +#include "chrome/renderer/safe_browsing/mock_feature_extractor_clock.h" #include "chrome/renderer/safe_browsing/render_view_fake_resources_test.h" #include "testing/gmock/include/gmock/gmock.h" @@ -24,17 +24,10 @@ namespace safe_browsing { class PhishingDOMFeatureExtractorTest : public RenderViewFakeResourcesTest { protected: - class MockClock : public FeatureExtractorClock { - public: - MOCK_METHOD0(Now, base::TimeTicks()); - }; - virtual void SetUp() { // Set up WebKit and the RenderView. RenderViewFakeResourcesTest::SetUp(); - - clock_ = new MockClock(); - extractor_.reset(new PhishingDOMFeatureExtractor(view_, clock_)); + extractor_.reset(new PhishingDOMFeatureExtractor(view_, &clock_)); } virtual void TearDown() { @@ -58,14 +51,14 @@ class PhishingDOMFeatureExtractorTest : public RenderViewFakeResourcesTest { message_loop_.Quit(); } + MockFeatureExtractorClock clock_; scoped_ptr<PhishingDOMFeatureExtractor> extractor_; - MockClock* clock_; // owned by extractor_ bool success_; // holds the success value from ExtractFeatures }; TEST_F(PhishingDOMFeatureExtractorTest, FormFeatures) { // This test doesn't exercise the extraction timing. - EXPECT_CALL(*clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); + EXPECT_CALL(clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); responses_["http://host.com/"] = "<html><head><body>" "<form action=\"query\"><input type=text><input type=checkbox></form>" @@ -123,7 +116,7 @@ TEST_F(PhishingDOMFeatureExtractorTest, FormFeatures) { TEST_F(PhishingDOMFeatureExtractorTest, LinkFeatures) { // This test doesn't exercise the extraction timing. - EXPECT_CALL(*clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); + EXPECT_CALL(clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); responses_["http://www.host.com/"] = "<html><head><body>" "<a href=\"http://www2.host.com/abc\">link</a>" @@ -165,7 +158,7 @@ TEST_F(PhishingDOMFeatureExtractorTest, LinkFeatures) { TEST_F(PhishingDOMFeatureExtractorTest, ScriptAndImageFeatures) { // This test doesn't exercise the extraction timing. - EXPECT_CALL(*clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); + EXPECT_CALL(clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); responses_["http://host.com/"] = "<html><head><script></script><script></script></head></html>"; @@ -196,7 +189,7 @@ TEST_F(PhishingDOMFeatureExtractorTest, ScriptAndImageFeatures) { TEST_F(PhishingDOMFeatureExtractorTest, SubFrames) { // This test doesn't exercise the extraction timing. - EXPECT_CALL(*clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); + EXPECT_CALL(clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); // Test that features are aggregated across all frames. responses_["http://host.com/"] = @@ -266,7 +259,7 @@ TEST_F(PhishingDOMFeatureExtractorTest, Continuation) { // Note that this assumes kClockCheckGranularity = 10 and // kMaxTimePerChunkMs = 50. base::TimeTicks now = base::TimeTicks::Now(); - EXPECT_CALL(*clock_, Now()) + EXPECT_CALL(clock_, Now()) // Time check at the start of extraction. .WillOnce(Return(now)) // Time check at the start of the first chunk of work. @@ -303,13 +296,13 @@ TEST_F(PhishingDOMFeatureExtractorTest, Continuation) { ASSERT_TRUE(ExtractFeatures(&features)); EXPECT_THAT(features.features(), ContainerEq(expected_features.features())); // Make sure none of the mock expectations carry over to the next test. - ::testing::Mock::VerifyAndClearExpectations(clock_); + ::testing::Mock::VerifyAndClearExpectations(&clock_); // Now repeat the test with the same page, but advance the clock faster so // that the extraction time exceeds the maximum total time for the feature // extractor. Extraction should fail. Note that this assumes // kMaxTotalTimeMs = 500. - EXPECT_CALL(*clock_, Now()) + EXPECT_CALL(clock_, Now()) // Time check at the start of extraction. .WillOnce(Return(now)) // Time check at the start of the first chunk of work. diff --git a/chrome/renderer/safe_browsing/phishing_term_feature_extractor.h b/chrome/renderer/safe_browsing/phishing_term_feature_extractor.h index d34ad66..0af5bd8 100644 --- a/chrome/renderer/safe_browsing/phishing_term_feature_extractor.h +++ b/chrome/renderer/safe_browsing/phishing_term_feature_extractor.h @@ -50,7 +50,7 @@ class PhishingTermFeatureExtractor { // destroyed. // // |clock| is used for timing feature extractor operations, and may be mocked - // for testing. PhishingTermFeatureExtractor takes ownership of the clock. + // for testing. The caller keeps ownership of the clock. PhishingTermFeatureExtractor( const base::hash_set<std::string>* page_term_hashes, const base::hash_set<std::string>* page_word_hashes, @@ -129,8 +129,8 @@ class PhishingTermFeatureExtractor { // The maximum number of words in an n-gram. size_t max_words_per_term_; - // Owned pointer to our clock. - scoped_ptr<FeatureExtractorClock> clock_; + // Non-owned pointer to our clock. + FeatureExtractorClock* clock_; // The output parameters from the most recent call to ExtractFeatures(). const string16* page_text_; // The caller keeps ownership of this. diff --git a/chrome/renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc b/chrome/renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc index 812fb93..4afa485 100644 --- a/chrome/renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc +++ b/chrome/renderer/safe_browsing/phishing_term_feature_extractor_unittest.cc @@ -15,8 +15,8 @@ #include "base/stringprintf.h" #include "base/time.h" #include "base/utf_string_conversions.h" -#include "chrome/renderer/safe_browsing/feature_extractor_clock.h" #include "chrome/renderer/safe_browsing/features.h" +#include "chrome/renderer/safe_browsing/mock_feature_extractor_clock.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -27,11 +27,6 @@ namespace safe_browsing { class PhishingTermFeatureExtractorTest : public ::testing::Test { protected: - class MockClock : public FeatureExtractorClock { - public: - MOCK_METHOD0(Now, base::TimeTicks()); - }; - virtual void SetUp() { base::hash_set<std::string> terms; terms.insert("one"); @@ -70,12 +65,11 @@ class PhishingTermFeatureExtractorTest : public ::testing::Test { word_hashes_.insert(base::SHA256HashString(*it)); } - clock_ = new MockClock(); extractor_.reset(new PhishingTermFeatureExtractor( &term_hashes_, &word_hashes_, 3 /* max_words_per_term */, - clock_)); + &clock_)); } // Runs the TermFeatureExtractor on |page_text|, waiting for the @@ -97,17 +91,16 @@ class PhishingTermFeatureExtractorTest : public ::testing::Test { } MessageLoop msg_loop_; + MockFeatureExtractorClock clock_; scoped_ptr<PhishingTermFeatureExtractor> extractor_; base::hash_set<std::string> term_hashes_; base::hash_set<std::string> word_hashes_; - MockClock* clock_; // owned by extractor_ bool success_; // holds the success value from ExtractFeatures }; TEST_F(PhishingTermFeatureExtractorTest, ExtractFeatures) { // This test doesn't exercise the extraction timing. - EXPECT_CALL(*clock_, Now()) - .WillRepeatedly(Return(base::TimeTicks::Now())); + EXPECT_CALL(clock_, Now()).WillRepeatedly(Return(base::TimeTicks::Now())); string16 page_text = ASCIIToUTF16("blah"); FeatureMap expected_features; // initially empty @@ -198,7 +191,7 @@ TEST_F(PhishingTermFeatureExtractorTest, Continuation) { // Note that this assumes kClockCheckGranularity = 10 and // kMaxTimePerChunkMs = 50. base::TimeTicks now = base::TimeTicks::Now(); - EXPECT_CALL(*clock_, Now()) + EXPECT_CALL(clock_, Now()) // Time check at the start of extraction. .WillOnce(Return(now)) // Time check at the start of the first chunk of work. @@ -225,13 +218,13 @@ TEST_F(PhishingTermFeatureExtractorTest, Continuation) { ASSERT_TRUE(ExtractFeatures(&page_text, &features)); EXPECT_THAT(features.features(), ContainerEq(expected_features.features())); // Make sure none of the mock expectations carry over to the next test. - ::testing::Mock::VerifyAndClearExpectations(clock_); + ::testing::Mock::VerifyAndClearExpectations(&clock_); // Now repeat the test with the same text, but advance the clock faster so // that the extraction time exceeds the maximum total time for the feature // extractor. Extraction should fail. Note that this assumes // kMaxTotalTimeMs = 500. - EXPECT_CALL(*clock_, Now()) + EXPECT_CALL(clock_, Now()) // Time check at the start of extraction. .WillOnce(Return(now)) // Time check at the start of the first chunk of work. |