Bug 1258595 - Shut down the Push service if errors occur at startup. r=wchen
authorKit Cambridge <kcambridge@mozilla.com>
Mon, 21 Mar 2016 18:07:16 -0700
changeset 293105 915a3ac4e5639da7c02b92ef1457711301393417
parent 293104 3b5dbf1464c4d7b9e6028cf81b13c4d416269fa8
child 293106 1085ea32229e7ef6df1a36ef98272fd06d5a4925
push id30175
push usercbook@mozilla.com
push dateThu, 14 Apr 2016 09:38:40 +0000
treeherdermozilla-central@91115264629d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerswchen
bugs1258595
milestone48.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 1258595 - Shut down the Push service if errors occur at startup. r=wchen MozReview-Commit-ID: HMWMJ5qPGwY
dom/push/PushService.jsm
dom/push/test/xpcshell/test_startup_error.js
dom/push/test/xpcshell/xpcshell.ini
modules/libpref/init/all.js
--- a/dom/push/PushService.jsm
+++ b/dom/push/PushService.jsm
@@ -116,18 +116,26 @@ this.PushService = {
   // a new db is started. This events must be sequential.
   _stateChangeProcessQueue: null,
   _stateChangeProcessEnqueue: function(op) {
     if (!this._stateChangeProcessQueue) {
       this._stateChangeProcessQueue = Promise.resolve();
     }
 
     this._stateChangeProcessQueue = this._stateChangeProcessQueue
-                                    .then(op)
-                                    .catch(_ => {});
+      .then(op)
+      .catch(error => {
+        console.error(
+          "stateChangeProcessEnqueue: Error transitioning state", error);
+        return this._shutdownService();
+      })
+      .catch(error => {
+        console.error(
+          "stateChangeProcessEnqueue: Error shutting down service", error);
+      });
   },
 
   // Pending request. If a worker try to register for the same scope again, do
   // not send a new registration request. Therefore we need queue of pending
   // register requests. This is the list of scopes which pending registration.
   _pendingRegisterRequest: {},
   _notifyActivated: null,
   _activated: null,
@@ -189,58 +197,59 @@ this.PushService = {
   },
 
   _changeStateOfflineEvent: function(offline, calledFromConnEnabledEvent) {
     console.debug("changeStateOfflineEvent()", offline);
 
     if (this._state < PUSH_SERVICE_ACTIVE_OFFLINE &&
         this._state != PUSH_SERVICE_ACTIVATING &&
         !calledFromConnEnabledEvent) {
-      return;
+      return Promise.resolve();
     }
 
     if (offline) {
       if (this._state == PUSH_SERVICE_RUNNING) {
         this._service.disconnect();
       }
       this._setState(PUSH_SERVICE_ACTIVE_OFFLINE);
-    } else {
-      if (this._state == PUSH_SERVICE_RUNNING) {
-        // PushService was not in the offline state, but got notification to
-        // go online (a offline notification has not been sent).
-        // Disconnect first.
-        this._service.disconnect();
+      return Promise.resolve();
+    }
+
+    if (this._state == PUSH_SERVICE_RUNNING) {
+      // PushService was not in the offline state, but got notification to
+      // go online (a offline notification has not been sent).
+      // Disconnect first.
+      this._service.disconnect();
+    }
+    return this.getAllUnexpired().then(records => {
+      this._setState(PUSH_SERVICE_RUNNING);
+      if (records.length > 0) {
+        // if there are request waiting
+        this._service.connect(records);
       }
-      this._db.getAllUnexpired()
-        .then(records => {
-          if (records.length > 0) {
-            // if there are request waiting
-            this._service.connect(records);
-          }
-        });
-      this._setState(PUSH_SERVICE_RUNNING);
-    }
+    });
   },
 
   _changeStateConnectionEnabledEvent: function(enabled) {
     console.debug("changeStateConnectionEnabledEvent()", enabled);
 
     if (this._state < PUSH_SERVICE_CONNECTION_DISABLE &&
         this._state != PUSH_SERVICE_ACTIVATING) {
-      return;
+      return Promise.resolve();
     }
 
     if (enabled) {
-      this._changeStateOfflineEvent(Services.io.offline, true);
-    } else {
-      if (this._state == PUSH_SERVICE_RUNNING) {
-        this._service.disconnect();
-      }
-      this._setState(PUSH_SERVICE_CONNECTION_DISABLE);
+      return this._changeStateOfflineEvent(Services.io.offline, true);
     }
+
+    if (this._state == PUSH_SERVICE_RUNNING) {
+      this._service.disconnect();
+    }
+    this._setState(PUSH_SERVICE_CONNECTION_DISABLE);
+    return Promise.resolve();
   },
 
   // Used for testing.
   changeTestServer(url, options = {}) {
     console.debug("changeTestServer()");
 
     this._stateChangeProcessEnqueue(_ => {
       if (this._state < PUSH_SERVICE_ACTIVATING) {
@@ -398,17 +407,17 @@ this.PushService = {
 
       case STARTING_SERVICE_EVENT:
       {
         let [service, uri] = this._findService(serverURI);
         if (!service) {
           this._setState(PUSH_SERVICE_INIT);
           return Promise.resolve();
         }
-        return this._startService(service, uri)
+        return this._startService(service, uri, options)
           .then(_ => this._stateChangeProcessEnqueue(_ =>
             this._changeStateConnectionEnabledEvent(prefs.get("connection.enabled")))
           );
       }
       case CHANGING_SERVICE_EVENT:
         let [service, uri] = this._findService(serverURI);
         if (service) {
           if (this._state == PUSH_SERVICE_INIT) {
@@ -473,28 +482,18 @@ this.PushService = {
 
     this._setState(PUSH_SERVICE_ACTIVATING);
 
     Services.obs.addObserver(this, "quit-application", false);
 
     if (options.serverURI) {
       // this is use for xpcshell test.
 
-      let [service, uri] = this._findService(options.serverURI);
-      if (!service) {
-        this._setState(PUSH_SERVICE_INIT);
-        return;
-      }
-
-      // Start service.
-      this._startService(service, uri, options).then(_ => {
-        // Before completing the activation check prefs. This will first check
-        // connection.enabled pref and then check offline state.
-        this._changeStateConnectionEnabledEvent(prefs.get("connection.enabled"));
-      }).catch(Cu.reportError);
+      this._stateChangeProcessEnqueue(_ =>
+        this._changeServerURL(options.serverURI, STARTING_SERVICE_EVENT, options));
 
     } else {
       // This is only used for testing. Different tests require connecting to
       // slightly different URLs.
       prefs.observe("serverURL", this);
 
       this._stateChangeProcessEnqueue(_ =>
         this._changeServerURL(prefs.get("serverURL"), STARTING_SERVICE_EVENT));
@@ -537,17 +536,17 @@ this.PushService = {
     // Prunes expired registrations and notifies dormant service workers.
     Services.obs.addObserver(this, "idle-daily", false);
 
     // Prunes registrations for sites for which the user revokes push
     // permissions.
     Services.obs.addObserver(this, "perm-changed", false);
   },
 
-  _startService: function(service, serverURI, options = {}) {
+  _startService(service, serverURI, options) {
     console.debug("startService()");
 
     if (this._state != PUSH_SERVICE_ACTIVATING) {
       return Promise.reject();
     }
 
     this._service = service;
 
@@ -618,33 +617,34 @@ this.PushService = {
     prefs.ignore("connection.enabled", this);
 
     Services.obs.removeObserver(this, this._networkStateChangeEventName);
     Services.obs.removeObserver(this, "clear-origin-data");
     Services.obs.removeObserver(this, "idle-daily");
     Services.obs.removeObserver(this, "perm-changed");
   },
 
+  _shutdownService() {
+    let promiseChangeURL = this._changeServerURL("", UNINIT_EVENT);
+    this._setState(PUSH_SERVICE_UNINIT);
+    console.debug("shutdownService: shutdown complete!");
+    return promiseChangeURL;
+  },
+
   uninit: function() {
     console.debug("uninit()");
 
     if (this._state == PUSH_SERVICE_UNINIT) {
       return;
     }
 
     prefs.ignore("serverURL", this);
     Services.obs.removeObserver(this, "quit-application");
 
-    this._stateChangeProcessEnqueue(_ =>
-      {
-        var p = this._changeServerURL("", UNINIT_EVENT);
-        this._setState(PUSH_SERVICE_UNINIT);
-        console.debug("uninit: shutdown complete!");
-        return p;
-      });
+    this._stateChangeProcessEnqueue(_ => this._shutdownService());
   },
 
   /**
    * Drops all active registrations and notifies the associated service
    * workers. This function is called when the user switches Push servers,
    * or when the server invalidates all existing registrations.
    *
    * We ignore expired registrations because they're already handled in other
new file mode 100644
--- /dev/null
+++ b/dom/push/test/xpcshell/test_startup_error.js
@@ -0,0 +1,73 @@
+'use strict';
+
+const {PushService, PushServiceWebSocket} = serviceExports;
+
+function run_test() {
+  setPrefs();
+  do_get_profile();
+  run_next_test();
+}
+
+add_task(function* test_startup_error() {
+  let db = PushServiceWebSocket.newPushDB();
+  do_register_cleanup(() => {return db.drop().then(_ => db.close());});
+
+  PushService.init({
+    serverURI: 'wss://push.example.org/',
+    networkInfo: new MockDesktopNetworkInfo(),
+    db: makeStub(db, {
+      getAllExpired(prev) {
+        return Promise.reject('database corruption on startup');
+      },
+    }),
+    makeWebSocket(uri) {
+      return new MockWebSocket(uri, {
+        onHello(request) {
+          ok(false, 'Unexpected handshake');
+        },
+        onRegister(request) {
+          ok(false, 'Unexpected register request');
+        },
+      });
+    },
+  });
+
+  yield rejects(
+    PushService.register({
+      scope: `https://example.net/1`,
+      originAttributes: ChromeUtils.originAttributesToSuffix(
+        { appId: Ci.nsIScriptSecurityManager.NO_APP_ID, inIsolatedMozBrowser: false }),
+    }),
+    'Should not register if startup failed'
+  );
+
+  PushService.uninit();
+
+  PushService.init({
+    serverURI: 'wss://push.example.org/',
+    networkInfo: new MockDesktopNetworkInfo(),
+    db: makeStub(db, {
+      getAllUnexpired(prev) {
+        return Promise.reject('database corruption on connect');
+      },
+    }),
+    makeWebSocket(uri) {
+      return new MockWebSocket(uri, {
+        onHello(request) {
+          ok(false, 'Unexpected handshake');
+        },
+        onRegister(request) {
+          ok(false, 'Unexpected register request');
+        },
+      });
+    },
+  });
+  yield rejects(
+    PushService.registration({
+      scope: `https://example.net/1`,
+      originAttributes: ChromeUtils.originAttributesToSuffix(
+        { appId: Ci.nsIScriptSecurityManager.NO_APP_ID, inIsolatedMozBrowser: false }),
+    }),
+    'Should not return registration if connection failed'
+  );
+});
--- a/dom/push/test/xpcshell/xpcshell.ini
+++ b/dom/push/test/xpcshell/xpcshell.ini
@@ -43,16 +43,17 @@ run-sequentially = This will delete all 
 [test_unregister_invalid_json.js]
 [test_unregister_not_found.js]
 [test_unregister_success.js]
 [test_updateRecordNoEncryptionKeys_ws.js]
 [test_reconnect_retry.js]
 [test_retry_ws.js]
 [test_service_parent.js]
 [test_service_child.js]
+[test_startup_error.js]
 
 #http2 test
 [test_resubscribe_4xxCode_http2.js]
 [test_resubscribe_5xxCode_http2.js]
 [test_resubscribe_listening_for_msg_error_http2.js]
 [test_register_5xxCode_http2.js]
 [test_updateRecordNoEncryptionKeys_http2.js]
 [test_register_success_http2.js]
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@@ -4607,17 +4607,17 @@ pref("dom.sms.maxReadAheadEntries", 0);
 
 // WebAlarms
 pref("dom.mozAlarms.enabled", false);
 
 // Push
 
 pref("dom.push.enabled", false);
 
-pref("dom.push.loglevel", "off");
+pref("dom.push.loglevel", "error");
 
 pref("dom.push.serverURL", "wss://push.services.mozilla.com/");
 pref("dom.push.userAgentID", "");
 
 // The maximum number of push messages that a service worker can receive
 // without user interaction.
 pref("dom.push.maxQuotaPerSubscription", 16);