Bug 1311739 - `PushDB.update` should reject instead of returning `undefined` for invalid records. r=dragana
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 20 Oct 2016 08:45:44 -0700
changeset 319177 c0c3b590372cef550c27b4ac24867bd491134e09
parent 319176 357a4a25901a9269c90a1dd09010c16d50ad8c72
child 319178 8fd3c7221c70211c5892f7cc1701f6bc7b0cc45e
push id30863
push usercbook@mozilla.com
push dateTue, 25 Oct 2016 08:26:35 +0000
treeherdermozilla-central@9fb2ac0875ec [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdragana
bugs1311739
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1311739 - `PushDB.update` should reject instead of returning `undefined` for invalid records. r=dragana MozReview-Commit-ID: HCLOSz4FHWO
dom/push/PushDB.jsm
dom/push/PushService.jsm
dom/push/test/xpcshell/test_registration_success_http2.js
dom/push/test/xpcshell/test_unregister_success_http2.js
--- a/dom/push/PushDB.jsm
+++ b/dom/push/PushDB.jsm
@@ -371,41 +371,39 @@ this.PushDB.prototype = {
     return this._getAllByPushQuota(IDBKeyRange.only(0));
   },
 
   /**
    * Updates an existing push registration.
    *
    * @param {String} aKeyID The registration ID.
    * @param {Function} aUpdateFunc A function that receives the existing
-   *  registration record as its argument, and returns a new record. If the
-   *  function returns `null` or `undefined`, the record will not be updated.
-   *  If the record does not exist, the function will not be called.
-   * @returns {Promise} A promise resolved with either the updated record, or
-   *  `undefined` if the record was not updated.
+   *  registration record as its argument, and returns a new record.
+   * @returns {Promise} A promise resolved with either the updated record.
+   *  Rejects if the record does not exist, or the function returns an invalid
+   *  record.
    */
   update: function(aKeyID, aUpdateFunc) {
     return new Promise((resolve, reject) =>
       this.newTxn(
         "readwrite",
         this._dbStoreName,
         (aTxn, aStore) => {
           aStore.get(aKeyID).onsuccess = aEvent => {
             aTxn.result = undefined;
 
             let record = aEvent.target.result;
             if (!record) {
-              console.error("update: Record does not exist", aKeyID);
-              return;
+              throw new Error("Record " + aKeyID + " does not exist");
             }
             let newRecord = aUpdateFunc(this.toPushRecord(record));
             if (!this.isValidRecord(newRecord)) {
               console.error("update: Ignoring invalid update",
                 aKeyID, newRecord);
-              return;
+              throw new Error("Invalid update for record " + aKeyID);
             }
             function putRecord() {
               let req = aStore.put(newRecord);
               req.onsuccess = aEvent => {
                 console.debug("update: Update successful", aKeyID, newRecord);
                 aTxn.result = newRecord;
               };
             }
--- a/dom/push/PushService.jsm
+++ b/dom/push/PushService.jsm
@@ -824,19 +824,16 @@ this.PushService = {
           if (newRecord.isExpired()) {
             return null;
           }
           newRecord.receivedPush(lastVisit);
           return newRecord;
         });
       });
     }).then(record => {
-      if (!record) {
-        throw new Error("Ignoring update for key ID " + keyID);
-      }
       gPushNotifier.notifySubscriptionModified(record.scope,
                                                record.principal);
       return record;
     });
   },
 
   /**
    * Decrypts an incoming message and notifies the associated service worker.
@@ -875,28 +872,26 @@ this.PushService = {
       }
       // If there are visible notifications, don't apply the quota penalty
       // for the message.
       if (record.uri && !this._visibleNotifications.has(record.uri.prePath)) {
         record.reduceQuota();
       }
       return record;
     }).then(record => {
-      if (record) {
-        if (record.isExpired()) {
-          this._recordDidNotNotify(kDROP_NOTIFICATION_REASON_EXPIRED);
-          // Drop the registration in the background. If the user returns to the
-          // site, the service worker will be notified on the next `idle-daily`
-          // event.
-          this._backgroundUnregister(record,
-            Ci.nsIPushErrorReporter.UNSUBSCRIBE_QUOTA_EXCEEDED);
-        } else {
-          gPushNotifier.notifySubscriptionModified(record.scope,
-                                                   record.principal);
-        }
+      if (record.isExpired()) {
+        this._recordDidNotNotify(kDROP_NOTIFICATION_REASON_EXPIRED);
+        // Drop the registration in the background. If the user returns to the
+        // site, the service worker will be notified on the next `idle-daily`
+        // event.
+        this._backgroundUnregister(record,
+          Ci.nsIPushErrorReporter.UNSUBSCRIBE_QUOTA_EXCEEDED);
+      } else {
+        gPushNotifier.notifySubscriptionModified(record.scope,
+                                                 record.principal);
       }
       if (this._updateQuotaTestCallback) {
         // Callback so that test may be notified when the quota update is complete.
         this._updateQuotaTestCallback();
       }
     }).catch(error => {
       console.debug("updateQuota: Error while trying to update quota", error);
     });
--- a/dom/push/test/xpcshell/test_registration_success_http2.js
+++ b/dom/push/test/xpcshell/test_registration_success_http2.js
@@ -1,23 +1,14 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 'use strict';
 
 Cu.import("resource://gre/modules/Services.jsm");
-Cu.import("resource://testing-common/PromiseTestUtils.jsm");
-
-///////////////////
-//
-// Whitelisting this test.
-// As part of bug 1077403, the leaking uncaught rejection should be fixed.
-//
-// Instances of the rejection "record is undefined" may or may not appear.
-PromiseTestUtils.thisTestLeaksUncaughtRejectionsAndShouldBeFixed();
 
 const {PushDB, PushService, PushServiceHttp2} = serviceExports;
 
 var prefs;
 
 var serverPort = -1;
 
 function run_test() {
--- a/dom/push/test/xpcshell/test_unregister_success_http2.js
+++ b/dom/push/test/xpcshell/test_unregister_success_http2.js
@@ -1,23 +1,14 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 'use strict';
 
 Cu.import("resource://gre/modules/Services.jsm");
-Cu.import("resource://testing-common/PromiseTestUtils.jsm");
-
-///////////////////
-//
-// Whitelisting this test.
-// As part of bug 1077403, the leaking uncaught rejection should be fixed.
-//
-// Instances of the rejection "record is undefined" may or may not appear.
-PromiseTestUtils.thisTestLeaksUncaughtRejectionsAndShouldBeFixed();
 
 const {PushDB, PushService, PushServiceHttp2} = serviceExports;
 
 var prefs;
 var tlsProfile;
 var pushEnabled;
 var pushConnectionEnabled;