Bug 1311739 - `PushDB.update` should reject instead of returning `undefined` for invalid records. r?dragana draft
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 20 Oct 2016 08:45:44 -0700
changeset 427602 268cf42489c7e6c39da890f34b9b5d9ebba4c8a5
parent 427601 fbde8443ce27b7154c8ae1534348106e71ace63c
child 427603 e022506bf1c356e59a6c143d425962cb350c4def
push id33063
push userbmo:kcambridge@mozilla.com
push dateThu, 20 Oct 2016 16:29:04 +0000
reviewersdragana
bugs1311739
milestone52.0a1
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;