1) Make push client implementation talk http2. draft version https://martinthomson.github.io/drafts/draft-thomson-webpush-http2.html 2) Encrypt data transferred between app server and the client. starting version: https://tools.ietf.org/html/draft-thomson-webpush-encryption-00

Dragana, sorry for the delays, but this is a large patch. I'll review it tomorrow.

Comment on attachment 8596666 [details] [diff] [review] part1_bug_1150812_use_h2_v1.patch Review of attachment 8596666 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/push/test/test_disconnect_http2.html @@ +28,5 @@ > + function debug(str) { > + console.log(str + "\n"); > + } > + > + function addCertOverride(host, port, bits) { This is duplicated across multiple files. It seems like a generally useful function to move to a common file. ::: dom/push/test/test_switch_ws_http2.html @@ +5,5 @@ > + > +Any copyright is dedicated to the Public Domain. > +http://creativecommons.org/licenses/publicdomain/ > + > +TODO MOZ_DISABLE_NONLOCAL_CONNECTIONS ? @@ +38,5 @@ > + if (port) { > + url = "https://" + host + ":" + port + "/"; > + } else { > + url = "https://" + host + "/"; > + } url = "https://" + host + (port ? (":" + port) : "") + "/"; @@ +61,5 @@ > + serverCert; > + var cos = SpecialPowers.Cc["@mozilla.org/security/certoverride;1"]. > + getService(SpecialPowers.Ci.nsICertOverrideService); > + cos.rememberValidityOverride(host, port, cert, bits, true); > + space @@ +66,5 @@ > + res(null); > + } > + }); > + > + // In case the certificats are accepted. typo: certificats @@ +103,5 @@ > + > + function start() { > + debug("start"); > + return navigator.serviceWorker.register("worker.js" + "?" + (Math.random()), {scope: "."}) > + .then((swr) => registration = swr); The arrow form doesn't need parentheses. .then(swr => registration = swr); @@ +115,5 @@ > + } > + > + function setupPushNotification(swr) { > + debug("setupPushNotification"); > + var p = new Promise(function(res, rej) { Easier here to avoid the wrapping and simply use: return swr.pushManager.subscribe() .then(sub => { ok(true, "successfully registered for push"); // why do we have to return swr here? return { swr: swr, pushSubscription: pushSubscription }; }, err => { ok(false, "failed to register for push"); throw err; }); In general, I would let failures propagate down the promise chain so that you avoid running subsequent steps. @@ +147,5 @@ > + > + function checkHasSubscription1(result) { > + debug("checkHasSubscription1"); > + var reg = result.swr; > + var p = new Promise(function(res, rej) { Again, I would just use a simplified form: return swr.pushManager.subscription .then(sub => ok(sub, "subscription is not null"), err => { ok(false, "error getting subscription"); throw err; }); @@ +199,5 @@ > + 8080, > + SpecialPowers.Ci.nsICertOverrideService.ERROR_UNTRUSTED | > + SpecialPowers.Ci.nsICertOverrideService.ERROR_MISMATCH | > + SpecialPowers.Ci.nsICertOverrideService.ERROR_TIME) > + .then(start) Since start() saves the service worker registration on a global, maybe you could rely on that, rather than passing an swr parameter down the chain. The other way is this: addCertOverride(...) .then(_ => registerServiceWorker()) // i.e., start .then(swr => createControlledIFrame() .then(_ => subscribe(swr)) // i.e., setupPushNotification .then(sub => checkSubscription(swr, sub)) .then(_ => changePushServer(swr, defaultServer)) .then(_ => checkNoSubscriptions(swr)) .then(_ => subscribe(swr)) .then(...) ) .catch(e => ok(false, "error in test execution: " + e)) .then(_ => SimpleTest.finish());

Comment on attachment 8596665 [details] [diff] [review] part0_bug_1150812_use_h2_v1.patch Review of attachment 8596665 [details] [diff] [review]: ----------------------------------------------------------------- I think that this could use the arrow form of functions and avoid a lot of calls to .bind(). I'm concerned about the amount of duplication between PushServiceHTTP2 and PushService. There seems to be a lot of messages that they both listen to. This could be a lot tighter if the push service instantiated the right instance of the service based on the URL that it receives. This wouldn't be a big concern if the websockets version were discarded at some point in the near future. ::: dom/push/PushServiceHttp2.jsm @@ +77,5 @@ > + * Callback function to invoke with result ID. > + * @param aErrorCb [optional] > + * Callback function to invoke when there was an error. > + */ > + put: function(aRecord, aSuccessCb, aErrorCb) { All these functions could be trivially fixed to return a promise, which would make use of them much easier. @@ +470,5 @@ > + > + let deferred = Promise.defer(); > + > + var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); Rather than use XHR, perhaps this should use channels directly (at least that is what :sicking suggested I do here: https://dxr.mozilla.org/mozilla-central/source/dom/media/IdpSandbox.jsm#44). @@ +472,5 @@ > + > + var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + if (this._serverURL[this._serverURL.length - 1] == '/') { > + req.open("POST", this._serverURL + kPUSH_SUBSCRIPTION_PATH.substr(1), true); Please don't hard-code the /subscribe value. Just make it part of the configuration. That should simplify this. @@ +476,5 @@ > + req.open("POST", this._serverURL + kPUSH_SUBSCRIPTION_PATH.substr(1), true); > + } else { > + req.open("POST", this._serverURL + kPUSH_SUBSCRIPTION_PATH, true); > + } > + req.onload = this._subscribeOnLoad.bind(this, req, deferred); This is easier to read: req.onload = e => this._subscribeOnLoad(req, deferred, e); However, this is a better structure overall: var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] .createInstance(Ci.nsIXMLHttpRequest); req.open("POST", this._serverURL, true); return new Promise((resolve, reject) => { req.onload = resolve; req.onerror = reject; }).then(event => this._subscribeOnError(req, event)); Then _subscribeOnError can take the event and return a promise of its own if necessary. As it turns out, that's a synchronous function. (It can throw to report an error.) @@ +490,5 @@ > + debug("subscribeOnLoad response" + evt.target.status); > + > + if (evt.target.status != 201) { > + deferred.reject("Error status" + evt.target.status + ":" + > + evt.target.responseText); If you take my advice above, this can just throw. @@ +512,5 @@ > + if ((/ *rel="urn:ietf:params:push" */.test(linkElems[1])) && > + (/ *<.*> */.test(linkElems[0]))) { > + pushEndpoint = this._serverURL + > + linkElems[0].substring(linkElems[0].indexOf('<') + ((endWithbs) ? 2 : 1), > + linkElems[0].indexOf('>')); IMPORTANT: This isn't going to work for all servers and links. You need to resolve the URI relative to the request URI. I believe that passing the server URI as aBaseURI to newURI works here. It helps if you store the server URI as a nsIURI instance. See https://tools.ietf.org/html/rfc5988#section-5.1 @@ +530,5 @@ > + return; > + } > + > + try { > + Services.io.newURI(pushEndpoint, null, null); Checking these URIs should be done in a loop, not one by one. var uri; try { [a, b, c].forEach(u => { uri = u; Services.io.newURI(uri, null, this._serverURL)); }); } catch (e) { debug('invalid URI: ' + uri); throw new Error('Got 201, but URI is bogus: ' + uri); } @@ +570,5 @@ > + }, > + > + /** > + * Unsubscribe the resource with a subscription uri subscriptionUri. > + * We do not care the response. "do not care *about* the response" I'd imagine that you do care that this succeeds, but there isn't anything you can do about it if it fails. @@ +578,5 @@ > + > + var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + req.open("DELETE", subscriptionUri, true); > + req.send(null); Might pay to factor out the delete, since this duplicates the ACK below. @@ +582,5 @@ > + req.send(null); > + }, > + > + /** > + * Start listening for messiges. "messages" @@ +667,5 @@ > + .getService(Ci.nsIMessageBroadcaster); > + > + kCHILD_PROCESS_MESSAGES.forEach(function addMessage(msgName) { > + ppmm.addMessageListener(msgName, this); > + }.bind(this)); kCHILD_PROCESS_MESSAGES.forEach(msgName => ppmm.addMessageListener(msgName, this)); @@ +724,5 @@ > + > + _dropRegistration: function() { > + let deferred = Promise.defer(); > + this._db.drop(deferred.resolve, deferred.reject); > + return deferred.promise; I generally advise that Promise.defer() be avoided, since it's not on the public promise API: return new Promise((resolve, reject) => this._db.drop(resolve, reject)); @@ +797,5 @@ > + } > + deferred.resolve(); > + } > + > + this._db.getAllSubscriptionUris(cleanUp.bind(this), deferred.reject) return new Promise((resolve, reject) => this._db.getAllSubscriptionUris(resolve, reject)) .then(records => { let globalMM = Cc['@mozilla.org/globalmessagemanager;1'] .getService(Ci.nsIMessageListenerManager); records.forEach(record => { // ... }); }); @@ +814,5 @@ > + > + function notify(record) { > + let globalMM = Cc['@mozilla.org/globalmessagemanager;1'] > + .getService(Ci.nsIMessageListenerManager); > + globalMM.broadcastAsyncMessage('pushsubscriptionchange', resord.scope); IMPORTANT: typo here: "resord.scope" @@ +893,5 @@ > + let pageURI = Services.io.newURI(aPushRecord.pageURL, null, null); > + let scopeURI = Services.io.newURI(aPushRecord.scope, null, null); > + > + // If permission has been revoked, trash the message. > + if(Services.perms.testExactPermission(scopeURI, "push") != Ci.nsIPermissionManager.ALLOW_ACTION) { nit space: "if (Services..." @@ +1010,5 @@ > + deferred.resolve(message); > + }.bind(this), > + function(error) { > + // Unable to save. > + this._unsubscribeResource(data.subscriptioUri); IMPORTANT: typo here: data.subscriptioUri @@ +1065,5 @@ > + requestID: aPageRecord.requestID, > + pushEndpoint: aPageRecord.pushEndpoint > + }); > + }.bind(this), fail); > + }.bind(this), fail); With promises, this wouldn't be quite as complicated. @@ +1169,5 @@ > + > + /** |delay| should be in milliseconds. */ > + _setAlarm: function(delay) { > + // Bug 909270: Since calls to AlarmService.add() are async, calls must be > + // 'queued' to ensure only one alarm is ever active. A better way to deal with this is: . setAlarm resolves a promise when it is registered . setAlarm saves that promise each time that it is run . setAlarm awaits that promise before doing its business i.e. _alarmPromise: Promise.resolve(), _setAlarm: function(delay) { var time = Date.now() + delay); this._alarmPromise = this.alarmPromise .then(_ => this._reallySetAlarm(time)); }, _reallySetAlarm; function(time) { if (time < Date.now()) { // hmm, missed the alarm, maybe we don't care about this } } The only drawback here is that using the promise attempts to set the alarm are always enqueued, whereas the current code effectively cancels any attempts to set the alarm.

I'm going to suggest that you split the encryption part of this off. That should let this land without getting into the details.

Comment on attachment 8596665 [details] [diff] [review] part0_bug_1150812_use_h2_v1.patch Review of attachment 8596665 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to take a look again once the comments (Martin's too) are addressed. Thanks! ::: dom/push/PushService.jsm @@ +290,5 @@ > + > + if (!this._started && > + (aTopic != "xpcom-shutdown") && > + (aTopic != "nsPref:changed") && > + (aData != "dom.push.serverURL")) { Since we have a check for aData in the nsPref:changed case below, please remove it from here. It is hard to reason about over here. @@ +327,5 @@ > + var serverURL = prefs.get("serverURL"); > + debug("dom.push.serverURL changed! new value " + serverURL); > + > + this._activate_deactivate_queue.push([serverURL, true]); > + if (this._activate_deactivate_queue.length == 1) { unconditionally call asyncTryProcessQueue (see below) @@ +414,5 @@ > break; > } > }, > > + _activate_deactivate_pop() { rename this to process_queue @@ +421,5 @@ > + if (this._activate_deactivate_queue.length == 0) { > + return; > + } > + > + let [serverURL, cleanup] = this._activate_deactivate_queue[0]; let [...] = this._activate_deactivate_queue.shift() and you can get rid of the shifts below. @@ +447,5 @@ > + if (this._serverURL) { > + if (!this._started) { > + this._activate(); > + this._activate_deactivate_queue.shift(); > + setTimeout(this._activate_deactivate_pop.bind(this), 0); wrap this in a function called asyncTryProcessQueue (which also checks that length is >= 1 before queuing) and move this call right to the top of this function (right after the shift()) so that we never forget to call it. Presumably you also want to call it in the this._serverURL == 'something falsy' case below. @@ +691,5 @@ > + .then(function() > + { > + this._db.close(); > + this._db = null; > + this._activate_deactivate_queue.shift(); I don't understand why this is being shifted here. ::: dom/push/PushServiceHttp2.jsm @@ +52,5 @@ > + this.initDBHelper(kPUSHHTTP2DB_DB_NAME, kPUSHHTTP2DB_DB_VERSION, > + [kPUSHHTTP2DB_STORE_NAME]); > +}; > + > +this.PushHTTP2DB.prototype = { It seems like the only difference between PushDB here and in PushService.jsm is the one subscriptionUri field. If you could collapse both DB implementations into one file, and refactor it to somehow allow specifying which db name to use and the key name (easiest to pass in a function that is called by upgradeSchema which creates the correct objectStore) I'll give you bonus points :) @@ +77,5 @@ > + * Callback function to invoke with result ID. > + * @param aErrorCb [optional] > + * Callback function to invoke when there was an error. > + */ > + put: function(aRecord, aSuccessCb, aErrorCb) { Yes please to converting to Promises. Happy to do that part for you if you'd like. @@ +230,5 @@ > + if (aIID.equals(Ci.nsIHttpPushListener) || > + aIID.equals(Ci.nsIStreamListener)) > + return this; > + throw Components.results.NS_ERROR_NO_INTERFACE; > + }, Any reason QI and getInterface is defined? Usually JS components do not require it as long as they satisfy the contract. @@ +264,5 @@ > + } > +} > + > +/** > + * The lisener for pushed messages. The message data is collecetd in type: listener and collected @@ +265,5 @@ > +} > + > +/** > + * The lisener for pushed messages. The message data is collecetd in > + * OnDataAvailable and send to app in OnStopRequest. sent to the app @@ +278,5 @@ > + _mainListener: null, > + _message: "", > + _ackUri: null, > + > + QueryInterface: function (aIID) { Same as above. @@ +326,5 @@ > + */ > +this.PushServiceHttp2 = { > + > + // Depending on the configuration we can have websocket or http2 push user > + // agent. If _serverURL is set, this push user agent is active. Update comment based on enabling only one of the services at a time. @@ +338,5 @@ > + > + observe: function observe(aSubject, aTopic, aData) { > + > + if (!this._serverURL && > + ((aTopic !="nsPref:changed") || (aData != "dom.push.serverURL"))) { Remove the aData check. @@ +370,5 @@ > + > + this._activate_deactivate_queue.push([serverURL, true]); > + if (this._activate_deactivate_queue.length == 1) { > + setTimeout(this._activate_deactivate_pop.bind(this), 0); > + } All the same refactorings I pointed out in the other file. @@ +605,5 @@ > + var loadGroup = Cc["@mozilla.org/network/load-group;1"].createInstance(Ci.nsILoadGroup); > + chan.loadGroup = loadGroup; > + > + this._conns[subscriptionUri].channel = chan; > + this._conns[subscriptionUri].listener = listener; I'd prefer creating a local variable |conn| on which you set these fields, then... @@ +622,5 @@ > + this._conns[subscriptionUri].countUnableToConnect++; > + > + this._retryAfterBackoff(subscriptionUri); > + } > + ... once everything happens successfully, set this._conns[subscriptionUri] to it via destructuring. ::: dom/push/PushServiceLauncher.js @@ +41,5 @@ > if (isParent) { > Cu.import("resource://gre/modules/PushService.jsm"); > PushService.init(); > + Cu.import("resource://gre/modules/PushServiceHttp2.jsm"); > + PushServiceHttp2.init(); I'd recommend adding a pref "dom.push.http2.enabled" and switching between websocket and http2 based on that. At some point once the server is ready to go and we have preliminary results about the http2 backend, we should junk the websocket backend. ::: dom/push/test/test_register.html @@ +23,5 @@ > > <script class="testbody" type="text/javascript"> > > function debug(str) { > + console.log(str + "\n"); Please don't push debug changes to m-c.

Comment on attachment 8596666 [details] [diff] [review] part1_bug_1150812_use_h2_v1.patch Review of attachment 8596666 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/push/test/frame2.html @@ +4,5 @@ > + <meta http-equiv="Content-type" content="text/html;charset=UTF-8"> > + <script> > + > + function waitOnPushSubscriptionChange(swr) { > + var p= new Promise(function(res, rej) { Nit: p = ::: dom/push/test/test_disconnect_http2.html @@ +164,5 @@ > + this._res(this._pushSubscription); > + } > + } > + } > + Nit: Trailing whitespace.

Comment on attachment 8596666 [details] [diff] [review] part1_bug_1150812_use_h2_v1.patch Review of attachment 8596666 [details] [diff] [review]: ----------------------------------------------------------------- per mt's comments.

Thank you for the review. (In reply to Martin Thomson [:mt] from comment #5) > Comment on attachment 8596665 [details] [diff] [review] > part0_bug_1150812_use_h2_v1.patch > > Review of attachment 8596665 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think that this could use the arrow form of functions and avoid a lot of > calls to .bind(). > > I'm concerned about the amount of duplication between PushServiceHTTP2 and > PushService. There seems to be a lot of messages that they both listen to. > This could be a lot tighter if the push service instantiated the right > instance of the service based on the URL that it receives. This wouldn't be > a big concern if the websockets version were discarded at some point in the > near future. I though about making a lot tighter, but it was not that easy and I was expecting that http2 version will replace ws soon. Looking at it again I have decided to split it up into common and not common code. Removing ws will be some work. > > ::: dom/push/PushServiceHttp2.jsm > @@ +77,5 @@ > > + * Callback function to invoke with result ID. > > + * @param aErrorCb [optional] > > + * Callback function to invoke when there was an error. > > + */ > > + put: function(aRecord, aSuccessCb, aErrorCb) { > > All these functions could be trivially fixed to return a promise, which > would make use of them much easier. Done in bug 1162414. > > @@ +470,5 @@ > > + > > + let deferred = Promise.defer(); > > + > > + var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > > + .createInstance(Ci.nsIXMLHttpRequest); > > Rather than use XHR, perhaps this should use channels directly (at least > that is what :sicking suggested I do here: > https://dxr.mozilla.org/mozilla-central/source/dom/media/IdpSandbox.jsm#44). > Ok I have changed it. > > @@ +1169,5 @@ > > + > > + /** |delay| should be in milliseconds. */ > > + _setAlarm: function(delay) { > > + // Bug 909270: Since calls to AlarmService.add() are async, calls must be > > + // 'queued' to ensure only one alarm is ever active. > > A better way to deal with this is: > . setAlarm resolves a promise when it is registered > . setAlarm saves that promise each time that it is run > . setAlarm awaits that promise before doing its business > > i.e. > > _alarmPromise: Promise.resolve(), > _setAlarm: function(delay) { > var time = Date.now() + delay); > this._alarmPromise = this.alarmPromise > .then(_ => this._reallySetAlarm(time)); > }, > _reallySetAlarm; function(time) { > if (time < Date.now()) { > // hmm, missed the alarm, maybe we don't care about this > } > } > > The only drawback here is that using the promise attempts to set the alarm > are always enqueued, whereas the current code effectively cancels any > attempts to set the alarm. I have left the old alarm for now.

(In reply to Martin Thomson [:mt] from comment #6) > I'm going to suggest that you split the encryption part of this off. That > should let this land without getting into the details. I wanted to do that. This is big enough alone.

Thank you for the review. (In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #7) > Comment on attachment 8596665 [details] [diff] [review] > part0_bug_1150812_use_h2_v1.patch > > Review of attachment 8596665 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd like to take a look again once the comments (Martin's too) are > addressed. Thanks! > > @@ +230,5 @@ > > + if (aIID.equals(Ci.nsIHttpPushListener) || > > + aIID.equals(Ci.nsIStreamListener)) > > + return this; > > + throw Components.results.NS_ERROR_NO_INTERFACE; > > + }, > > Any reason QI and getInterface is defined? Usually JS components do not > require it as long as they satisfy the contract. getInterface for Ci.nsIHttpPushListener is needed here otherwise it does not work. > > @@ +278,5 @@ > > + _mainListener: null, > > + _message: "", > > + _ackUri: null, > > + > > + QueryInterface: function (aIID) { > > Same as above. here it isn't needed. (I have just copy it...) > > ::: dom/push/PushServiceLauncher.js > @@ +41,5 @@ > > if (isParent) { > > Cu.import("resource://gre/modules/PushService.jsm"); > > PushService.init(); > > + Cu.import("resource://gre/modules/PushServiceHttp2.jsm"); > > + PushServiceHttp2.init(); > > I'd recommend adding a pref "dom.push.http2.enabled" and switching between > websocket and http2 based on that. At some point once the server is ready to > go and we have preliminary results about the http2 backend, we should junk > the websocket backend. it depends on server uri whether it is wss or https.

Comment on attachment 8603354 [details] [diff] [review] part0_bug_1150812_use_h2_v2.patch Review of attachment 8603354 [details] [diff] [review]: ----------------------------------------------------------------- I hope there were no subtle changes in this patch and it was mainly moving code around. ::: dom/push/PushDB.jsm @@ +19,5 @@ > + > +this.EXPORTED_SYMBOLS = ["PushDB"]; > + > +// This is a singleton > +this.PushDB = function PushDB(dbName, dbVersion, dbStoreName, scemaFunction) { Nit: spelling is schema here and everywhere. ::: dom/push/PushServiceWebSocket.jsm @@ +490,5 @@ > + nextPingInterval = Math.floor(lastTriedPingInterval / 2); > + > + // If the new ping interval is close to the last good one, we are near > + // optimum, so stop calculating. > + if (nextPingInterval - this._lastGoodPingInterval < prefs.get('adaptive.gap')) { Please monitor Bug 1152264 and cherry pick those changes if that lands.

Comment on attachment 8603355 [details] [diff] [review] part1_bug_1150812_use_h2_v2.patch Review of attachment 8603355 [details] [diff] [review]: ----------------------------------------------------------------- Martin, please take a look at the http2 bits. ::: dom/push/PushService.jsm @@ +61,5 @@ > // associated with an app. > objectStore.createIndex("scope", "scope", { unique: true }); > }; > > +var upgradeSchemaHttp2 = function(aTransaction, aDb, aOldVersion, aNewVersion, aDbInstance) { How about moving these functions to their invididual service files? @@ +179,5 @@ > break; > } > }, > > + asyncTryProcessQueue: function() { tryAsyncProcessQueue since we are trying to process async and not async going to try to process. @@ +188,5 @@ > + > + _process_queue_pop: function() { > + debug("process_queue_pop()"); > + > + if (this._process_queue.length == 0 || this._process_queue_active) { can we make the active part of this check log to console if it is violated? @@ +231,5 @@ > + // First set into in active state, stop alarm and close db. > + this._stopService(cleanup) > + .then(_ => { > + this._startService(); > + this._process_queue_active = false; it sounds like a bad idea to set active to false inside an async call. Any reason for this? @@ +237,5 @@ > + } > + } else { > + this._stopObservers(); > + this._stopService(cleanup); > + this._process_queue_active = false; why not set this only once at the end of this function? @@ +277,5 @@ > + > + var serverURI = prefs.get("serverURL"); > + debug("serverURL " + serverURI); > + this._process_queue.push([serverURI, false]); > + if (this._process_queue.length == 1) { Better to just call asyncTryProcessQueue() here @@ +409,5 @@ > + > + Services.obs.removeObserver(this, "xpcom-shutdown", false); > + > + this._process_queue.push(["", false]); > + if (this._process_queue.length == 1) { tryAsyncProcessQueue(). @@ +630,5 @@ > debug("_onRegisterSuccess()"); > + if (this.serverURI.scheme === "wss") { > + return this._onRegisterSuccessWS(aPageRecord, generatedChannelID, data); > + } else { > + return this._onRegisterSuccessHttp2(aPageRecord, data); could these be moved onto their respective services and called just onRegisterSuccess? If not, that is ok. @@ +736,5 @@ > register: function(aPageRecord, aMessageManager) { > debug("register(): " + JSON.stringify(aPageRecord)); > > + if (!this._active) { > + aMessageManager.sendAsyncMessage("PushService:Register:KO", { a) Can this really happen? b) If it can, perhaps it would be better to queue up the requests than fail the client immediately. This can be a follow up. @@ +880,5 @@ > + pushEndpoint: pushRecord.pushEndpoint, > + version: pushRecord.version, > + lastPush: pushRecord.lastPush, > + pushCount: pushRecord.pushCount > + }; It would be better if this class didn't know about the details the 2 services need and just passed them the full record and had them do the filtering. ::: dom/push/PushServiceHttp2.jsm @@ +567,5 @@ > + }, > + > + onAlarmFired: function() { > + // Conditions are arranged in decreasing specificity. > + // i.e. when _waitingForPong is true, other conditions are also true. remove comment.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #18) > Comment on attachment 8603355 [details] [diff] [review] > part1_bug_1150812_use_h2_v2.patch > > Review of attachment 8603355 [details] [diff] [review]: > ----------------------------------------------------------------- > > Martin, please take a look at the http2 bits. > > ::: dom/push/PushService.jsm > @@ +61,5 @@ > > // associated with an app. > > objectStore.createIndex("scope", "scope", { unique: true }); > > }; > > > > +var upgradeSchemaHttp2 = function(aTransaction, aDb, aOldVersion, aNewVersion, aDbInstance) { > > How about moving these functions to their invididual service files? > > + return this._onRegisterSuccessWS(aPageRecord, generatedChannelID, data); > > + } else { > > + return this._onRegisterSuccessHttp2(aPageRecord, data); > > could these be moved onto their respective services and called just > onRegisterSuccess? If not, that is ok. Definitely it is better to move both of these. > @@ +231,5 @@ > > + // First set into in active state, stop alarm and close db. > > + this._stopService(cleanup) > > + .then(_ => { > > + this._startService(); > > + this._process_queue_active = false; > > it sounds like a bad idea to set active to false inside an async call. Any > reason for this? > > @@ +237,5 @@ > > + } > > + } else { > > + this._stopObservers(); > > + this._stopService(cleanup); > > + this._process_queue_active = false; > > why not set this only once at the end of this function? > > @@ +736,5 @@ > > register: function(aPageRecord, aMessageManager) { > > debug("register(): " + JSON.stringify(aPageRecord)); > > > > + if (!this._active) { > > + aMessageManager.sendAsyncMessage("PushService:Register:KO", { > > a) Can this really happen? > b) If it can, perhaps it would be better to queue up the requests than fail > the client immediately. This can be a follow up. The reason for process_queue and this _active are serverURL changes. I do not know how important this feature is. At PushService webSocket code there was a comment that this feature is only used for testing. Therefore I have not implemented request queuing. If server uri does not change this will never happen(as soon as db is up service will stay active and it is not active if there is no serveruri). If we change server url we must wait that db is cleaned up before changing it again therefore this._process_queue_active is set in async function. So my question is how important is a possibility to change serverURI?

Comment on attachment 8603358 [details] [diff] [review] part2_bug_1150812_use_h2_v2.patch Review of attachment 8603358 [details] [diff] [review]: ----------------------------------------------------------------- martin please look at the cert stuff. thanks! ::: dom/push/test/frame2.html @@ +4,5 @@ > + <meta http-equiv="Content-type" content="text/html;charset=UTF-8"> > + <script> > + > + function waitOnPushSubscriptionChange() { > + var p= new Promise(function(res, rej) { Nit: |p =| ::: dom/push/test/test_disconnect_http2.html @@ +23,5 @@ > +</pre> > + > +<script class="testbody" type="text/javascript"> > + > + var pushHttpServer = "https://localhost:8080"; How does this work on treeherder? ::: dom/push/test/worker.js @@ +20,5 @@ > +this.onpushsubscriptionchange = handlePushSubsciptionChange; > + > +function handlePushSubsciptionChange(event) { > + > + self.clients.matchAll().then(function(result) { Add something like this so we have a clue when things go wrong: if (result.length == 0) { dump("************* push test serviceworker: No clients! Something went wrong\n"); }

Comment on attachment 8603354 [details] [diff] [review] part0_bug_1150812_use_h2_v2.patch Review of attachment 8603354 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/push/PushDB.jsm @@ +18,5 @@ > +Cu.importGlobalProperties(["indexedDB"]); > + > +this.EXPORTED_SYMBOLS = ["PushDB"]; > + > +// This is a singleton I don't see how this is true. ::: dom/push/PushService.jsm @@ +124,2 @@ > > + this._service.unregister(records) semicolon @@ +142,3 @@ > }, > > init: function(options = {}) { does this actually need a default now? @@ +156,5 @@ > .getService(Ci.nsIMessageBroadcaster); > > kCHILD_PROCESS_MESSAGES.forEach(function addMessage(msgName) { > ppmm.addMessageListener(msgName, this); > }.bind(this)); use arrow => avoid .bind(this) @@ +498,5 @@ > + requestID: aPageRecord.requestID, > + pushEndpoint: pushRecord.pushEndpoint > + }; > + aMessageManager.sendAsyncMessage("PushService:Register:OK", message); > + }.bind(this, aPageRecord, aMessageManager), arrow function please: pushRecord => {...} aPageRecord and aMessageManager will just be captured by the closure @@ +572,5 @@ > + return new Promise((resolve, reject) => { > + this._db.clearAll() > + .then(_ => resolve(), > + _ => reject("Database error")); > + }); return this._db.clearAll().catch(e => { throw new Error("Database error"); }); ::: dom/push/PushServiceWebSocket.jsm @@ +1,4 @@ > +/* jshint moz: true, esnext: true */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ I'm going to assume that the bulk of this file is just moved code AND not really wanted long term. I've not thoroughly reviewed it. (I wouldn't want to land this code, but I won't hold you responsible.) @@ +52,5 @@ > + * that the PushService can silence messages from the WebSocket by setting > + * PushWebSocketListener._pushService to null. This is required because > + * a WebSocket can continue to send messages or errors after it has been > + * closed but the PushService may not be interested in these. It's easier to > + * stop listening than to have checks at specific points. Don't have the push service set the value to null. Have the push service call a function that sets the value to null. @@ +54,5 @@ > + * a WebSocket can continue to send messages or errors after it has been > + * closed but the PushService may not be interested in these. It's easier to > + * stop listening than to have checks at specific points. > + */ > +this.PushWebSocketListener = function(pushService) { If this isn't exported (see EXPORTED_SYMBOLS) why is it being set like this (rather than ``function PushWebSocketListener(...)`` @@ +82,5 @@ > + > + onMessageAvailable: function(context, message) { > + if (!this._pushService) > + return; > + this._pushService._wsOnMessageAvailable(context, message); I don't think that it makes sense to have the WS-specific part of this code structured in this fashion. You have an ostensibly-ws-specific class (or set of classes) in this file, this file should be handling the protocol-specific parts. When it comes down to it, a generic push service needs to have something like: connect(), disconnect(), onconnected, ondisconnected, subscribe(), onmessage, ...

Comment on attachment 8603355 [details] [diff] [review] part1_bug_1150812_use_h2_v2.patch Review of attachment 8603355 [details] [diff] [review]: ----------------------------------------------------------------- I'm not done yet, but I am tired. ::: dom/push/PushService.jsm @@ +110,5 @@ > + debug("dom.push.serverURL changed! new value " + serverURI); > + > + this._process_queue.push([serverURI, true]); > + if (this._process_queue.length == 1) { > + setTimeout(this._process_queue_pop.bind(this), 0); setTimeout(_ => this.process_queue_pop(), 0) Though I don't get why this isn't just calling the function from the timeout. On the whole, I think that this scheme for managing synchronization is a poor construction. If the problem is that you can't call code from here directly, setTimeout(..., 0) is a reasonable way to manage it. And that might be sufficient if that were the only problem. But it looks like there is a second issue here. You want to ensure that only one operation happens at a time. A better way to manage that is with a promise. _queue: Promise.resolve(), _enqueue: function(op) { this._queue = this._queue.then(op).catch(_ => {}); }, ... this._enqueue(_ => this.doSomething()); ... @@ +199,5 @@ > + this.asyncTryProcessQueue(); > + > + if (!serverURI) { > + debug("No dom.push.serverURI found!"); > + this.serverURI = null; This could return early if you split this into a function for getting the server URI and a function for acting upon it. @@ +325,5 @@ > prefs.observe("connection.enabled", this); > // Debugging > prefs.observe("debug", this); > + > + this._observerStarted= true; ' = ' @@ +335,5 @@ > + return; > + } > + > + this.stopAlarm(); > + if (this.serverURI.scheme === "wss") { why isn't this in the service-specific files? @@ +374,5 @@ > + _stopService: function(cleanup) { > + this._active = false; > + this.stopAlarm(); > + this._service.uninit(); > + return new Promise((resolve, reject) => { This wrapping is unnecessary. Just do the if statements and return the promises at each point. if (!this._db) { return Promise.resolve(); // or just return, if the call to this is wrapped in a .then } if (cleanup) { return this.notify...().then...; } this._db.close(); return @@ +378,5 @@ > + return new Promise((resolve, reject) => { > + if (this._db) { > + if (cleanup) { > + this.notifyAllAppsRegister() > + .then(this.dropRegistrations.bind(this)) arrow @@ +675,5 @@ > + function(error) { > + // Unable to save. > + this._service.unregister(record); > + throw error; > + }.bind(this) arrow @@ +789,5 @@ > */ > _unregister: function(aPageRecord) { > debug("unregisterWithServer()"); > > + return new Promise((resolve, reject) => { again, unnecessary @@ +792,5 @@ > > + return new Promise((resolve, reject) => { > + let fail = function(error) { > + debug("unregister() fail() error " + error); > + reject(error); return Promise.reject(...) @@ +798,3 @@ > > + if (!aPageRecord.scope) { > + fail("NotFoundError"); return Promse.reject(..) @@ +803,2 @@ > > + this._db.getByScope(aPageRecord.scope) return here @@ +820,5 @@ > + // Let's be nice to the server and try to inform it, > + // but we don't care about the reply. > + this._service.unregister(record); > + resolve(); > + }, err => fail(err)); let this error fall through

Comment on attachment 8603355 [details] [diff] [review] part1_bug_1150812_use_h2_v2.patch I have decided to to do more refactoring of ws version. I did not want to do it because the code is rather not easy... So please wait for a new version.

/r/8901 - Bug 1150812 - split PushService - separate webSocket part. r=nsm, r=mt /r/8903 - Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt /r/8905 - Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt /r/8907 - Bug 1150812 - Add Http2 Push service. r=nsm, r=mt /r/8909 - Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Pull down these commits: hg pull -r bb6c468afbae24342decf6d5d9ef6931a079d0e1 https://reviewboard-hg.mozilla.org/gecko/

Bug 1150812 - split PushService - separate webSocket part. r=nsm, r=mt This is just splitting with no real code change. Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt This is making changes to the original code. Only part that Martin had suggested to change and I have not is set/stopAlarm(the patches are already big enough and that can be easily changed in a separat bug) Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt This are only tests Bug 1150812 - Add Http2 Push service. r=nsm, r=mt Adding http part. Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Adding xpchell tests

This are mochitest which will not be landed until we have a PushServer running in our test environment.

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana https://reviewboard.mozilla.org/r/8899/#review7609 I think that I'd like to see this again. I might also recommend running eslint or jshint over this. The Hello configuration is probably sane. You don't have to fix everything it reports, though I'd prefer warning-clean code. ::: dom/push/PushDB.jsm:19 (Diff revision 1) > +const Cu = Components.utils; Components.utils is used on line 11. ::: dom/push/PushDB.jsm:42 (Diff revision 1) > + _dbName: null, > + _dbVersion: null, > + _dbStoreName: null, > + _schemaFunction: null, I generally avoid declaring variables as null on the prototype if they are always set in the constructor. Also, I don't see the values for `_dbName` and `_dbVersion` being used anywhere. ::: dom/push/PushDB.jsm:25 (Diff revision 1) > +// This is a singleton Still not true. You might only instantiate one, but that's not the same thing. ::: dom/push/PushDB.jsm:81 (Diff revision 1) > + nit: empty line ::: dom/push/PushService.jsm:53 (Diff revision 1) > +/** > + * The implementation of the SimplePush system. This runs in the B2G parent > + * process and is started on boot. It uses WebSockets to communicate with the > + * server and PushDB (IndexedDB) for persistence. > - */ > + */ Is this comment accurate? SimplePush == WebSocket. ::: dom/push/PushService.jsm:71 (Diff revision 1) > - ) > - ); > - }, > + if (!this._requestQueue) { > + this._requestQueue = this._requestQueueStart; > + } You don't need to add this, or `_requestQueueStart`. It should be sufficient to initialize `_requestQueue = Promise.resolve()`. ::: dom/push/PushService.jsm:84 (Diff revision 1) > - resolve, > - reject > - ) > + if (!this._serverURIProcessQueue) { > + this._serverURIProcessQueue = this._serverURIProcessQueueStart; > + } As above. ::: dom/push/PushService.jsm:98 (Diff revision 1) > - aTxn.result = undefined; > + _offlineState: function(offline) { This name is a little confusing. `_offlineState` reads like a getter, but in practice it's a setter. ::: dom/push/PushService.jsm:117 (Diff revision 1) > - }, > - resolve, > - reject > - ) > - ); > - }, > - > - drop: function() { > + this._state = PUSH_SERVICE_RUNNING; > + debug("this._state: " + this._state); > + this._db.getAllKeyIDs() > + .then(keyIDs => { > + if ((this._pendingRegisterRequest.length > 0) || > + (keyIDs.length > 0)) { > + debug("this._state: " + this._state); > + this._service.connect(this._requestQueueStart, keyIDs); I'm not seeing any evidence that pending register requests are retried here. Is the idea that pending requests would just fail (or succeed) independent of any of this churn and be retried by content? ::: dom/push/PushService.jsm:130 (Diff revision 1) > - onBinaryMessageAvailable: function(context, message) { > + _connectionEnabledPref: function(enabled) { Again, this looks like a getter, when it's a setter. Suggest using a verb: `_enableConnection`. ::: dom/push/PushService.jsm:656 (Diff revision 1) > - _generateID: function() { > - let uuidGenerator = Cc["@mozilla.org/uuid-generator;1"] > - .getService(Ci.nsIUUIDGenerator); > - // generateUUID() gives a UUID surrounded by {...}, slice them off. > - return uuidGenerator.generateUUID().toString().slice(1, -1); > + _enqueueRequest: function(op) { > + debug("enqueueRequest(), state: " + this._state + " waiting for connected: " + this._notifyRequestQueue); > + > + if (this._state == PUSH_SERVICE_ACTIVATING) { > + // Wait until service is active. > + if (!this._notifyRequestQueue) { > + var p = new Promise((resolve, reject) => { > + this._notifyRequestQueue = resolve; > + }); > + this._enqueueRequestQueue( > + function() { > + return p; > + }); > + } > + } > + > + // Enqueue operation. > + this._enqueueRequestQueue(op); > }, Rather than this complicated structure, how about using a less complicated structure: Initially, and any time that the state leaves `..._RUNNING`: ``` _connected = new Promise(r => this._notifyConnected = r); ``` Then, whenever you enter `..._RUNNING`: ``` this._state = PUSH_SERVICE_RUNNING; this._notifyConnected(); ``` Then any operations that are waiting for connection just do: ``` this._connected.then(_ => doSomething()); ``` I would strongly recommend adding a `_setState() ` function here to more cleanly manage state-driven activity, rather than setting state in an ad-hoc fashion. ::: dom/push/PushService.jsm:621 (Diff revision 1) > - debug("receiveMessage(): " + aMessage.name); > + return this._db.getAllKeyIDs() missing ; ::: dom/push/PushService.jsm:617 (Diff revision 1) > - return this._db.drop(); > + return this._db.getByKeyID(aKeyID) missing ; ::: dom/push/PushService.jsm:595 (Diff revision 1) > if(Services.perms.testExactPermission(scopeURI, "push") != Ci.nsIPermissionManager.ALLOW_ACTION) { 'if (' ::: dom/push/PushService.jsm:510 (Diff revision 1) > + }.bind(this) Use the arrow form rather than `function{...}.bind(this)` ::: dom/push/PushService.jsm:678 (Diff revision 1) > + return new Promise((resolve, reject) => Shouldn't need to wrap this with `new Promise` ::: dom/push/PushService.jsm:689 (Diff revision 1) > - let channelID = this._generateID(); > + if (typeof this._pendingRegisterRequest[aPageRecord.scope] == "object") { if (x) { then return x; } ::: dom/push/PushService.jsm:684 (Diff revision 1) > + this._db.getByScope(aPageRecord.scope) Just return this. ::: dom/push/PushService.jsm:679 (Diff revision 1) > + this._enqueueRequest(_ => { > + if (this._state < PUSH_SERVICE_ACTIVATING) { > + reject({state: 0, error: "Service not active"}); > + return; > + } > + this._db.getByScope(aPageRecord.scope) > + .then(resolve, reject); > + })) > .then(pushRecord => { The consequence of this is that only the block inside `_enqueueRequest` is serialized. What is `_enqueueRequest` guarding? ::: dom/push/PushService.jsm:696 (Diff revision 1) > error => { > debug("getByScope failed"); > throw "Database error"; > } Rethrowing like this makes it hard to get to the source of the problem. Just remove the error handler. ::: dom/push/PushService.jsm:711 (Diff revision 1) > .then( > function() { > - return record; > + return aRecord; > }, > function(error) { > // Unable to save. > - this._send("unregister", {channelID: record.channelID}); > + this._sendRequest("unregister", aRecord); > throw error; > }.bind(this) > ); ``` .then(_ => aRecord, error => { this._sendRequest("unregister", aRecord); throw error; }); ``` ::: dom/push/PushService.jsm:753 (Diff revision 1) > + function(aPageRecord, aMessageManager, pushRecord) { > + let message = { > + requestID: aPageRecord.requestID, > + pushEndpoint: pushRecord.pushEndpoint > + }; > + aMessageManager.sendAsyncMessage("PushService:Register:OK", message); > + }.bind(this, aPageRecord, aMessageManager), The complicated .bind call isn't needed, just use the closure. ``` .then( pushRecord => { let message = { ... }; aMessageManager.sendAsyncMessage(...); }, error => { ... } ``` ::: dom/push/PushService.jsm:802 (Diff revision 1) > - return this._db.getByScope(aPageRecord.scope) > + return new Promise((resolve, reject) => No `new Promise()` wrapper needed. ::: dom/push/PushService.jsm:808 (Diff revision 1) > + this._db.getByScope(aPageRecord.scope) Given that you have a record, do you really need to look it up before you remove it? I note that this is called from places where the record hasn't even be added. ::: dom/push/PushService.jsm:858 (Diff revision 1) > - return this._db.getByScope(aPageRecord.scope) > + return new Promise((resolve, reject) => Unwrap. ::: dom/push/PushService.jsm:817 (Diff revision 1) > - this._db.delete(record.channelID) > + this._db.delete(this._service.getKeyFromRecord(record)) > .then(_ => > // Let's be nice to the server and try to inform it, but we don't care > // about the reply. > - this._send("unregister", {channelID: record.channelID}) > + this._sendRequest("unregister", record) I would suggest that you run these in parallel (use Promise.all()). And you should await the unregister message. You can swallow errors, but at least debug log them. Also, the delete isn't enqueued under `_enqueueRequest`. I don't know if that is a problem, because I don't know what `_enqueueRequest` guards. ::: dom/push/PushService.jsm:868 (Diff revision 1) > - let registration = null; > + let registration = null; > - if (pushRecord) { > + if (pushRecord) { > - registration = { > + registration = { if (!pushRecord) { return null; } return { pushEndpoint: pushRecord.pushEndpoint, ... }; ::: dom/push/PushService.jsm:878 (Diff revision 1) > - }, _ => { > + }, _ => { > - throw "Database error"; > + throw "Database error"; > - }); > + }); Don't wrap errors like this. ::: dom/push/PushService.jsm:888 (Diff revision 1) > aMessageManager.sendAsyncMessage("PushService:Registration:OK", { > requestID: aPageRecord.requestID, > registration > }); IMPORTANT: this isn't a valid object initializer form. registration will be parsed as a key with no value (maybe, I don't know what this actually means in JS. ::: dom/push/PushService.jsm:838 (Diff revision 1) > requestID: aPageRecord.requestID, > error IMPORTANT: invalid code ::: dom/push/PushService.jsm:894 (Diff revision 1) > aMessageManager.sendAsyncMessage("PushService:Registration:KO", { > requestID: aPageRecord.requestID, > error > }); IMPORTANT: invalid code ::: dom/push/PushServiceHttp2.jsm:31 (Diff revision 1) > + if (gDebuggingEnabled) > + dump("-*- PushServiceHttp2.jsm: " + s + "\n"); Braces please. ::: dom/push/PushDB.jsm:15 (Diff revision 1) > + if (gDebuggingEnabled) > + dump("-*- PushDB.jsm: " + s + "\n"); Braces please. ::: dom/push/PushServiceHttp2.jsm:61 (Diff revision 1) > + if (aIID.equals(Ci.nsIHttpPushListener) || > + aIID.equals(Ci.nsIStreamListener)) > + return this; Braces. ::: dom/push/PushServiceHttp2.jsm:183 (Diff revision 1) > + // We are not cancelling subcscriptions so check if we are still active. nit: typo ::: dom/push/PushServiceHttp2.jsm:208 (Diff revision 1) > + var pushEndpoint; > + var pushReceiptEndpoint; It might pay to split the link header field parsing into a new function. This is really long. ::: dom/push/PushServiceHttp2.jsm:223 (Diff revision 1) > + for (var inx = 0; inx < linkList.length; inx++) { linkList.forEach(link => { var linkElems = link.split(';'); }) ::: dom/push/PushServiceHttp2.jsm:227 (Diff revision 1) > + if ((/ *rel="urn:ietf:params:push" */.test(linkElems[1])) && linkElements[1].trim() === 'rel="urn..."' ::: dom/push/PushServiceHttp2.jsm:242 (Diff revision 1) > + if (!pushEndpoint || !pushReceiptEndpoint || !subscriptionUri) { I don't think that we should be blocking on the receipt endpoint being present. If the server doesn't provide a receipt endpoint, we should still surface the message. ::: dom/push/PushServiceHttp2.jsm:262 (Diff revision 1) > + this._service.subscribed(subscriptionUri); Why have a subscribed() callback AND a promise that resolves? ::: dom/push/PushServiceHttp2.jsm:340 (Diff revision 1) > + gPushSubscriptionPath = prefs.get("http2.path"); I don't think that you should have a separate path pref. The server URL includes a path. Use that. ::: dom/push/PushServiceHttp2.jsm:372 (Diff revision 1) > + subscribed: function(aSubscriptionUri) { > + this._conns[aSubscriptionUri] = {channel: null, > + listener: null, > + countUnableToConnect: 0, > + waitingForAlarm: false}; > + this._listenForMsgs(aSubscriptionUri); > + }, As noted above, this can be moved to the `_subscribeResource` function. ::: dom/push/PushServiceHttp2.jsm:405 (Diff revision 1) > + var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] > + .createInstance(Ci.nsIXMLHttpRequest); > + req.open("DELETE", aUri, true); > + req.send(null); _makeChannel() ::: dom/push/PushServiceHttp2.jsm:431 (Diff revision 1) > + var chan = this._makeChannel(aSubscriptionUri); This doesn't have a base URI set. If you follow my advice regarding the pref, then `_makeChannel` will only have one argument, so this might be less of a problem. ::: dom/push/PushServiceHttp2.jsm:443 (Diff revision 1) > + conn.listener._pushService = null; Don't set private values on other objects. listener should provide a `disconnect()` function to do this for you. ::: dom/push/PushServiceHttp2.jsm:451 (Diff revision 1) > + [this._conns[aSubscriptionUri].channel, this._conns[aSubscriptionUri].listener] = > + [conn.channel, conn.listener]; Two separate assignments please. ::: dom/push/PushServiceHttp2.jsm:486 (Diff revision 1) > + let retryTimeout = prefs.get("http2.retryInterval") * > + Math.pow(2, this._conns[aSubscriptionUri].countUnableToConnect); Please add a +/-20% random bias to this backoff. ::: dom/push/PushServiceHttp2.jsm:497 (Diff revision 1) > + if (typeof this._conns[subscriptionUri] == "object") { if (this._conns[subscriptionUri]) { ::: dom/push/PushServiceHttp2.jsm:607 (Diff revision 1) > + } else if (aRequest.responseStatus == 204) { // This should be 204 I think that I'd just look for 2xx here. ::: dom/push/PushServiceHttp2.jsm:608 (Diff revision 1) > + this._listenForMsgs.bind(this, aSubscriptionUri); IMPORTANT: this does nothing. ::: dom/push/PushServiceHttp2.jsm:606 (Diff revision 1) > + this._mainPushService.dropRegistrationAndNotifyApp(aSubscriptionUri); IMPORTANT: We should make a new subscription at this point. Not just drop it on the floor. This is what generates the pushsubscriptionchange event: http://w3c.github.io/push-api/index.html#the-pushsubscriptionchange-event ::: dom/push/PushServiceHttp2.jsm:617 (Diff revision 1) > + let sendNotification = function(aAckUri, aPushRecord) { > + aPushRecord.pushCount = aPushRecord.pushCount + 1; > + aPushRecord.lastPush = new Date().getTime(); > + this._mainPushService.receivedPushMessage(aPushRecord, aMessage); > + this._ackMsgRecv(aAckUri); > + } Please use an arrow function here rather than bind. ::: dom/push/PushServiceWebSocket.jsm:438 (Diff revision 1) > + _calculateAdaptivePing: function(wsWentDown) { This function is frightful. I won't ask you to change it, because the current plan is to have the server drive. ::: dom/push/PushServiceWebSocket.jsm:745 (Diff revision 1) > +#ifdef MOZ_B2G We should not be preprocessing our JS. Again, I won't ask you to fix this though, just a gripe. ::: dom/push/test/test_multiple_register_different_scope.html:42 (Diff revision 1) > + dump("Unregistering the SW failed with " + err + "\n"); ok(false,...) should be ok here. ::: dom/push/test/test_multiple_register_different_scope.html:60 (Diff revision 1) > + return new Promise((res, rej) => { > + var firstSub; > + var checkEqual = function(sub) { > + if (!firstSub) { > + firstSub = sub; > + } else { > + ok(firstSub.endpoint != sub.endpoint, "setupMultipleSubscriptions - Got different endpoints."); > + res({sub1: firstSub, sub2: sub}); > + } > + }; > + > + subscribe(swr1).then(sub => checkEqual(sub)); > + subscribe(swr2).then(sub => checkEqual(sub)); > + }); This is the clumsy way. More elegantly: ``` return Promise.all([ subscribe(swr1), subscribe(swr2) ]).then(a => { ok(a[0].endpoint !== a[1].endpoint, 'should be different'); return a; }); ::: dom/push/test/test_multiple_register_during_service_activation.html:58 (Diff revision 1) > + function setupMultipleSubscriptions(swr) { See previous comment ::: dom/push/test/test_multiple_register_during_service_activation.html:85 (Diff revision 1) > + function getEndpoint(swr, subOld) { It seems like you could combine this with the previous function. ::: dom/push/test/test_try_registering_offline_disabled.html:89 (Diff revision 1) > + function observer(res) { Maybe name this OfflineObserver. ::: dom/push/test/test_try_registering_offline_disabled.html:119 (Diff revision 1) > + function goOnline() { This duplicates goOffline. Combine. ::: dom/push/test/xpcshell/test_register_error_http2.js:11 (Diff revision 1) > +var CertOverrideListener = function(host, port, bits) { Please move CertOverrideListener to a common file. You can have a head-http2.js file included everywhere. ::: dom/push/test/xpcshell/test_notification_http2.js:13 (Diff revision 1) > + if (port) { > + this.port = port; > + } this.port = port || 443; And remove the default from the prototype. ::: dom/push/test/xpcshell/test_notification_http2.js:51 (Diff revision 1) > + if (port) { If port is -1, this will be true. But that's not what you want. Try port !== 443. ::: dom/push/test/xpcshell/test_register_error_http2.js:104 (Diff revision 1) > + db bug? ::: dom/push/test/xpcshell/test_register_success_http2.js:70 (Diff revision 1) > +function run_test() { This can probably be moved into a common file too. ::: dom/push/test/xpcshell/test_registration_success_http2.js:77 (Diff revision 1) > + prefs.setBoolPref("network.http.spdy.enforce-tls-profile", tlsProfile); Um, tlsProfile === undefined? Just set the value to false then. ::: testing/xpcshell/moz-http2/moz-http2.js:411 (Diff revision 1) > + 'https://localhost:' + serverPort + '/s/665e33a7-4f1a-457b-a088-b14a80b1b28b'); Do these really have to be UUIDs? Why not give them nice names? Those names could describe what their function is much more clearly than opaque strings of gibberish. ::: dom/push/PushService.jsm:644 (Diff revision 1) > - ); > + if (typeof this._pendingRegisterRequest[aPageRecord.scope] == "object") { if (this.\_pendingRegisterRequest\[aPageRecord.scope\]) should suffice. ::: dom/push/PushServiceHttp2.jsm:524 (Diff revision 1) > + this._conns[record.subscriptionUri] = {channel: null, > + listener: null, > + countUnableToConnect: 0, > + waitingForAlarm: false}; nk that I'd like to see this aThis structure looks big enough to warrant having a class for it. gain

Comment on attachment 8607026 [details] [diff] [review] mochitests Review of attachment 8607026 [details] [diff] [review]: ----------------------------------------------------------------- I can't really r+ something that doesn't run. What is your intent with these? Do you intend to modify the mochitest runners to run a push server? My experience is that that is a lengthy and challenging process. ::: dom/push/test/cert_override.js @@ +1,4 @@ > + > +function addCertOverride(uri, bits) { > + debug("addCertOverride"); > + var p = new Promise(function(res, rej) { If you don't use rej, then: var p = new Promise(res => { @@ +1,5 @@ > + > +function addCertOverride(uri, bits) { > + debug("addCertOverride"); > + var p = new Promise(function(res, rej) { > + var reqCert =SpecialPowers.Cc["@mozilla.org/xmlextras/xmlhttprequest;1"] ' = ' @@ +37,5 @@ > + } > + }); > + > + // In case the certificats are accepted. > + reqCert.onreadystatechange = function() { I'd just use onload. @@ +39,5 @@ > + > + // In case the certificats are accepted. > + reqCert.onreadystatechange = function() { > + debug("XHR state: "+reqCert.readyState ); > + if (reqCert.readyState===4){ ' === ' ::: dom/push/test/frame2.html @@ +7,5 @@ > + function waitOnPushSubscriptionChange() { > + var p = new Promise(function(res, rej) { > + navigator.serviceWorker.onmessage = function(e) { > + if (e.data.type == "change") { > + res(null); res(); ::: dom/push/test/test_disconnect_http2.html @@ +33,5 @@ > + function createControlledIFrame() { > + var p = new Promise(function(res, rej) { > + var iframe = document.createElement('iframe'); > + iframe.id = "controlledFrame"; > + iframe.src = "http://mochi.test:8888/tests/dom/push/test/frame.html"; Is this going to work on a http:// URI? Try https://example.com/ instead perhaps. @@ +44,5 @@ > + return p; > + } > + > + function sendPushToPushServer(pushEndpoint) { > + // TODO: send this to app server, but need to figure out how to You don't need an app server. The push endpoint should accept pushes from browsers, though it might need to be modified to accept OPTIONS requests. ::: dom/push/test/test_register_http2.html @@ +46,5 @@ > + > + function sendPushToPushServer(pushEndpoint) { > + // TODO: send this to app server, but need to figure out how to > + // addCertOverride there. > + var xhr = new XMLHttpRequest(); I think that fetch would be better here. @@ +87,5 @@ > + > + function waitForPushNotification(pushSubscription, controlledFrame) { > + debug("waitForPushNotification"); > + var p = controlledFrame.contentWindow.waitOnPushMessage(); > + sendPushToPushServer(pushSubscription.endpoint); You should comment that you aren't awaiting the completion of the push if that is what you intend. I would though. You can lose errors if you don't fold everything you do into the same chain. Promise.all() is good if you have to do two things "at once". @@ +113,5 @@ > + .then(controlledFrame => > + subscribe(swr) > + .then(sub => > + checkHasSubscription(swr) > + .then(_ => waitForPushNotification(sub, controlledFrame)) You can step up a level of indenting here.

(In reply to Martin Thomson [:mt] from comment #27) > Comment on attachment 8607020 [details] > MozReview Request: bz://1150812/dragana > > ::: dom/push/PushServiceHttp2.jsm:606 > (Diff revision 1) > > + this._mainPushService.dropRegistrationAndNotifyApp(aSubscriptionUri); > > IMPORTANT: We should make a new subscription at this point. Not just drop > it on the floor. > > This is what generates the pushsubscriptionchange event: > http://w3c.github.io/push-api/index.html#the-pushsubscriptionchange-event > So there are different scenarios. We got subscription and we are trying to connect to the server to listen for messages: 1) the connection could not be established, can be network reasons --- retry n times and if it is still failing notify the app with pushsubscriptionchange and resubscribe (this can fail too but they can be two different hosts, load balancing). if the resubscription fails delete subscription from db How to notify app that the resubscription succeeded? Send another pushsubscriptionchange or the app should poll the registration(then the db entry must be invalidated, depends if we send the second pushsubscriptionchange)? 2) server returns 4xx --- notify app with pushsubscriptionchange and resubscribe. if the resubscription fails delete the subscription from db. The same questions as above. 3) returns a code other than 4xx and 2xx --- do the same as 1) For completeness: 4) returns 2xx --- just reconnect.

(In reply to Martin Thomson [:mt] from comment #28) > Comment on attachment 8607026 [details] [diff] [review] > mochitests > > Review of attachment 8607026 [details] [diff] [review]: > ----------------------------------------------------------------- > > I can't really r+ something that doesn't run. > > What is your intent with these? Do you intend to modify the mochitest > runners to run a push server? My experience is that that is a lengthy and > challenging process. we have already http2 server using node running, so it is not going to be hard to add pushServer. I would like to do this in another bug. This patch is here because it is useful for testing, but you are right I should have ask only for feedback.

To comment #29 Send pushsubscriptionchange when resubscribing succeeded or failed not at the point when a problem is discovered.

(In reply to Dragana Damjanovic [:dragana] from comment #29) > > This is what generates the pushsubscriptionchange event: > > http://w3c.github.io/push-api/index.html#the-pushsubscriptionchange-event > > > > > So there are different scenarios. > We got subscription and we are trying to connect to the server to listen for > messages: > 1) the connection could not be established, can be network reasons --- retry > n times and if it is still failing notify the app with > pushsubscriptionchange and resubscribe (this can fail too but they can be > two different hosts, load balancing). if the resubscription fails delete > subscription from db > How to notify app that the resubscription succeeded? Send another > pushsubscriptionchange or the app should poll the registration(then the db > entry must be invalidated, depends if we send the second > pushsubscriptionchange)? This case is one where I would rather you start the subscription process over. That means that subscription will need retries, which it should have anyway. If the retries expire, then we might need to add some sort of error notification and let the app SW deal with the problem. Opened https://github.com/w3c/push-api/issues/149 > 2) server returns 4xx --- notify app with pushsubscriptionchange and > resubscribe. if the resubscription fails delete the subscription from db. > The same questions as above. 4xx means that the subscription is (likely) dead, yes. Automatically resubscribing is probably the best option. I'd be more cautious with 4xx because it could signal more fundamental issues on our end, like a loss of authentication. > 3) returns a code other than 4xx and 2xx --- do the same as 1) > > For completeness: 4) returns 2xx --- just reconnect. Yes, a simple reconnect unless you get a 4xx. Even reconnect on 5xx, though you might like to look at the Retry-After header field in that case. For the most part, I'd treat 5xx as equivalent to a network error. (In reply to Dragana Damjanovic [:dragana] from comment #30) > we have already http2 server using node running, so it is not going to be > hard to add pushServer. > > I would like to do this in another bug. This patch is here because it is > useful for testing, but you are right I should have ask only for feedback. If it is useful for testing, then it should be landed and tested. I think that mochitests are a slightly different beast than xpcshell tests, you might find them slightly harder to work with.

(In reply to Dragana Damjanovic [:dragana] from comment #31) > To comment #29 > > Send pushsubscriptionchange when resubscribing succeeded or failed not at > the point when a problem is discovered. Well, there's a gap: do you notify when you detect the error, or when you repair it? That's why I think we need pushsubscriptionerror or a similar event. That's different from the change in that it means that the subscription is gone and the application will be required to request a new one at that point.

I will wait until mt's comments are addressed.

https://reviewboard.mozilla.org/r/8899/#review7649 > I generally avoid declaring variables as null on the prototype if they are always set in the constructor. > > Also, I don't see the values for `_dbName` and `_dbVersion` being used anywhere. I have just copied it from the old code. Will be fixed in next version. > IMPORTANT: this isn't a valid object initializer form. registration will be parsed as a key with no value (maybe, I don't know what this actually means in JS. This works.(ECMAScript 6 - Shorthand property names) > IMPORTANT: invalid code see above. > This function is frightful. I won't ask you to change it, because the current plan is to have the server drive. I did not want to change to much of only WebSocket part. > bug? this works. > This can probably be moved into a common file too. They are not all the same, so I will leave it in separate files. > Um, tlsProfile === undefined? Just set the value to false then. oop, it is left there from my last change, it is not needed. > Rethrowing like this makes it hard to get to the source of the problem. Just remove the error handler. this is from the original code, I will change it, I do not know why is it done like this. > I'm not seeing any evidence that pending register requests are retried here. Is the idea that pending requests would just fail (or succeed) independent of any of this churn and be retried by content? my mistake. I was changing some stuff and forgot to fix this. > This name is a little confusing. `_offlineState` reads like a getter, but in practice it's a setter. Changed to changeStateOfflineEvent. > Again, this looks like a getter, when it's a setter. > > Suggest using a verb: `_enableConnection`. Changed to changeStateConnectionEnabledEvent > The complicated .bind call isn't needed, just use the closure. > > ``` > .then( > pushRecord => { > let message = { ... }; > aMessageManager.sendAsyncMessage(...); > }, > error => { ... } > ``` That is all old code. I will fix it. > Given that you have a record, do you really need to look it up before you remove it? I note that this is called from places where the record hasn't even be added. This is page record what the app sends amd it does not contain channelID. > IMPORTANT: invalid code this works with ES6 > Don't wrap errors like this. This is from the old code, I will check why they needed that.

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana /r/8901 - Bug 1150812 - split PushService - separate webSocket part. r=nsm, r=mt /r/8903 - Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt /r/8905 - Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt /r/8907 - Bug 1150812 - Add Http2 Push service. r=nsm, r=mt /r/8909 - Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Pull down these commits: hg pull -r 08e8d657ddcbc4b86515b47fd0b4ceb36ef0ed3a https://reviewboard-hg.mozilla.org/gecko/

(In reply to Martin Thomson [:mt] from comment #32) > (In reply to Dragana Damjanovic [:dragana] from comment #29) > > > This is what generates the pushsubscriptionchange event: > > > http://w3c.github.io/push-api/index.html#the-pushsubscriptionchange-event > > > >bro case 1) connections is broken, no response code: retry to connect n times, with backoff, it it fails it tries to resubscribe case 2) code 5xx: look for Retry-After header,retry to connect n times, with backoff, it it fails it tries to resubscribe case 3) 4xx code: resubscribe case 4) 2xx reconnect resubscribe: try to subscribe again: on success delete old subscription, add new one and send pushsubscriptionchange (in this order) on error delete old subscription and send pushsubscriptionchange

https://reviewboard.mozilla.org/r/8899/#review7947 > This works.(ECMAScript 6 - Shorthand property names) Damn. I guess it's OK. I don't like it though. > They are not all the same, so I will leave it in separate files. If they look that much the same, then you have a maintenance problem. Please try to parameterize any variances.

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana /r/8901 - Bug 1150812 - split PushService - separate webSocket part. r=nsm, r=mt /r/8903 - Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt /r/8905 - Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt /r/8907 - Bug 1150812 - Add Http2 Push service. r=nsm, r=mt /r/8909 - Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Pull down these commits: hg pull -r 264256c129964f22f2c60bf09396677d5be9e012 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana /r/8901 - Bug 1150812 - split PushService - separate webSocket part. r=nsm, r=mt /r/8903 - Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt /r/8905 - Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt /r/8907 - Bug 1150812 - Add Http2 Push service. r=nsm, r=mt /r/8909 - Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Pull down these commits: hg pull -r 1d624f149afaed690ba9e229574541c8b1ab0e16 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana https://reviewboard.mozilla.org/r/8899/#review7989 ::: dom/push/PushServiceHttp2.jsm:306 (Diff revisions 2 - 4) > + retryAfter = retryField * 1000; Please use parseInt(retryField, 10) ::: dom/push/PushServiceHttp2.jsm:309 (Diff revisions 2 - 4) > + } catch(e) {} What is this catching? If it's just Date.parse(), then only capture that.

https://reviewboard.mozilla.org/r/8901/#review7997 ::: dom/push/PushService.jsm:199 (Diff revision 2) > + this._db = new PushDB(kPUSHWSDB_DB_NAME, kPUSHWSDB_DB_VERSION, kPUSHWSDB_STORE_NAME, upgradeSchemaWS); wrap to 80 chars.

https://reviewboard.mozilla.org/r/8903/#review7999 ::: dom/push/PushService.jsm:101 (Diff revision 2) > + // register/unregister requests. Please add a comment about what keys map to what values. Also, it doesn't seem to handle unregister ::: dom/push/PushService.jsm:754 (Diff revision 2) > + Promise.reject({state: 0, error: "Service not active"}); Is this promise being rejected to just log the error? Otherwise you probably want to return it or just use debug(). ::: dom/push/PushService.jsm:149 (Diff revision 2) > + if ((this._activated) || > + (channelIDs.length > 0)) { > + // if there are request waiting > + debug("this._state: " + this._state); Could you add a comment about in what conditions we want to reconnect? Before, we only checked for channelIDS.length > 0. Now this will connect if this._activated is a non-null promise but there are no registered channels. Do you want &&? Also, "if there is a request waiting", how is that being checked? ::: dom/push/PushService.jsm:165 (Diff revision 2) > + debug("this._state: " + this._state); Why is it useful to log this in this specific case vs. always logging the state? Perhaps improve the debug message. ::: dom/push/PushService.jsm:229 (Diff revision 2) > this._db.getByScope(scope) > .then(record => > - this._db.delete(records.channelID) > - .then(_ => > + this._db.delete(record.channelID) > + .then(_ => this._sendRequest("unregister", record), > - this._service.unregister(records), > err => { > debug("webapps-clear-data: " + scope + > - " Could not delete entry " + records.channelID); > + " Could not delete entry " + record.channelID); > > - this._service.unregister(records) > - throw "Database error"; > - }) > + this._sendRequest("unregister", record); > + throw err; > + }), _ => { > - , _ => { > debug("webapps-clear-data: Error in getByScope(" + scope + ")"); > }); The indentation makes this hard to read. Replacing the two arg then() with catch(), and liberal use of {} would help. Also, no need to rethrow the error, and we can swapt the then() and catch() in the inner promise since we always want to sendRequest(): this._db.getByScope(scope) .then(record => { this._db.delete(...) .catch(_ => { debug(...); // Since this function returns undefined, the next (implicit) Promise in the chain is resolved, so the then block always runs and we always sendRequest. }).then(_ => sendRequest(...)) }).catch(_ => { debug(...); }); ::: dom/push/PushService.jsm:299 (Diff revision 2) > + // The new serverUri is empty of misconfigured - stop service. Nit: s/of/or ::: dom/push/PushService.jsm:603 (Diff revision 2) > + this._notifyApp(aPushRecord, message); notifyApp should block on updatePushRecord. ::: dom/push/PushService.jsm:808 (Diff revision 2) > return Promise.reject("NotFoundError"); Please reject with the same format as the other rejections.

https://reviewboard.mozilla.org/r/8907/#review8005 Ship It!

https://reviewboard.mozilla.org/r/8903/#review8073 > Please add a comment about what keys map to what values. > > Also, it doesn't seem to handle unregister you are rigth, it does not queue unregister. > Could you add a comment about in what conditions we want to reconnect? Before, we only checked for channelIDS.length > 0. Now this will connect if this._activated is a non-null promise but there are no registered channels. Do you want &&? > > Also, "if there is a request waiting", how is that being checked? It used to check the request queue. Now this part is changed a bit. I will drop this._activated it is not that important. In this way it does not work exactly what I wanted. > Why is it useful to log this in this specific case vs. always logging the state? Perhaps improve the debug message. This logging is not needed.

https://reviewboard.mozilla.org/r/8901/#review8071 > wrap to 80 chars. This is change in the following part of these patches.

https://reviewboard.mozilla.org/r/8899/#review8069 > What is this catching? If it's just Date.parse(), then only capture that. It is guarding aRequest.getResponseHeader("retry-after").

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana /r/8901 - Bug 1150812 - split PushService - separate webSocket part. r=nsm, r=mt /r/8903 - Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt /r/8905 - Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt /r/8907 - Bug 1150812 - Add Http2 Push service. r=nsm, r=mt /r/8909 - Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Pull down these commits: hg pull -r 0c4090909b1be678e8aa839dbff48d7c6607d3e7 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana https://reviewboard.mozilla.org/r/8899/#review8195 ::: dom/push/PushService.jsm:229 (Diff revisions 4 - 5) > this._db.delete(this._service.getKeyFromRecord(record)) If you return this here, then you don't need the second (inner) catch block. Well, you do if you want to ensure that the unregister request is sent, but you could instead change the structure to: this._db.getByScope(scope) .then(record => Promise.all([ this._db.delete(this._service.getKeyFromRecord(record)), this._sendRequest("unregister", record) ]) ).catch(_ => debug("webapps-clear-data: Error in cleanup(" + scope + ")")); ::: dom/push/PushService.jsm:471 (Diff revisions 4 - 5) > kCHILD_PROCESS_MESSAGES.forEach(msgName => > - ppmm.removeMessageListener(msgName, this) > + ppmm.removeMessageListener(msgName, > + this) > ); Perhaps kCHILD_PROCESS_MESSAGES.forEach( msgName => ppmm.removeMessageListener(msgName, this) ); ::: dom/push/PushService.jsm:787 (Diff revisions 4 - 5) > if (this._state < PUSH_SERVICE_ACTIVATING) { > dump(" This should not happen because we do not listen to the messages" + > " in PUSH_SERVICE_UNINIT or PUSH_SERVICE_INIT state.\n"); > - Promise.reject({state: 0, error: "Service not active"}); > + // This will reply to the message with an error. > + this[aMessage.name.slice("Push:".length).toLowerCase()](json, mm); > + return; > } Rather than include the function invocation twice, how about you do this: let activated = Promise.resolve(); if (this.state >= PUSH_SERVICE_ACTIVATING) { dump("warning...."); activated = this._activated; } this[aMessage.name.slice...](json, mm); ::: dom/push/PushServiceHttp2.jsm:605 (Diff revisions 4 - 5) > - if (retryAfter == -1) { > + if (retryAfter != -1) { !== ::: dom/push/PushServiceHttp2.jsm:622 (Diff revisions 4 - 5) > - setTimeout(this._listenForMsgs(aSubscriptionUri), 0); > + setTimeout(this._listenForMsgs.bind(this, aSubscriptionUri), 0); setTimeout(_ => this._listenForMsgs(aSubscriptionUri), 0) or Promise.resolve().then(_ => this._listen....) ::: dom/push/PushServiceHttp2.jsm:771 (Diff revisions 4 - 5) > - setTimeout(this._listenForMsgs(aSubscriptionUri), 0); > + setTimeout(this._listenForMsgs.bind(this, aSubscriptionUri), 0); See above

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana /r/8901 - Bug 1150812 - split PushService - separate webSocket part. r=nsm, r=mt /r/8903 - Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt /r/8905 - Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt /r/8907 - Bug 1150812 - Add Http2 Push service. r=nsm, r=mt /r/8909 - Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Pull down these commits: hg pull -r 6d4a8252bcbd7cb713fbf5803c73584e4330942a https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8607020 [details] MozReview Request: bz://1150812/dragana https://reviewboard.mozilla.org/r/8899/#review8253 We could keep going around on this. I'm pretty sure that the more that I look at this code, the more I will want to suggest changes. Let's just land it and we can deal with improvements later. Unless you have plans to abandon this once it lands, that is. ::: dom/push/PushServiceHttp2.jsm:605 (Diff revisions 5 - 6) > - if (retryAfter != -1) { > + if (retryAfter !== -1) { > // This is a 5xx response. > this._conns[aSubscriptionUri].countUnableToConnect++; > - setTimeout(this._listenForMsgs.bind(this, aSubscriptionUri), retryAfter); > + setTimeout(_ => this._listenForMsgs(aSubscriptionUri), retryAfter); > return; > } Looking at this again, why use setTimeout here and the alarm below? (I've never been clear on the merits of using the alarm, but this if it's worth using, it should be used.) I think that I'd like to see this code path unified with that below: i.e., if (retryAfter === -1) { retryAfter = pref.get('http2.retryInterval') * ... } ...countUnableToConnect++; ::: dom/push/PushService.jsm:885 (Diff revisions 5 - 6) > - return this._db.clearAll(); > + if (this._state < PUSH_SERVICE_ACTIVATING) { > + return Promise.resolve(); > + } > + return this._activated > + .then(_ => { > + if (this._state < PUSH_SERVICE_ACTIVATING) { > + return Promise.resolve(); > + } > + this._db.clearAll(); > + }); I wonder if you can't change the value of `this._activated` to encompass the logic that you have just repeated across all of these functions. This case is the only one where the promise needs to be resolved instead of rejected as a result of the state being incorrect. You can add a call to .catch() here. The change I'd make is to assign `_activated` a value initially. Then reassign the value each time the service is stopped. (The more I stare at these states, the more I find them confusing. Using X.731 and its interconnected states might have been a better design choice, but that's too late now.)

https://reviewboard.mozilla.org/r/8905/#review8267 Ship It!

https://reviewboard.mozilla.org/r/8899/#review8391 > Looking at this again, why use setTimeout here and the alarm below? (I've never been clear on the merits of using the alarm, but this if it's worth using, it should be used.) > > I think that I'd like to see this code path unified with that below: > > i.e., > if (retryAfter === -1) { > retryAfter = pref.get('http2.retryInterval') * ... > } > ...countUnableToConnect++; Alarm is cumulative alarm, if there are more setAlram called the last delay value will be used (that is why the alarm is used not to wake phone up too often), so I wanted to respect 5xx responses correctly. > I wonder if you can't change the value of `this._activated` to encompass the logic that you have just repeated across all of these functions. > > This case is the only one where the promise needs to be resolved instead of rejected as a result of the state being incorrect. You can add a call to .catch() here. > > The change I'd make is to assign `_activated` a value initially. Then reassign the value each time the service is stopped. > > (The more I stare at these states, the more I find them confusing. Using X.731 and its interconnected states might have been a better design choice, but that's too late now.) I have change it a bit. The promise will be rejected if the activation doesn't succeed. Simple assigning _activated a value does not work - sanity check complains if reject is not caught. This code would probably look different if I hadn't merged SimplePush and WebPush. I really didn't want to change SimplePush too much, not to break already running code (SimplePush is more complex than WebPush). I do not have time not to do big rewriting, like changing states. I do not mind if you have time to change it.

Bug 1150812 - split PushService - make the separation more common for webSocket and http2. r=nsm, r=mt

Bug 1150812 - adapt xpcshell test and add mochi tests for WebSocket version. r=nsm, r=mt

Bug 1150812 - Add Http2 Push service. r=nsm, r=mt

Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt

(In reply to Dragana Damjanovic [:dragana] from comment #58) > Alarm is cumulative alarm, if there are more setAlram called the last delay > value will be used (that is why the alarm is used not to wake phone up too > often), so I wanted to respect 5xx responses correctly. Please add a comment to that effect. > This code would probably look different if I hadn't merged SimplePush and > WebPush. I really didn't want to change SimplePush too much, not to break > already running code (SimplePush is more complex than WebPush). I do not > have time not to do big rewriting, like changing states. I do not mind if > you have time to change it. Yeah, I get that.

Please stop flagging me for review here and land the code.

Comment on attachment 8612821 [details] MozReview Request: Bug 1150812 - Add Http2 Push service. r=nsm, r=mt Bug 1150812 - Add Http2 Push service. r=nsm, r=mt

Comment on attachment 8612822 [details] MozReview Request: Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt Bug 1150812 - xcpshell test for PushService with http2. r=nsm, r=mt

Just uploaded final patches that needs to be checked in: part0,...part4

Part 0 needs rebasing.

rebased

rebased

Part 0 is hitting conflicts again :(. This must be prone to bitrotting quickly :( patching file dom/push/PushService.jsm Hunk #4 FAILED at 245 1 out of 10 hunks FAILED -- saving rejects to file dom/push/PushService.jsm.rej Ping me on IRC when you rebase and I'll try to get it landed ASAP before it bitrots again. Sorry for the hassle :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9707252997d https://hg.mozilla.org/integration/mozilla-inbound/rev/4d17715232f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/036d9abeb00c https://hg.mozilla.org/integration/mozilla-inbound/rev/db9050215b8a https://hg.mozilla.org/integration/mozilla-inbound/rev/4c897e262cd5

https://hg.mozilla.org/mozilla-central/rev/d9707252997d https://hg.mozilla.org/mozilla-central/rev/4d17715232f5 https://hg.mozilla.org/mozilla-central/rev/036d9abeb00c https://hg.mozilla.org/mozilla-central/rev/db9050215b8a https://hg.mozilla.org/mozilla-central/rev/4c897e262cd5