Bug 1254323 - Reduce identical gethash requests done by the URL Classifier. r?gcp draft
authorDimiL <dlee@mozilla.com>
Wed, 14 Feb 2018 16:12:29 -0800
changeset 757119 0feba3a8483641a22d60e99310e8e4073dc7a277
parent 753210 c4c9b0c5796bd6786b432597b6fbe98ffcf1340b
push id99672
push userfmarier@mozilla.com
push dateTue, 20 Feb 2018 01:30:47 +0000
reviewersgcp
bugs1254323
milestone60.0a1
Bug 1254323 - Reduce identical gethash requests done by the URL Classifier. r?gcp MozReview-Commit-ID: KNNL1dBqXx0
toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
--- a/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
+++ b/toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
@@ -161,16 +161,19 @@ FullHashMatch.prototype = {
   cacheDuration: null,
 };
 
 function HashCompleter() {
   // The current HashCompleterRequest in flight. Once it is started, it is set
   // to null. It may be used by multiple calls to |complete| in succession to
   // avoid creating multiple requests to the same gethash URL.
   this._currentRequest = null;
+  // An Array of ongoing gethash requests which is used to find requests for
+  // the same hash prefix.
+  this._ongoingRequests = [];
   // A map of gethashUrls to HashCompleterRequests that haven't yet begun.
   this._pendingRequests = {};
 
   // A map of gethash URLs to RequestBackoff objects.
   this._backoffs = {};
 
   // Whether we have been informed of a shutdown by the shutdown event.
   this._shuttingDown = false;
@@ -196,16 +199,25 @@ HashCompleter.prototype = {
   // This is mainly how the HashCompleter interacts with other components.
   // Even though it only takes one partial hash and callback, subsequent
   // calls are made into the same HTTP request by using a thread dispatch.
   complete: function HC_complete(aPartialHash, aGethashUrl, aTableName, aCallback) {
     if (!aGethashUrl) {
       throw Cr.NS_ERROR_NOT_INITIALIZED;
     }
 
+    // Check ongoing requests before creating a new HashCompleteRequest
+    for (let r of this._ongoingRequests) {
+      if (r.find(aPartialHash, aGethashUrl, aTableName)) {
+        log("Merge gethash request in " + aTableName + " for prefix : " + btoa(aPartialHash));
+        r.add(aPartialHash, aCallback, aTableName);
+        return;
+      }
+    }
+
     if (!this._currentRequest) {
       this._currentRequest = new HashCompleterRequest(this, aGethashUrl);
     }
     if (this._currentRequest.gethashUrl == aGethashUrl) {
       this._currentRequest.add(aPartialHash, aCallback, aTableName);
     } else {
       if (!this._pendingRequests[aGethashUrl]) {
         this._pendingRequests[aGethashUrl] =
@@ -257,28 +269,32 @@ HashCompleter.prototype = {
     if (!this._currentRequest && (pendingUrls.length > 0)) {
       let nextUrl = pendingUrls[0];
       this._currentRequest = this._pendingRequests[nextUrl];
       delete this._pendingRequests[nextUrl];
     }
 
     if (this._currentRequest) {
       try {
-        this._currentRequest.begin();
+        if (this._currentRequest.begin()) {
+          this._ongoingRequests.push(this._currentRequest);
+        }
       } finally {
         // If |begin| fails, we should get rid of our request.
         this._currentRequest = null;
       }
     }
   },
 
   // Pass the server response status to the RequestBackoff for the given
   // gethashUrl and fetch the next pending request, if there is one.
-  finishRequest(url, aStatus) {
-    this._backoffs[url].noteServerResponse(aStatus);
+  finishRequest(aRequest, aStatus) {
+    this._ongoingRequests = this._ongoingRequests.filter(v => v != aRequest);
+
+    this._backoffs[aRequest.gethashUrl].noteServerResponse(aStatus);
     Services.tm.dispatchToMainThread(this);
   },
 
   // Returns true if we can make a request from the given url, false otherwise.
   canMakeRequest(aGethashUrl) {
     return this._backoffs[aGethashUrl].canMakeRequest() &&
            Date.now() >= this._nextGethashTimeMs[aGethashUrl];
   },
@@ -346,29 +362,42 @@ HashCompleterRequest.prototype = {
       let isTableNameV4 = aTableName.endsWith("-proto");
       if (0 === this.tableNames.size) {
         // Decide if this request is v4 by the first added partial hash.
         this.isV4 = isTableNameV4;
       } else if (this.isV4 !== isTableNameV4) {
         log('ERROR: Cannot mix "proto" tables with other types within ' +
             "the same gethash URL.");
       }
-      this.tableNames.set(aTableName);
+      if (!this.tableNames.has(aTableName)) {
+        this.tableNames.set(aTableName);
+      }
 
       // Assuming all tables with the same gethash URL have the same provider
       if (this.provider == "") {
         this.provider = gUrlUtil.getProvider(aTableName);
       }
 
       if (this.telemetryProvider == "") {
         this.telemetryProvider = gUrlUtil.getTelemetryProvider(aTableName);
       }
     }
   },
 
+  find: function HCR_find(aPartialHash, aGetHashUrl, aTableName) {
+    if (this.gethashUrl != aGetHashUrl ||
+        !this.tableNames.has(aTableName)) {
+      return false;
+    }
+
+    return this._requests.find(function(r) {
+      return r.partialHash === aPartialHash;
+    });
+  },
+
   fillTableStatesBase64: function HCR_fillTableStatesBase64(aCallback) {
     gDbService.getTables(aTableData => {
       aTableData.split("\n").forEach(line => {
         let p = line.indexOf(";");
         if (-1 === p) {
           return;
         }
         // [tableName];[stateBase64]:[checksumBase64]
@@ -387,36 +416,40 @@ HashCompleterRequest.prototype = {
 
   // This initiates the HTTP request. It can fail due to backoff timings and
   // will notify all callbacks as necessary. We notify the backoff object on
   // begin.
   begin: function HCR_begin() {
     if (!this._completer.canMakeRequest(this.gethashUrl)) {
       log("Can't make request to " + this.gethashUrl + "\n");
       this.notifyFailure(Cr.NS_ERROR_ABORT);
-      return;
+      return false;
     }
 
     Services.obs.addObserver(this, "quit-application");
 
     // V4 requires table states to build the request so we need
     // a async call to retrieve the table states from disk.
     // Note that |HCR_begin| is fine to be sync because
     // it doesn't appear in a sync call chain.
     this.fillTableStatesBase64(() => {
       try {
         this.openChannel();
         // Notify the RequestBackoff if opening the channel succeeded. At this
         // point, finishRequest must be called.
         this._completer.noteRequest(this.gethashUrl);
       } catch (err) {
+        this._completer._ongoingRequests =
+          this._completer._ongoingRequests.filter(v => v != this);
         this.notifyFailure(err);
         throw err;
       }
     });
+
+    return true;
   },
 
   notify: function HCR_notify() {
     // If we haven't gotten onStopRequest, just cancel. This will call us
     // with onStopRequest since we implement nsIStreamListener on the
     // channel.
     if (this._channel && this._channel.isPending()) {
       log("cancelling request to " + this.gethashUrl + " (timeout)\n");
@@ -771,30 +804,30 @@ HashCompleterRequest.prototype = {
       let success = channel.requestSucceeded;
       httpStatus = channel.responseStatus;
       if (!success) {
         aStatusCode = Cr.NS_ERROR_ABORT;
       }
     }
     let success = Components.isSuccessCode(aStatusCode);
     log("Received a " + httpStatus + " status code from the " + this.provider +
-        " gethash server (success=" + success + ").");
+        " gethash server (success=" + success + "): " + btoa(this._response));
 
     Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_REMOTE_STATUS2").
       add(this.telemetryProvider, httpStatusToBucket(httpStatus));
     if (httpStatus == 400) {
       dump("Safe Browsing server returned a 400 during completion: request= " +
            this.request.url + ",payload= " + this.request.body + "\n");
     }
 
     Services.telemetry.getKeyedHistogramById("URLCLASSIFIER_COMPLETE_TIMEOUT2").
       add(this.telemetryProvider, 0);
 
     // Notify the RequestBackoff once a response is received.
-    this._completer.finishRequest(this.gethashUrl, httpStatus);
+    this._completer.finishRequest(this, httpStatus);
 
     if (success) {
       try {
         this.handleResponse();
       } catch (err) {
         log(err.stack);
         aStatusCode = err.value;
         success = false;