Bug 1524655 - Remove `dom.push.alwaysConnect` and connect unconditionally. r=jrconlin,pjenvey
☠☠ backed out by ab709310d23f ☠ ☠
authorLina Cambridge <lina@yakshaving.ninja>
Thu, 14 Mar 2019 04:22:18 +0000
changeset 521826 88ea72c345ab
parent 521825 63b6e32fe386
child 521827 082a7bbf945a
push id10867
push userdvarga@mozilla.com
push dateThu, 14 Mar 2019 15:20:45 +0000
treeherdermozilla-beta@abad13547875 [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 Also, remove 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/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 => {
@@ -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,17 +573,16 @@ 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() {
--- 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;
   },
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
@@ -5229,18 +5229,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,