Bug 1524655 - Remove dom.push.alwaysConnect and connect unconditionally. r=jrconlin,pjenvey

This commit also fixes a race in `test_error_reporting.html`, where the
push service would initialize and attach its listeners after
`sessionstore-windows-restored`. Even though the test replaces the real
service with a mock, the former keeps listening for pref changes.
When the test calls `setupPrefs` to enable the push connection, the
real service tries to connect to the push server, which asserts in
automation because non-local connections aren't allowed.

We work around this by ensuring that `replacePushService` and
`restorePushService` always wait for the service to shut down before
replacing it with a mock, or restoring the real implementation.

Finally, this commit removes a test that's no longer relevant, since
we don't need to fetch all subscriptions at startup.

Differential Revision: https://phabricator.services.mozilla.com/D20085

--HG--
extra : moz-landing-system : lando

dom/push/PushService.jsm

@@ -217,7 +217,6 @@ var PushService = {
       this._service.disconnect();
     }
 
-    let records = await this.getAllUnexpired();
     let broadcastListeners = await pushBroadcastService.getListeners();
 
     // In principle, a listener could be added to the
@@ -230,13 +229,7 @@ var PushService = {
     // getListeners.
     this._setState(PUSH_SERVICE_RUNNING);
 
-    if (records.length > 0 || prefs.get("alwaysConnect")) {
-      // Connect if we have existing subscriptions, or if the always-on pref
-      // is set. We gate on the pref to let us do load testing before
-      // turning it on for everyone, but if the user has push
-      // subscriptions, we need to connect them anyhow.
-      this._service.connect(records, broadcastListeners);
-    }
+    this._service.connect(broadcastListeners);
   },
 
   _changeStateConnectionEnabledEvent: function(enabled) {
@@ -297,7 +290,7 @@ var PushService = {
                                   CHANGING_SERVICE_EVENT)
           );
 
-        } else if (aData == "dom.push.connection.enabled" || aData == "dom.push.alwaysConnect") {
+        } else if (aData == "dom.push.connection.enabled") {
           this._stateChangeProcessEnqueue(_ =>
             this._changeStateConnectionEnabledEvent(prefs.get("connection.enabled"))
           );
@@ -460,7 +453,7 @@ var PushService = {
    *                                        PUSH_SERVICE_ACTIVE_OFFLINE or
    *                                        PUSH_SERVICE_CONNECTION_DISABLE.
    */
-  init: function(options = {}) {
+  async init(options = {}) {
     console.debug("init()");
 
     if (this._state > PUSH_SERVICE_UNINIT) {
@@ -475,13 +468,13 @@ var PushService = {
     if (options.serverURI) {
       // this is use for xpcshell test.
 
-      return this._stateChangeProcessEnqueue(_ =>
+      await 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.
-      return this._stateChangeProcessEnqueue(_ =>
+      await this._stateChangeProcessEnqueue(_ =>
         this._changeServerURL(prefs.get("serverURL"), STARTING_SERVICE_EVENT));
     }
   },
@@ -502,8 +495,6 @@ var PushService = {
 
     // Used to monitor if the user wishes to disable Push.
     prefs.observe("connection.enabled", this);
-    // Used to load-test the server-side infrastructure for broadcast.
-    prefs.observe("alwaysConnect", this);
 
     // Prunes expired registrations and notifies dormant service workers.
     Services.obs.addObserver(this, "idle-daily");
@@ -587,7 +578,6 @@ var PushService = {
     }
 
     prefs.ignore("connection.enabled", this);
-    prefs.ignore("alwaysConnect", this);
 
     Services.obs.removeObserver(this, "network:offline-status-changed");
     Services.obs.removeObserver(this, "clear-origin-attributes-data");
@@ -602,7 +592,7 @@ var PushService = {
     return promiseChangeURL;
   },
 
-  uninit: function() {
+  async uninit() {
     console.debug("uninit()");
 
     if (this._state == PUSH_SERVICE_UNINIT) {
@@ -612,7 +602,7 @@ var PushService = {
     prefs.ignore("serverURL", this);
     Services.obs.removeObserver(this, "quit-application");
 
-    this._stateChangeProcessEnqueue(_ => this._shutdownService());
+    await this._stateChangeProcessEnqueue(_ => this._shutdownService());
   },
 
   /**

dom/push/PushServiceHttp2.jsm

@@ -425,7 +425,8 @@ var PushServiceHttp2 = {
     return serverURI.scheme == "https";
   },
 
-  connect: function(subscriptions, broadcastListeners) {
+  async connect(broadcastListeners) {
+    let subscriptions = await this._mainPushService.getAllUnexpired();
     this.startConnections(subscriptions);
   },
 

dom/push/PushServiceWebSocket.jsm

@@ -512,7 +512,7 @@ var PushServiceWebSocket = {
     }
   },
 
-  connect: function(records, broadcastListeners) {
+  connect: function(broadcastListeners) {
     console.debug("connect()", broadcastListeners);
     this._broadcastListeners = broadcastListeners;
     this._beginWSSetup();

dom/push/test/mockpushserviceparent.js

@@ -146,14 +146,32 @@ var MockService = {
       reason: reason,
     });
   },
+
+  uninit() {
+    return Promise.resolve();
+  },
 };
 
-addMessageListener("service-replace", function () {
-  pushService.service = MockService;
+async function replaceService(service) {
+  await pushService.service.uninit();
+  pushService.service = service;
+  await pushService.service.init();
+}
+
+addMessageListener("service-replace", function() {
+  replaceService(MockService).then(_ => {
+    sendAsyncMessage("service-replaced");
+  }).catch(error => {
+    Cu.reportError(`Error replacing service: ${error}`);
+  });
 });
 
-addMessageListener("service-restore", function () {
-  pushService.service = null;
+addMessageListener("service-restore", function() {
+  replaceService(null).then(_ => {
+    sendAsyncMessage("service-restored");
+  }).catch(error => {
+    Cu.reportError(`Error restoring service: ${error}`);
+  });
 });
 
 addMessageListener("service-response", function (response) {

dom/push/test/test_utils.js

@@ -9,8 +9,7 @@
    * from the DOM API. This allows tests to simulate local errors and error
    * reporting, bypassing the `PushService.jsm` machinery.
    */
-  function replacePushService(mockService) {
-    chromeScript.sendSyncMessage("service-replace");
+  async function replacePushService(mockService) {
     chromeScript.addMessageListener("service-delivery-error", function(msg) {
       mockService.reportDeliveryError(msg.messageId, msg.reason);
     });
@@ -34,10 +33,23 @@
         });
       });
     });
+    await new Promise(resolve => {
+      chromeScript.addMessageListener("service-replaced", function onReplaced() {
+        chromeScript.removeMessageListener("service-replaced", onReplaced);
+        resolve();
+      });
+      chromeScript.sendAsyncMessage("service-replace");
+    });
   }
 
-  function restorePushService() {
-    chromeScript.sendSyncMessage("service-restore");
+  async function restorePushService() {
+    await new Promise(resolve => {
+      chromeScript.addMessageListener("service-restored", function onRestored() {
+        chromeScript.removeMessageListener("service-restored", onRestored);
+        resolve();
+      });
+      chromeScript.sendAsyncMessage("service-restore");
+    });
   }
 
   let userAgentID = "8e1c93a9-139b-419c-b200-e715bb1e8ce8";
@@ -153,13 +165,11 @@
 }(this));
 
 // Remove permissions and prefs when the test finishes.
-SimpleTest.registerCleanupFunction(() => {
-  return new Promise(resolve =>
-    SpecialPowers.flushPermissions(resolve)
-  ).then(_ => SpecialPowers.flushPrefEnv()).then(_ => {
-    restorePushService();
-    return teardownMockPushSocket();
-  });
+SimpleTest.registerCleanupFunction(async function() {
+  await new Promise(resolve => SpecialPowers.flushPermissions(resolve));
+  await SpecialPowers.flushPrefEnv();
+  await restorePushService();
+  await teardownMockPushSocket();
 });
 
 function setPushPermission(allow) {
@@ -181,9 +191,9 @@ function setupPrefs() {
     ]});
 }
 
-function setupPrefsAndReplaceService(mockService) {
-  replacePushService(mockService);
-  return setupPrefs();
+async function setupPrefsAndReplaceService(mockService) {
+  await replacePushService(mockService);
+  await setupPrefs();
 }
 
 function setupPrefsAndMockSocket(mockSocket) {

dom/push/test/xpcshell/test_startup_error.js

@@ -1,73 +0,0 @@
-'use strict';
-
-const {PushService, PushServiceWebSocket} = serviceExports;
-
-function run_test() {
-  setPrefs();
-  do_get_profile();
-  run_next_test();
-}
-
-add_task(async function test_startup_error() {
-  let db = PushServiceWebSocket.newPushDB();
-  registerCleanupFunction(() => {return db.drop().then(_ => db.close());});
-
-  PushService.init({
-    serverURI: 'wss://push.example.org/',
-    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');
-        },
-      });
-    },
-  });
-
-  await rejects(
-    PushService.register({
-      scope: `https://example.net/1`,
-      originAttributes: ChromeUtils.originAttributesToSuffix(
-        { appId: Ci.nsIScriptSecurityManager.NO_APP_ID, inIsolatedMozBrowser: false }),
-    }),
-    /Push service not active/,
-    'Should not register if startup failed'
-  );
-
-  PushService.uninit();
-
-  PushService.init({
-    serverURI: 'wss://push.example.org/',
-    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');
-        },
-      });
-    },
-  });
-  await rejects(
-    PushService.registration({
-      scope: `https://example.net/1`,
-      originAttributes: ChromeUtils.originAttributesToSuffix(
-        { appId: Ci.nsIScriptSecurityManager.NO_APP_ID, inIsolatedMozBrowser: false }),
-    }),
-    /Push service not active/,
-    'Should not return registration if connection failed'
-  );
-});

dom/push/test/xpcshell/xpcshell.ini

@@ -56,7 +56,6 @@ skip-if = os == "linux" # Bug 1265233
 [test_retry_ws.js]
 [test_service_parent.js]
 [test_service_child.js]
-[test_startup_error.js]
 
 #http2 test
 [test_resubscribe_4xxCode_http2.js]

modules/libpref/init/all.js

@@ -5262,8 +5262,6 @@ pref("dom.battery.enabled", true);
 
 // Push
 
-pref("dom.push.alwaysConnect", false);
-
 pref("dom.push.loglevel", "Error");
 
 pref("dom.push.serverURL", "wss://push.services.mozilla.com/");

toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain.js

@@ -383,6 +383,13 @@ async function test_push_cleared() {
               uaid: userAgentID,
             }));
           },
+          onUnregister(request) {
+            this.serverSendMsg(JSON.stringify({
+              messageType: "unregister",
+              status: 200,
+              channelID: request.channelID,
+            }));
+          },
         });
       },
     });