Revert 60082 - Add a PhishingClassifier implementation.

The PhishingClassifier ties together the feature extractors and the scorer,
and will be invoked by the RenderView when a page finishes loading.  This
change also makes the feature extractors have a non-owned pointer to the clock
so that it can be shared, and fixes a problem where the DOM feature extractor
cleared out any already-present features.

BUG=none
TEST=PhishingClassifierTest

Review URL: http://codereview.chromium.org/3452010

TBR=bryner@chromium.org
Review URL: http://codereview.chromium.org/3436026

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60091 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 39 insertions, 483 deletions

diff --git a/chrome/chrome_renderer.gypi b/chrome/chrome_renderer.gypi
index cf5219e..38e122f 100644
--- a/chrome/chrome_renderer.gypi
+++ b/chrome/chrome_renderer.gypi
@@ -199,8 +199,6 @@
         '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 8638e0e..acdbca3 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1368,7 +1368,6 @@
         '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',
@@ -1796,8 +1795,6 @@
         '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
deleted file mode 100644
index a783732..0000000
--- a/chrome/renderer/safe_browsing/mock_feature_extractor_clock.h
+++ /dev/null
@@ -1,28 +0,0 @@
-// 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
deleted file mode 100644
index 32fa48e..0000000
--- a/chrome/renderer/safe_browsing/phishing_classifier.cc
+++ /dev/null
@@ -1,168 +0,0 @@
-// 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
deleted file mode 100644
index b460c9a..0000000
--- a/chrome/renderer/safe_browsing/phishing_classifier.h
+++ /dev/null
@@ -1,124 +0,0 @@
-// 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
deleted file mode 100644
index e201dae..0000000
--- a/chrome/renderer/safe_browsing/phishing_classifier_browsertest.cc
+++ /dev/null
@@ -1,134 +0,0 @@
-// 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 a709f7c..db17e1d 100644
--- a/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc
+++ b/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc
@@ -421,6 +421,7 @@ 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 e8fc68a..2b72d46 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.  The caller maintains ownership of the
-  // clock.
+  // and may be mocked for testing.  PhishingDOMFeatureExtractor takes
+  // 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_;
 
-  // Non-owned pointer to our clock.
-  FeatureExtractorClock* clock_;
+  // Owned pointer to our clock.
+  scoped_ptr<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 4f23249..b05ec5f 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,10 +24,17 @@ 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();
-    extractor_.reset(new PhishingDOMFeatureExtractor(view_, &clock_));
+
+    clock_ = new MockClock();
+    extractor_.reset(new PhishingDOMFeatureExtractor(view_, clock_));
   }
 
   virtual void TearDown() {
@@ -51,14 +58,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>"
@@ -116,7 +123,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>"
@@ -158,7 +165,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>";
 
@@ -189,7 +196,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/"] =
@@ -259,7 +266,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.
@@ -296,13 +303,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 0af5bd8..d34ad66 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.  The caller keeps ownership of the clock.
+  // for testing.  PhishingTermFeatureExtractor takes 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_;
 
-  // Non-owned pointer to our clock.
-  FeatureExtractorClock* clock_;
+  // Owned pointer to our clock.
+  scoped_ptr<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 4afa485..812fb93 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,6 +27,11 @@ 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");
@@ -65,11 +70,12 @@ 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
@@ -91,16 +97,17 @@ 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
@@ -191,7 +198,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.
@@ -218,13 +225,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.