Bug 1524655 - Remove `dom.push.alwaysConnect` and connect unconditionally. r=jrconlin,pjenvey
authorLina Cambridge <lina@yakshaving.ninja>
Thu, 14 Mar 2019 22:37:51 +0000
changeset 521988 f32dc8409c3de7b2f7181f034df2176219a728ad
parent 521987 f0c35396d81ff7b9ae11bbe23bd424fcf5c3be8d
child 521989 57a544b33b511811350a56cd2f4206c2a4dc155a
push id10870
push usernbeleuzu@mozilla.com
push dateFri, 15 Mar 2019 20:00:07 +0000
treeherdermozilla-beta@c594aee5b7a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrconlin, pjenvey
bugs1524655
milestone67.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 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
dom/push/PushService.jsm
dom/push/PushServiceHttp2.jsm
dom/push/PushServiceWebSocket.jsm
dom/push/test/mockpushserviceparent.js
dom/push/test/test_utils.js
dom/push/test/xpcshell/test_startup_error.js
dom/push/test/xpcshell/xpcshell.ini
modules/libpref/init/all.js
toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain.js
--- a/dom/push/PushService.jsm
+++ b/dom/push/PushService.jsm
@@ -212,36 +212,29 @@ var PushService = {
 
     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();
     }
 
-    let records = await this.getAllUnexpired();
     let broadcastListeners = await pushBroadcastService.getListeners();
 
     // In principle, a listener could be added to the
     // pushBroadcastService here, after we have gotten listeners and
     // before we're RUNNING, but this can't happen in practice because
     // the only caller that can add listeners is PushBroadcastService,
     // and it waits on the same promise we are before it can add
     // listeners. If PushBroadcastService gets woken first, it will
     // update the value that is eventually returned from
     // 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) {
     console.debug("changeStateConnectionEnabledEvent()", enabled);
 
     if (this._state < PUSH_SERVICE_CONNECTION_DISABLE &&
         this._state != PUSH_SERVICE_ACTIVATING) {
       return Promise.resolve();
@@ -292,17 +285,17 @@ var PushService = {
         if (aData == "dom.push.serverURL") {
           console.debug("observe: dom.push.serverURL changed for websocket",
                 prefs.get("serverURL"));
           this._stateChangeProcessEnqueue(_ =>
             this._changeServerURL(prefs.get("serverURL"),
                                   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"))
           );
         }
         break;
 
       case "idle-daily":
         this._dropExpiredRegistrations().catch(error => {
@@ -455,38 +448,38 @@ var PushService = {
    *                  state is change to PUSH_SERVICE_ACTIVATING.
    * startObservers() - start other observers.
    * changeStateConnectionEnabledEvent  - checks prefs and offline state.
    *                                      It changes state to:
    *                                        PUSH_SERVICE_RUNNING,
    *                                        PUSH_SERVICE_ACTIVE_OFFLINE or
    *                                        PUSH_SERVICE_CONNECTION_DISABLE.
    */
-  init: function(options = {}) {
+  async init(options = {}) {
     console.debug("init()");
 
     if (this._state > PUSH_SERVICE_UNINIT) {
       return;
     }
 
     this._setState(PUSH_SERVICE_ACTIVATING);
 
     prefs.observe("serverURL", this);
     Services.obs.addObserver(this, "quit-application");
 
     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));
     }
   },
 
   _startObservers: function() {
     console.debug("startObservers()");
 
     if (this._state != PUSH_SERVICE_ACTIVATING) {
@@ -497,18 +490,16 @@ var PushService = {
 
     // The offline-status-changed event is used to know
     // when to (dis)connect. It may not fire if the underlying OS changes
     // networks; in such a case we rely on timeout.
     Services.obs.addObserver(this, "network:offline-status-changed");
 
     // 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");
 
     // Prunes registrations for sites for which the user revokes push
     // permissions.
     Services.obs.addObserver(this, "perm-changed");
   },
@@ -582,42 +573,41 @@ var PushService = {
   _stopObservers: function() {
     console.debug("stopObservers()");
 
     if (this._state < PUSH_SERVICE_ACTIVATING) {
       return;
     }
 
     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");
     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() {
+  async uninit() {
     console.debug("uninit()");
 
     if (this._state == PUSH_SERVICE_UNINIT) {
       return;
     }
 
     prefs.ignore("serverURL", this);
     Services.obs.removeObserver(this, "quit-application");
 
-    this._stateChangeProcessEnqueue(_ => this._shutdownService());
+    await 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
--- a/dom/push/PushServiceHttp2.jsm
+++ b/dom/push/PushServiceHttp2.jsm
@@ -420,17 +420,18 @@ var PushServiceHttp2 = {
 
   validServerURI: function(serverURI) {
     if (serverURI.scheme == "http") {
       return !!prefs.getBoolPref("testing.allowInsecureServerURL", false);
     }
     return serverURI.scheme == "https";
   },
 
-  connect: function(subscriptions, broadcastListeners) {
+  async connect(broadcastListeners) {
+    let subscriptions = await this._mainPushService.getAllUnexpired();
     this.startConnections(subscriptions);
   },
 
   sendSubscribeBroadcast: async function(serviceId, version) {
     // Not implemented yet
   },
 
   isConnected: function() {
--- a/dom/push/PushServiceWebSocket.jsm
+++ b/dom/push/PushServiceWebSocket.jsm
@@ -507,17 +507,17 @@ var PushServiceWebSocket = {
       this._currentState = STATE_WAITING_FOR_WS_START;
     } catch(e) {
       console.error("beginWSSetup: Error opening websocket.",
         "asyncOpen failed", e);
       this._reconnect();
     }
   },
 
-  connect: function(records, broadcastListeners) {
+  connect: function(broadcastListeners) {
     console.debug("connect()", broadcastListeners);
     this._broadcastListeners = broadcastListeners;
     this._beginWSSetup();
   },
 
   isConnected: function() {
     return !!this._ws;
   },
--- a/dom/push/test/mockpushserviceparent.js
+++ b/dom/push/test/mockpushserviceparent.js
@@ -141,21 +141,39 @@ var MockService = {
   },
 
   reportDeliveryError(messageId, reason) {
     sendAsyncMessage("service-delivery-error", {
       messageId: messageId,
       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) {
   MockService.handleResponse(response);
 });
--- a/dom/push/test/test_utils.js
+++ b/dom/push/test/test_utils.js
@@ -4,18 +4,17 @@
   let url = SimpleTest.getTestFileURL("mockpushserviceparent.js");
   let chromeScript = SpecialPowers.loadChromeScript(url);
 
   /**
    * Replaces `PushService.jsm` with a mock implementation that handles requests
    * 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);
     });
     chromeScript.addMessageListener("service-request", function(msg) {
       let promise;
       try {
         let handler = mockService[msg.name];
         promise = Promise.resolve(handler(msg.params));
@@ -29,20 +28,33 @@
         });
       }, error => {
         chromeScript.sendAsyncMessage("service-response", {
           id: msg.id,
           error: error,
         });
       });
     });
+    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";
 
   let currentMockSocket = null;
 
   /**
    * Sets up a mock connection for the WebSocket backend. This only replaces
@@ -148,23 +160,21 @@
   g.MockWebSocket = MockWebSocket;
   g.setupMockPushSocket = setupMockPushSocket;
   g.teardownMockPushSocket = teardownMockPushSocket;
   g.replacePushService = replacePushService;
   g.restorePushService = restorePushService;
 }(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) {
   return new Promise(resolve => {
     SpecialPowers.pushPermissions([
       { type: "desktop-notification", allow, context: document },
       ], resolve);
   });
@@ -176,19 +186,19 @@ function setupPrefs() {
     ["dom.push.connection.enabled", true],
     ["dom.push.maxRecentMessageIDsPerSubscription", 0],
     ["dom.serviceWorkers.exemptFromPerDomainMax", true],
     ["dom.serviceWorkers.enabled", true],
     ["dom.serviceWorkers.testing.enabled", true]
     ]});
 }
 
-function setupPrefsAndReplaceService(mockService) {
-  replacePushService(mockService);
-  return setupPrefs();
+async function setupPrefsAndReplaceService(mockService) {
+  await replacePushService(mockService);
+  await setupPrefs();
 }
 
 function setupPrefsAndMockSocket(mockSocket) {
   setupMockPushSocket(mockSocket);
   return setupPrefs();
 }
 
 function injectControlledFrame(target = document.body) {
deleted file mode 100644
--- a/dom/push/test/xpcshell/test_startup_error.js
+++ /dev/null
@@ -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'
-  );
-});
--- a/dom/push/test/xpcshell/xpcshell.ini
+++ b/dom/push/test/xpcshell/xpcshell.ini
@@ -51,17 +51,16 @@ run-sequentially = This will delete all 
 [test_unregister_not_found.js]
 [test_unregister_success.js]
 [test_updateRecordNoEncryptionKeys_ws.js]
 skip-if = os == "linux" # Bug 1265233
 [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
@@ -5257,18 +5257,16 @@ pref("dom.vibrator.enabled", true);
 pref("dom.vibrator.max_vibrate_ms", 10000);
 pref("dom.vibrator.max_vibrate_list_len", 128);
 
 // Battery API
 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/");
 pref("dom.push.userAgentID", "");
 
 // The maximum number of push messages that a service worker can receive
 // without user interaction.
 pref("dom.push.maxQuotaPerSubscription", 16);
--- a/toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain.js
+++ b/toolkit/forgetaboutsite/test/unit/test_removeDataFromDomain.js
@@ -378,16 +378,23 @@ async function test_push_cleared() {
         return new MockWebSocket(uriObj, {
           onHello(request) {
             this.serverSendMsg(JSON.stringify({
               messageType: "hello",
               status: 200,
               uaid: userAgentID,
             }));
           },
+          onUnregister(request) {
+            this.serverSendMsg(JSON.stringify({
+              messageType: "unregister",
+              status: 200,
+              channelID: request.channelID,
+            }));
+          },
         });
       },
     });
 
     const TEST_URL = "https://www.mozilla.org/scope/";
     Assert.equal(false, await push_registration_exists(TEST_URL, ps));
     await db.put({
       channelID,