Small modifications to safebrowsing code to make it simpler to add the extension

chrome/browser/safe_browsing/database_manager.h

 // Copyright (c) 2012 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.
 //
 // The Safe Browsing service is responsible for downloading anti-phishing and
 // anti-malware tables and checking urls against them.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_DATABASE_MANAGER_H_
 #define CHROME_BROWSER_SAFE_BROWSING_DATABASE_MANAGER_H_
 
 #include <deque>
+#include <map>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/hash_tables.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/synchronization/lock.h"
 #include "base/time.h"
@@ skipping 18 matching lines @@
 class DownloadProtectionService;
 }
 
 // Construction needs to happen on the main thread.
 class SafeBrowsingDatabaseManager
     : public base::RefCountedThreadSafe<SafeBrowsingDatabaseManager>,
       public SafeBrowsingProtocolManagerDelegate {
  public:
   class Client;
 
-  // Bundle of SafeBrowsing state for one URL or hash prefix check.
+  // Bundle of SafeBrowsing state while performing a URL or hash prefix check.
   struct SafeBrowsingCheck {
-    SafeBrowsingCheck();
+    // |check_type| should correspond to the type of item that is being checked,
+    // either a URL or a binary hash/URL. This is largely for accounting.
+    //
+    // NOTE: since we actually just query strings and are returned a threat
+    // type, this cannot be used to predict the response type (e.g. we can
+    // query the MALWARE list but may be given results for phishing).
+    explicit SafeBrowsingCheck(safe_browsing_util::ListType check_type);

[Scott Hess - ex-Googler, 2013/01/11 23:44:05]

Have you talked to the server people about this?  AFAICT, this only matters if
there is a full-hash collision, which should be very unlikely.  If it's deemed
likely enough to be worth protecting against, then we need to allow per-list
queries.

[not at google - send to devlin, 2013/01/14 23:00:55]

As discussed (err, I think) this is just for knowing which of Client's callback
methods to run. If we just combined all the callbacks into a single ohe, this
would be moot (though you'd go back to needing that "is_download" thing for
accounting in ProtocolManager). I'm going to leave it the way it is here but if
you would prefer me to change it (e.g. combine the callbacks, make it a callback
itself, etc) then I'm happy to change it. In the meantime I re-wrote the
comment.

     ~SafeBrowsingCheck();
 
-    // Either |urls| or |prefix| is used to lookup database.
+    // Either |urls| or |full_hashes| is used to lookup database.
     std::vector<GURL> urls;
-    scoped_ptr<SBFullHash> full_hash;
+    std::vector<SBFullHash> full_hashes;
+
+    // Results from either |urls| or |full_hashes|.
+    std::map<GURL, SBThreatType> url_threats;
+    std::map<SBFullHash, SBThreatType> full_hash_threats;

[Scott Hess - ex-Googler, 2013/01/11 23:44:05]

Having these maps seems over-engineered.  I think you could either:
 - use parallel vectors<SBThreadType> pre-filled with SB_THREAT_TYPE_SAFE.
 - make the vectors above be maps, with default value SB_THREAD_TYPE_SAFE.
 - make the above vectors of pairs, like vector<pair<GURL,SBThreatType>>.

Of course, the latter is exactly like the map version, just more efficient for
storage, and less general for searching.  But it's not clear if searching is
very important for these structures, and they're short.

[not at google - send to devlin, 2013/01/14 23:00:55]

I like the vector<pair<>> one, but it's much simpler to implement this stuff
with parallel vectors so I'll just do it that way.

 
     Client* client;
     bool need_get_hash;
     base::TimeTicks start;  // When check was sent to SB service.
-    SBThreatType threat_type;
-    bool is_download;  // If this check for download url or hash.
+    safe_browsing_util::ListType check_type;  // See comment in constructor.
     std::vector<SBPrefix> prefix_hits;
     std::vector<SBFullHashResult> full_hits;
 
     // Vends weak pointers for TimeoutCallback().  If the response is
     // received before the timeout fires, factory is destructed and
     // the timeout won't be fired.
     // TODO(lzheng): We should consider to use this time out check
     // for browsing too (instead of implementin in
     // safe_browsing_resource_handler.cc).
     scoped_ptr<base::WeakPtrFactory<
@@ skipping 107 matching lines @@
   friend class SafeBrowsingServerTest;
   friend class SafeBrowsingServiceTest;
   friend class SafeBrowsingServiceTestHelper;
 
   typedef std::set<SafeBrowsingCheck*> CurrentChecks;
   typedef std::vector<SafeBrowsingCheck*> GetHashRequestors;
   typedef base::hash_map<SBPrefix, GetHashRequestors> GetHashRequests;
 
   // Clients that we've queued up for checking later once the database is ready.
   struct QueuedCheck {
+    safe_browsing_util::ListType check_type;
     Client* client;
     GURL url;
     base::TimeTicks start;  // When check was queued.
   };
 
   // Called to stop operations on the io_thread. This may be called multiple
   // times during the life of the DatabaseManager. Should be called on IO
   // thread.
   void DoStopOnIOThread();
 
@@ skipping 87 matching lines @@
   // Calls the Client's callback on IO thread after CheckDownloadUrl finishes.
   void CheckDownloadUrlDone(SafeBrowsingCheck* check);
 
   // Calls the Client's callback on IO thread after CheckDownloadHash finishes.
   void CheckDownloadHashDone(SafeBrowsingCheck* check);
 
   // Helper function that calls safe browsing client and cleans up |checks_|.
   void SafeBrowsingCheckDone(SafeBrowsingCheck* check);
 
   // Helper function to set |check| with default values and start a safe
-  // browsing check with timeout of |timeout_ms|. |task| will be called upon
+  // browsing check with timeout of |timeout|. |task| will be called on
   // success, otherwise TimeoutCallback will be called.
-  void StartDownloadCheck(SafeBrowsingCheck* check,
-                          Client* client,
-                          const base::Closure& task,
-                          int64 timeout_ms);
+  void StartSafeBrowsingCheck(SafeBrowsingCheck* check,
+                              Client* client,
+                              const base::Closure& task,
+                              const base::TimeDelta& timeout);
 
   // SafeBrowsingProtocolManageDelegate override
   virtual void ResetDatabase() OVERRIDE;
   virtual void UpdateStarted() OVERRIDE;
   virtual void UpdateFinished(bool success) OVERRIDE;
   virtual void GetChunks(GetChunksCallback callback) OVERRIDE;
   virtual void AddChunks(const std::string& list, SBChunkList* chunks,
                          AddChunksCallback callback) OVERRIDE;
   virtual void DeleteChunks(
       std::vector<SBChunkDelete>* delete_chunks) OVERRIDE;
@@ skipping 42 matching lines @@
   bool database_update_in_progress_;
 
   // Indicates if we're in the midst of trying to close the database.  If this
   // is true, nothing on the IO thread should access the database.
   bool closing_database_;
 
   std::deque<QueuedCheck> queued_checks_;
 
   // When download url check takes this long, client's callback will be called
   // without waiting for the result.
-  int64 download_urlcheck_timeout_ms_;
+  base::TimeDelta download_url_check_timeout_;
 
-  // Similar to |download_urlcheck_timeout_ms_|, but for download hash checks.
-  int64 download_hashcheck_timeout_ms_;
+  // Similar to |download_urlcheck_timeout_| but for download hash checks.
+  base::TimeDelta download_hash_check_timeout_;
+
+  // Similar to |download_urlcheck_timeout_| but for extension ID checks.
+  base::TimeDelta extension_id_check_timeout_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingDatabaseManager);
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_DATABASE_MANAGER_H_