Bug 338045 (for tony@ponderer.org) r=brettw a=bryner safe browsing slows down page load in non-enhanced mode

browser/components/safebrowsing/content/controller.js

@@ -82,7 +82,6 @@ function PROT_Controller(win, tabWatcher, phishingWarden) {
   this.prefs_.addObserver(this.checkRemotePrefName_, 
                           this.checkRemotePrefObserver);
 
-
   // Global preference to enable the phishing warden
   this.phishWardenPrefName_ = PROT_GlobalStore.getPhishWardenEnabledPrefName();
   this.phishWardenEnabled_ = this.prefs_.getPref(this.phishWardenPrefName_, 
@@ -96,8 +95,6 @@ function PROT_Controller(win, tabWatcher, phishingWarden) {
 
   // Set us up to receive the events we want.
   this.tabWatcher_ = tabWatcher;
-  this.onShutdown_ = BindToObject(this.shutdown, this);
-  this.win_.addEventListener("unload", this.onShutdown_, false);
   this.onTabSwitchCallback_ = BindToObject(this.onTabSwitch, this);
   this.tabWatcher_.registerListener("tabswitch",
                                     this.onTabSwitchCallback_);

browser/components/safebrowsing/content/list-warden.js

@@ -165,15 +165,10 @@ function MultiTableQuerier(url, whiteTables, blackTables, evilCallback) {
   this.debugZone = "multitablequerier";
   this.url_ = url;
 
-  // Since we pop from whiteTables_ and blackTables_, we need to make a copy.
+  this.whiteTables_ = whiteTables;
-  this.whiteTables_ = [];
+  this.blackTables_ = blackTables;
-  this.blackTables_ = [];
+  this.whiteIdx_ = 0;
-  for (var i = 0; i < whiteTables.length; ++i) {
+  this.blackIdx_ = 0;
-    this.whiteTables_.push(whiteTables[i]);
-  }
-  for (var i = 0; i < blackTables.length; ++i) {
-    this.blackTables_.push(blackTables[i]);
-  }
 
   this.evilCallback_ = evilCallback;
   this.listManager_ = Cc["@mozilla.org/url-classifier/listmanager;1"]
@@ -188,21 +183,26 @@ function MultiTableQuerier(url, whiteTables, blackTables, evilCallback) {
  * (i.e., it's not black url).
  */
 MultiTableQuerier.prototype.run = function() {
-  if (this.whiteTables_.length > 0) {
+  var whiteTable = this.whiteTables_[this.whiteIdx_];
-    var tableName = this.whiteTables_.pop();
+  var blackTable = this.blackTables_[this.blackIdx_];
-    G_Debug(this, "Looking in whitetable: " + tableName);
+  if (whiteTable) {
-    this.listManager_.safeExists(tableName, this.url_,
+    //G_Debug(this, "Looking in whitetable: " + whiteTable);
+    ++this.whiteIdx_;
+    this.listManager_.safeExists(whiteTable, this.url_,
                                  BindToObject(this.whiteTableCallback_,
                                               this));
-  } else if (this.blackTables_.length > 0) {
+  } else if (blackTable) {
-    var tableName = this.blackTables_.pop();
+    //G_Debug(this, "Looking in blacktable: " + blackTable);
-    G_Debug(this, "Looking in blacktable: " + tableName);
+    ++this.blackIdx_;
-    this.listManager_.safeExists(tableName, this.url_,
+    this.listManager_.safeExists(blackTable, this.url_,
                                  BindToObject(this.blackTableCallback_,
                                               this));
   } else {
     // No tables left to check, so we quit.
     G_Debug(this, "Not found in any tables: " + this.url_);
+
+    // Break circular ref to callback.
+    this.evilCallback_ = null;
   }
 }
 
@@ -211,7 +211,7 @@ MultiTableQuerier.prototype.run = function() {
  * we can stop.  Otherwise, we call run again.
  */
 MultiTableQuerier.prototype.whiteTableCallback_ = function(isFound) {
-  G_Debug(this, "whiteTableCallback_: " + isFound);
+  //G_Debug(this, "whiteTableCallback_: " + isFound);
   if (!isFound)
     this.run();
   else
@@ -223,7 +223,7 @@ MultiTableQuerier.prototype.whiteTableCallback_ = function(isFound) {
  * we can call the evilCallback and stop.  Otherwise, we call run again.
  */
 MultiTableQuerier.prototype.blackTableCallback_ = function(isFound) {
-  G_Debug(this, "blackTableCallback_: " + isFound);
+  //G_Debug(this, "blackTableCallback_: " + isFound);
   if (!isFound) {
     this.run();
   } else {

browser/components/safebrowsing/content/phishing-warden.js

@@ -87,13 +87,9 @@ function PROT_PhishingWarden() {
   // We use this dude to do lookups on our remote server
   this.fetcher_ = new PROT_TRFetcher();
 
-  // Register w/NavWatcher to hear notifications about requests for docs
+  var wp = Ci.nsIWebProgress;
-  if (!this.testing_) {
+  var wpService = Cc["@mozilla.org/docloaderservice;1"].getService(wp);
-    this.navWatcher_ = new G_NavWatcher();
+  wpService.addProgressListener(this, wp.NOTIFY_STATE_REQUEST);
-    this.navWatcher_.registerListener("docnavstart", 
-                                      BindToObject(this.onDocNavStart, 
-                                                   this));
-  }
 
   // We need to know whether we're enabled and whether we're in advanced
   // mode, so reflect the appropriate preferences into our state.
@@ -125,6 +121,17 @@ function PROT_PhishingWarden() {
 
 PROT_PhishingWarden.inherits(PROT_ListWarden);
 
+/**
+ * We implement nsIWebProgressListener
+ */
+PROT_PhishingWarden.prototype.QueryInterface = function(iid) {
+  if (iid.equals(Ci.nsISupports) || 
+      iid.equals(Ci.nsIWebProgressListener) ||
+      iid.equals(Ci.nsISupportsWeakReference))
+    return this;
+  throw Components.results.NS_ERROR_NO_INTERFACE;
+}
+
 /**
  * When a preference (either advanced features or the phishwarden
  * enabled) changes, we might have to start or stop asking for updates. 
@@ -234,15 +241,10 @@ PROT_PhishingWarden.prototype.onPhishWardenEnabledPrefChanged = function(
 /**
  * A request for a Document has been initiated somewhere. Check it!
  *
- * @param e Event object passed in by the NavWatcher
+ * @param request
+ * @param url
  */ 
-PROT_PhishingWarden.prototype.onDocNavStart = function(e) {
+PROT_PhishingWarden.prototype.onDocNavStart = function(request, url) {
-  if (this.phishWardenEnabled_ !== true) {
-    return;
-  }
-  var url = e.url;
-  var request = e.request;
-
   //G_Debug(this, "phishWarden: " + 
   //        (this.phishWardenEnabled_ ? "enabled" : "disabled"));
   //G_Debug(this, "checkRemote: " +
@@ -260,9 +262,12 @@ PROT_PhishingWarden.prototype.onDocNavStart = function(e) {
   // explicitly disabled.
 
   // If we're on the test page and we're not explicitly disabled
-  if (this.isBlacklistTestURL(url)) {
+  // XXX Do we still need a test url or should each provider just put
-    this.houstonWeHaveAProblem_(request);
+  // it in their local list?
-  }
+  //if (this.isBlacklistTestURL(url)) {
+  //  this.houstonWeHaveAProblem_(request);
+  //  return;
+  //}
   // Either send a request off or check locally
   if (this.checkRemote_) {
     // TODO: Use local whitelists to suppress remote BL lookups. 
@@ -271,9 +276,15 @@ PROT_PhishingWarden.prototype.onDocNavStart = function(e) {
                                    this,
                                    request));
   } else {
-    this.checkUrl(url, BindToObject(this.houstonWeHaveAProblem_,
+    // Check the local lists for a match.
+    // XXX This is to not slow down Tp.  The real solution is to
+    // move all the logic in isEvilURL_ to the background thread.
+    // This involves moving the method into the listmanager.
+    var evilCallback = BindToObject(this.localListMatch_,
                                     this,
-                                    request));
+                                    url,
+                                    request);
+    new G_Alarm(BindToObject(this.checkUrl_, this, url, evilCallback), 1000);
   }
 }
 
@@ -423,24 +434,24 @@ PROT_PhishingWarden.prototype.isBlacklistTestURL = function(url) {
 }
 
 /**
- * Look the URL up in our local blacklists 
+ * Check to see if the url is in the blacklist.
- *
- * @param url URL to check
- * @param callback Function to invoke if there is a problem.
  *
+ * @param url String
+ * @param callback Function
  */
-PROT_PhishingWarden.prototype.checkUrl = function(url, callback) {
+PROT_PhishingWarden.prototype.checkUrl_ = function(url, callback) {
-  G_Debug(this, "Checking URL for " + url);
+  if (!this.isSpurious_(url))
-  // We wrap the callback because we also want
+    this.isEvilURL_(url, callback);
-  // to report the blacklist hit to our data provider.
+}
-  function evilCallback() {
+
-    G_Debug("evilCallback", "Local blacklist hit");
+/**
-    // maybe send a report
+ * Callback for found local blacklist match.  First we report that we have
+ * a blacklist hit, then we bring up the warning dialog.
+ */
+PROT_PhishingWarden.prototype.localListMatch_ = function(url, request) {
+  // Maybe send a report
   (new PROT_Reporter).report("phishblhit", url);
-    callback();
+  this.houstonWeHaveAProblem_(request);
-  }
-  // Check the local lists.
-  this.isEvilURL_(url, evilCallback);
 }
 
 /**
@@ -467,3 +478,86 @@ PROT_PhishingWarden.prototype.checkRemoteData = function(callback,
     G_Debug(this, "Remote blacklist miss");
   }
 }
+
+/**
+ * Helper function to determine whether a given URL is "spurious" for some
+ * definition of "spurious".
+ *
+ * @param url String containing the URL to check
+ * 
+ * @returns Boolean indicating whether Fritz thinks it's too boring to notice
+ */ 
+PROT_PhishingWarden.prototype.isSpurious_ = function(url) {
+  return (url == "about:blank" ||
+          url == "about:config" ||  
+          url.startsWith("chrome://") ||
+          url.startsWith("file://") ||
+          url.startsWith("jar:") ||
+          url.startsWith("javascript:"));
+}
+
+/**
+ * We do our dirtywork on state changes.
+ */
+PROT_PhishingWarden.prototype.onStateChange = function(webProgress, 
+                                                       request, 
+                                                       stateFlags, 
+                                                       status) {
+  var wp = Ci.nsIWebProgressListener;
+
+  // Thanks Darin for helping with this
+  if (stateFlags & wp.STATE_START &&
+      stateFlags & wp.STATE_IS_REQUEST &&
+      request.loadFlags & Ci.nsIChannel.LOAD_DOCUMENT_URI &&
+      this.phishWardenEnabled_ === true) {
+
+    var url;
+    try {
+      url = request.name;
+    } catch(e) { return; }
+
+    G_Debug(this, "firing docnavstart for " + url);
+    this.onDocNavStart(request, url);
+  }
+}
+
+// We don't care about the other kinds of updates (and won't get them since we
+// only signed up for state requests), but we should implement the interface
+// anyway.
+  
+/**
+ * NOP
+ */
+PROT_PhishingWarden.prototype.onLocationChange = function(webProgress, 
+                                                          request, 
+                                                          location) { }
+
+/**
+ * NOP
+ */
+PROT_PhishingWarden.prototype.onProgressChange = function(webProgress, 
+                                                          request, 
+                                                          curSelfProgress, 
+                                                          maxSelfProgress, 
+                                                          curTotalProgress, 
+                                                          maxTotalProgress) { }
+
+/**
+ * NOP
+ */
+PROT_PhishingWarden.prototype.onSecurityChange = function(webProgress, 
+                                                          request, 
+                                                          state) { }
+
+/**
+ * NOP
+ */
+PROT_PhishingWarden.prototype.onStatusChange = function(webProgress, 
+                                                        request, 
+                                                        status, 
+                                                        message) { }
+
+/**
+ * NOP
+ */
+PROT_PhishingWarden.prototype.onLinkIconAvailable = function(browser, aHref) { }

browser/components/safebrowsing/content/sb-loader.js

@@ -40,31 +40,27 @@
  * browser/base/content/global-scripts.inc
  */
 
-window.addEventListener("load", SB_startup, false);
+var safebrowsing = {
+  controller: null,
+  globalStore: null,
+  phishWarden: null,
 
-var SB_controller;  // Exported so it's accessible by child windows
+  startup: function() {
-var SB_appContext;  // The context in which our Application lives
+    // XXX Defer this to make startup faster?
-                    // TODO: appContext does not need to be global
-
-function SB_startup() {
     var Cc = Components.classes;
-  SB_appContext = Cc["@mozilla.org/safebrowsing/application;1"]
+    var appContext = Cc["@mozilla.org/safebrowsing/application;1"]
-                      .getService();
+                     .getService().wrappedJSObject;
-  SB_appContext = SB_appContext.wrappedJSObject;
+    safebrowsing.globalStore = appContext;
 
     // Each new browser window needs its own controller. 
 
     var contentArea = document.getElementById("content");
-  var tabWatcher = new SB_appContext.G_TabbedBrowserWatcher(
-      contentArea,
-      "safebrowsing-watcher",
-      true /*ignore about:blank*/);
-
-  var phishWarden = new SB_appContext.PROT_PhishingWarden();
 
+    var phishWarden = new appContext.PROT_PhishingWarden();
+    safebrowsing.phishWarden = phishWarden;
 
     // Register tables
-  // TODO: move table names to a pref
+    // XXX: move table names to a pref
     phishWarden.registerWhiteTable("goog-white-domain");
     phishWarden.registerWhiteTable("goog-white-url");
     phishWarden.registerBlackTable("goog-black-url");
@@ -72,16 +68,34 @@ function SB_startup() {
 
     // Download/update lists if we're in non-enhanced mode
     phishWarden.maybeToggleUpdateChecking();
-
+    var tabWatcher = new appContext.G_TabbedBrowserWatcher(
-  SB_controller = new SB_appContext.PROT_Controller(
+        contentArea,
+        "safebrowsing-watcher",
+        true /*ignore about:blank*/);
+    safebrowsing.controller = new appContext.PROT_Controller(
         window,
         tabWatcher,
         phishWarden);
 
     // clean up
-  window.removeEventListener("load", SB_startup, false);
+    window.removeEventListener("load", safebrowsing.startup, false);
+  },
+
+  /**
+   * Clean up.
+   */
+  shutdown: function() {
+    safebrowsing.controller.shutdown();
+    window.removeEventListener("unload", safebrowsing.shutdown, false);
+  }
 }
 
+window.addEventListener("load", safebrowsing.startup, false);
+window.addEventListener("unload", safebrowsing.shutdown, false);
+
+
+// XXX Everything below here should be removed from the global namespace and
+// moved into the safebrowsing object.
 
 // Some utils for our UI.
 
@@ -118,7 +132,7 @@ function SB_executeCommandLocally(cmd) {
  * @param link ID of a link for which we should show status text
  */
 function SB_setStatusFor(link) {
-  var gs = SB_appContext.PROT_GlobalStore;
+  var gs = safebrowsing.globalStore;
   var msg;
   if (link == "safebrowsing-palm-faq-link")
     msg = gs.getPhishingFaqURL();