In bug 822712, we landed support for push notifications in B2G applications. We should extend this to non-manifest-based web pages. I think, one approach, would be to allow web content to specify, in register, what callback or wakeup url they would like pinged back. For example navigator.push.register("https://some.tld/push") When a user agent sees a push notification event from the push server, it can load (in a shared worker, or sandbox) the url specified by the register call. For example, see: https://dl.dropbox.com/u/8727858/push.html https://dl.dropbox.com/u/8727858/push-callback.html

Comment on attachment 732687 [details] [diff] [review] patch v.1 nsm/justin: could you provide feedback on the API change and use of the sandbox that identity is using. It would be nice to use this while we work on its replacement.

nsm, can you upload your rebased patch?

Comment on attachment 732687 [details] [diff] [review] patch v.1 Does the sandbox ever go away? Refreshing the sandbox when we get a new wakeup seems non-ideal, since presumably that will wipe out whatever the code is trying to do at the time.

> Does the sandbox ever go away? no. we should fix that. maybe kill it based on some ideal timer? > Refreshing the sandbox when we get a new wakeup seems non-ideal, since presumably that will wipe out whatever the code is trying to do at the time. Right. That is wrong. We should probably just dispatch the system event instead of reloading.

What's the status of this patch? I tried to apply it to see if it could give me a better idea of how things could work in a desktop context, but I didn't go very far. var request = navigator.push.register(); is executed without causing an exception (should it throw when omitting the wakeupURL parameter when not in an app context?), but neither onsuccess nor onerror is ever called. Turning on debug logging in Push.js and PushService.js showed that PushService.js: init() was never called. After some more poking at the code, it turns out this is because the user preferences from the profile haven't been read yet when the "app-startup" notification is fired, so enabling from about:config doesn't work. I fixed this by editing the code to add the "final-ui-startup" observer unconditionally, and check the pref when "final-ui-startup" is fired. After getting this actually enabled, still no call to onsuccess or onerror. The debug info printed in my terminal looked like wss://simple-push.com:9999/ doesn't accept connections, telnet simple-push.com 9999 confirmed it. Is there another test server I could have used? By the way, a red herring while trying this: When I execute navigator.push.register() in the Error Console, I get: JavaScript error: file:///.../components/Push.js, line 127: TypeError: this._pageURL is null I guess this is because document.nodePrincipal.URI is null when running in the Error Console, but it confused me. There may be a way to fail more gracefully in this case.

@flo - simple-push probably was killed during media temple's migration last night. I'll take a look later today. Please try again later. :)

http://nikhilism.com:9082 is usually up all the time.

This patch unifies the app and web page APIs by shimming system messages into the sandbox without modifying system messages code too much. Here is how it works: 1) URL passed to register() is saved by Push as wakeupURL based on dougt's original patch. 2) SystemMessageManager is modified to use the wakeupURL origin (because that page calls mozSetMessageHandler) as the 'manifest' in its handlers list. This is fine because push.register() verifies that wakeupURL is in the same origin as the page that calls register(). 3) When a push is delivered on the websocket, we registerPage() with SystemMessageInternal just before dispatching the push message so that the system messages service knows which page to deliver to. The code also enables push permission on that page. Fabrice: Do you think the approach will break existing system messages in any way? Test app running at: http://nikhilm.github.io/simplepush-test-app/

Forgot to mention that to see the test app running, you'll want to open the web developer console, where it will log the time of the last push every 5 seconds. Register and push to an endpoint to update this.

Earlier patch was only system messages and not sandbox. This one is both folded into one.

Update which fixes: Webpages in the same origin can all see each others registrations(). This makes sense because all pages in an app can also do the same. Operationally, a push handler page may want to consult the registrations() list made by the push registration page. Unregister() is also restricted by origin for web pages. manifestURLs are still maintained as null in IDB for pages. That is the test for whether an entry is by a page or by an app.

pingInterval is handled by nsITimer on desktop since AlarmService doesn't work and we don't worry about 'sleep mode'.

Comment on attachment 744173 [details] [diff] [review] Sandbox + system messages Review of attachment 744173 [details] [diff] [review]: ----------------------------------------------------------------- olli should also review. ::: browser/app/profile/firefox.js @@ +1243,5 @@ > +// SimplePush > +pref("services.push.enabled", true); > +// serverURL to be assigned by services team > +pref("services.push.serverURL", ""); > +pref("services.push.userAgentID", ""); Use the mozilla serverURL. set services.push.enabled to false in the nightly for now. ::: dom/interfaces/push/nsIDOMPushManager.idl @@ +17,1 @@ > interface nsIDOMPushManager : nsISupports uuid change. ::: dom/messages/SystemMessageManager.js @@ +202,5 @@ > .getService(Ci.nsIAppsService); > this._manifest = appsService.getManifestURLByLocalId(principal.appId); > + if (!this._manifest) { > + // principal.origin doesn't have the trailing slash, so normalize the > + // URL. i don't understand this comment. ::: toolkit/xre/nsAppRunner.cpp @@ +2985,5 @@ > Output(true, "Error: Platform version '%s' is not compatible with\n" > "minVersion >= %s\nmaxVersion <= %s\n", > gToolkitVersion, > mAppData->minVersion, mAppData->maxVersion); > + // return 1; kill this change.

Comment on attachment 744175 [details] [diff] [review] Use nsITimer instead of AlarmService in non b2g builds. Review of attachment 744175 [details] [diff] [review]: ----------------------------------------------------------------- holy ifdefs. How about implementing AlarmService in terms of nsITimer? Is that a ton of work?

(In reply to Doug Turner (:dougt) from comment #14) > > ::: dom/messages/SystemMessageManager.js > @@ +202,5 @@ > > .getService(Ci.nsIAppsService); > > this._manifest = appsService.getManifestURLByLocalId(principal.appId); > > + if (!this._manifest) { > > + // principal.origin doesn't have the trailing slash, so normalize the > > + // URL. > > i don't understand this comment. Well the origin here and the origin derived from getNoAppCodebasePrincipal() in PushService, differ by a trailing slash, which leads to the message delivery not working, so I fixed it in one of the places. I'll update the comment in the patch.

(In reply to Doug Turner (:dougt) from comment #15) > Comment on attachment 744175 [details] [diff] [review] > Use nsITimer instead of AlarmService in non b2g builds. > > Review of attachment 744175 [details] [diff] [review]: > ----------------------------------------------------------------- > > holy ifdefs. How about implementing AlarmService in terms of nsITimer? Is > that a ton of work? Yes, AlarmService relies on AlarmHalService which relies on Hal. So I'd have to write a nsITimer based Hal implementation where I believe XPCOM access isn't allowed.

> So I'd have to write a nsITimer based Hal implementation where I believe XPCOM access isn't allowed. As the current module owner of hal, I hereby give you permission to write a fallback implementation of the alarm service in hal using nsITimers, if we can verify that long-running nsITimers don't cause battery problems on laptops or Android phones. I never encountered the restriction against using XPCOM in hal myself, but I expect that the goal was to keep hal as "dumb" as possible, and to keep application logic out of hal. I think implementing a fallback alarm service in terms of nsITimers keeps with that spirit.

Comment on attachment 744173 [details] [diff] [review] Sandbox + system messages Review of attachment 744173 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +4262,5 @@ > MOZ_WEBSMS_BACKEND= > MOZ_ANDROID_WALLPAPER= > MOZ_ANDROID_BEAM= > ACCESSIBILITY=1 > +MOZ_SYS_MSG=1 We removed MOZ_SYS_MSG in bug 861496 and replaced it by a pref. ::: dom/push/src/PushService.jsm @@ +18,5 @@ > Cu.import("resource://gre/modules/IndexedDBHelper.jsm"); > Cu.import("resource://gre/modules/Timer.jsm"); > Cu.import("resource://gre/modules/Preferences.jsm"); > Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); > +Cu.import("resource://gre/modules/identity/Sandbox.jsm"); That looks strange to depend on something from /identity. Maybe Sandbox.jsm should be moved to toolkit or something. @@ +69,5 @@ > // associated with an app. Since an app can have multiple endpoints > // uniqueness cannot be enforced > objectStore.createIndex("manifestURL", "manifestURL", { unique: false }); > + > + // index to fetch records per page URL. Nit: Index (and check other comments also) @@ +1042,5 @@ > + let manifestURI = Services.io.newURI(principal.origin, null, null); > + > + let messenger = Cc["@mozilla.org/system-message-internal;1"] > + .getService(Ci.nsISystemMessagesInternal); > + Use a lazy service getter, you're doing that twice. @@ +1057,5 @@ > + Services.perms.add(wakeupURI, 'push', Ci.nsIPermissionManager.ALLOW_ACTION, > + Ci.nsIPermissionManager.EXPIRE_SESSION); > + > + // Register the page so that system messages can perform delivery. > + messenger.registerPage('push', wakeupURI, manifestURI); Do you need to unregister it after you send the message? If yes, we'll need to add that to SystemMessageInternal.

Comment on attachment 744173 [details] [diff] [review] Sandbox + system messages Sorry, but I don't know what I could review here. I'm not a peer for toolkit stuff, like Sandbox.jsm and I have no idea what SystemMessageManager.js is. It does use MessageManager API internally, and I'm familiar with that, but what SystemMessageManager is about, I have no idea. If I need to review this, I'd have to understand first what Sandbox.jsm and SystemMessageManager.js are about. Currently I know nothing about those. Perhaps mounir knows about this stuff?

Comment on attachment 744173 [details] [diff] [review] Sandbox + system messages Review of attachment 744173 [details] [diff] [review]: ----------------------------------------------------------------- r- because there are a few design decisions that I would like to discuss and some bits of code that need to be fixed. We might want someone else to review the Sandbox usage. ::: browser/app/profile/firefox.js @@ +1240,5 @@ > // before content. > pref("dom.debug.propagate_gesture_events_through_content", false); > + > +// SimplePush > +pref("services.push.enabled", true); Could you do something like: #ifdef RELEASE_BUILD pref("services.push.enabled", false); #else pref("services.push.enabled", true); #endif ::: dom/messages/SystemMessageManager.js @@ +203,5 @@ > this._manifest = appsService.getManifestURLByLocalId(principal.appId); > + if (!this._manifest) { > + // principal.origin doesn't have the trailing slash, so normalize the > + // URL. > + this._manifest = Services.io.newURI(principal.origin, null, null).spec; Why are you doing that? ::: dom/push/src/Push.js @@ +50,5 @@ > .getService(Ci.nsIAppsService); > this._app = appsService.getAppByLocalId(principal.appId); > this._manifestURL = appsService.getManifestURLByLocalId(principal.appId); > + // if its an app, test for permission > + if (this._manifestURL) { You should verify if something is an app by checking if appId != NO_APP_ID. If appId != NO_APP_ID, _manifestURL can't be the empty string. @@ +56,5 @@ > + "push"); > + if (perm != Ci.nsIPermissionManager.ALLOW_ACTION) > + return null; > + } > + else { nit: our coding style usually looks like that: } else { @@ +129,5 @@ > + > + if (this._isWebPage) { > + // TODO: Prompt for permission in case of web page. > + if (!aPushWakeupUrl) { > + throw "URI required as argument to navigator.push.register()"; You should throw a NS_ERROR_INVALID_ARG, not this. Also, it is always nice to have a comprehensive default behaviour. Why not simply take aPushWakeupUrl = current document, instead? @@ +137,5 @@ > + let wakeupURI = null; > + try { > + wakeupURI = Services.io.newURI(aPushWakeupUrl, null, null); > + } catch(e) { > + throw "Invalid URL: " + aPushWakeupUrl; ditto Also, it would be very appreciated by developers if they could pass a relative URL here. You could make that happen by passing a baseURI I believe. @@ +143,5 @@ > + > + try { > + Services.scriptSecurityManager.checkSameOriginURI(originURI, > + wakeupURI, > + true); Do you really want to report this error? @@ +145,5 @@ > + Services.scriptSecurityManager.checkSameOriginURI(originURI, > + wakeupURI, > + true); > + } catch(e) { > + throw "Origin mismatch"; You should probably throw a NS_ERROR_DOM_SECURITY_ERR. ::: dom/push/src/PushService.jsm @@ +18,5 @@ > Cu.import("resource://gre/modules/IndexedDBHelper.jsm"); > Cu.import("resource://gre/modules/Timer.jsm"); > Cu.import("resource://gre/modules/Preferences.jsm"); > Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js"); > +Cu.import("resource://gre/modules/identity/Sandbox.jsm"); Importing something from identity might look weird but the line above is importing something from addon-sdk ;) @@ +211,5 @@ > + function txnCb(aTxn, aStore) { > + aTxn.result = []; > + aStore.openCursor().onsuccess = function(event) { > + var cursor = event.target.result; > + if (cursor) { nit: I would do the following the gain in lisibility: if (!cursor) { return; } That way, you win a level of indentation. @@ +223,5 @@ > + let recordPageURI = Services.io.newURI(cursor.value.pageURL, null, null); > + > + Services.scriptSecurityManager.checkSameOriginURI(pageURI, > + recordPageURI, > + true); Do you really want to report this error? @@ +1038,5 @@ > + // The origin has been verified during register(), so we just derive the > + // manifest URL from it. > + let principal = Services.scriptSecurityManager > + .getNoAppCodebasePrincipal(wakeupURI); > + let manifestURI = Services.io.newURI(principal.origin, null, null); Why are you faking a manifestURI? that sounds pretty hacky. @@ +1062,5 @@ > + > + let s = new Sandbox(aPushRecord.wakeupURL, function(e) { > + messenger.sendMessage('push', message, wakeupURI, manifestURI); > + }); > + this._sandboxes[aPushRecord.wakeupURL] = s; FWIW, I'm not familiar with sandboxes. @@ +1283,2 @@ > return; > } I think you can quite simplify this code: if (record.manifestURL != aPageRecord.manifestURL) { fackeSuccess(); return; } if (!record.manifestURL) { let recordPageURI = Services.io.newURI(record.pageURL, null, null); let callerPageURI = Services.io.newURI(aPageRecord.pageURL, null, null); try { Services.scriptSecurityManager.checkSameOriginURI(callerPageURI, recordPageURI, false); } catch (e) { fakeSuccess(); return; } } Also, I'm not sure that faking success is a good idea. Being clear that there is a CORS problem wouldn't hurt I believe.

Comment on attachment 744175 [details] [diff] [review] Use nsITimer instead of AlarmService in non b2g builds. Review of attachment 744175 [details] [diff] [review]: ----------------------------------------------------------------- r-. we are going to make a nsITimer based AlarmService.

Comment on attachment 744175 [details] [diff] [review] Use nsITimer instead of AlarmService in non b2g builds. Review of attachment 744175 [details] [diff] [review]: ----------------------------------------------------------------- Nikhil, this nsITimer patch does not apply cleanly over your Sandbox patch. These patches assume PushService.jsm has some AlarmService calls that are not in mozilla-central. Am I missing a prerequisite patch or are you working in a different tree?

Comment on attachment 744175 [details] [diff] [review] Use nsITimer instead of AlarmService in non b2g builds. Obsoleted by Bug 867868.

Fixes issues and nits raised by Fabrice and Mounir. I've kept the Sandbox in identity for now, it would be cleaner to file a separate bug to move it into toolkit/modules. I've also moved prefs into a separate attachment with RELEASE_BUILD guards.

Preferences.

Pass on the absolute URI derived from newURI to the service, otherwise relative URLs will fail on push notification. Remove this._origin, this._pageURI is enough.

Comment on attachment 747914 [details] [diff] [review] Preferences Review of attachment 747914 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer to have those prefs being called "dom.push.*" but I will not block this patch for that ;)

Comment on attachment 748378 [details] [diff] [review] Sandbox + system messages Review of attachment 748378 [details] [diff] [review]: ----------------------------------------------------------------- I would be interested to know more about the system message hack and why it has been preferred against adding non-app support to system messages. I'm worried that as long as the code [of system message] is written with the app context in mind, this hack is going to be terribly weak. ::: dom/messages/SystemMessageManager.js @@ +201,5 @@ > + if (principal.appId == Ci.nsIScriptSecurityManager.NO_APP_ID) { > + // principal.origin doesn't have the trailing slash (http://example.com) > + // while a URI.spec does. Since the SystemMessageInternal API expects > + // URIs from callers, obtain a origin with a trailing slash. > + this._manifest = Services.io.newURI(principal.origin, null, null).spec; I'm not a big fan of that hack. It seems more dangerous than rewriting the system message code to make it assume that the manifest url can be the empty string. More or less how you handled this for Push.js. ::: dom/push/src/Push.js @@ +131,5 @@ > + debug("register() wakeup url: " + aPushWakeupUrl); > + let req = this.createRequest(); > + > + if (this._isWebPage) { > + // TODO: Prompt for permission in case of web page. Please, open a bug and add the number here. @@ +147,5 @@ > + } > + > + Services.scriptSecurityManager.checkSameOriginURI(this._pageURI, > + wakeupURI, > + true); I think there was a confusion regarding my previous comment. I was wondering if you really intended to pass 'true' as a last argument. You should definitely throw if checkSameOriginURI() fails. After re-thinking, passing true is probably the right thing so it should look like: try { Services.scriptSecurityManager.checkSameOriginURI(this._pageURI, wakeupURI, true); } catch (e) { throw Components.results.NS_ERROR_DOM_SECURITY_ERR; } ::: dom/push/src/PushService.jsm @@ +202,5 @@ > + getAllByOrigin: function(aPageURL, aSuccessCb, aErrorCb) { > + debug("getAllByOrigin() " + aPageURL); > + if (!aPageURL) { > + if (typeof aErrorCb == "function") { > + aErrorCb("PushDB.getAllByOrigin: Got undefined aPageURL"); It's not clear to me what this message is for. Isn't this going to call _onRegistrationsError() ultimately? How is this trying being used there? By concern is that we shouldn't have that kind of stuff in DOMError.name. @@ -488,5 @@ > - > - // At this point, profile-change-net-teardown has already fired, so the > - // WebSocket has been closed with NS_ERROR_ABORT (if it was up) and will > - // try to reconnect. Stop the timer. > - this._stopAlarm(); Why are you removing this code? @@ +1078,5 @@ > + } > + > + // Add push permissions to the wakeup URL. > + Services.perms.add(wakeupURI, 'push', Ci.nsIPermissionManager.ALLOW_ACTION, > + Ci.nsIPermissionManager.EXPIRE_SESSION); I would prefer if you remove this. If you really need that for debugging, add it locally but I don't think it's fine to allow any web page to have a permission that we don't even give to every applications. @@ +1268,3 @@ > this._db.getByPushEndpoint(aPageRecord.pushEndpoint, function(record) { > + if (!record) { > + fail("Non-existent push endpoint"); sic... The purpose of DOMError isn't to have .name being that kind of strings. See: http://dom.spec.whatwg.org/#interface-domerror It's terrible that we got this habit to send an error string through IPC without knowing what's going to happen to it :( @@ +1274,5 @@ > + // Checking that a manifestURL exists in the record but not in the caller > + // ensures that a normal web page on the same domain as a hosted app > + // cannot delete the hosted app's endpoints. > + if (record.manifestURL && record.manifestURL !== aPageRecord.manifestURL) { > + fakeSuccess(); I think you should return a "SecurityError" instead of faking a success. @@ +1279,5 @@ > + return; > + } > + // If endpoint was created by a web page, only allow pages with the same > + // origin permission to delete the endpoint. > + else if (!record.manifestURL) { No "else" is needed here, the 'if' statement returns if it succeeded. @@ +1288,5 @@ > + " is trying to delete endpoint set by " + recordPageURI.spec); > + try { > + Services.scriptSecurityManager.checkSameOriginURI(callerPageURI, > + recordPageURI, > + false); Maybe 'true' would actually be appropriate here. @@ +1290,5 @@ > + Services.scriptSecurityManager.checkSameOriginURI(callerPageURI, > + recordPageURI, > + false); > + } catch(e) { > + fakeSuccess(); ditto

Nikhil, feedback? This patch (to your patch) renames `PushWebSocketListener` to `PushSocket` and encapsulates all WebSocket calls. The `PushService` object currently maintains state about WebSocket, PushDB, and current registrations. I'd like to separate some of these dependencies because our Android background service will need a different WebSocket or GCM channel.

(In reply to Chris Peterson (:cpeterson) from comment #31) > Created attachment 749580 [details] [diff] [review] > encapsulate-WebSocket-in-PushSocket.patch > > Nikhil, feedback? This patch (to your patch) renames `PushWebSocketListener` > to `PushSocket` and encapsulates all WebSocket calls. The `PushService` > object currently maintains state about WebSocket, PushDB, and current > registrations. I'd like to separate some of these dependencies because our > Android background service will need a different WebSocket or GCM channel. I like the direction of this patch. So the Android implementation will have it's own AndroidPushSocket or will it be a AndroidPushService using the current PushSocket? I didn't understand why PushDB has to be custom on Android. Or did you mean that PushDB should go into its own file?

(In reply to Nikhil Marathe from comment #32) > I like the direction of this patch. So the Android implementation will have > it's own AndroidPushSocket or will it be a AndroidPushService using the > current PushSocket? I'm hoping to use something like a AndroidPushSocket or events from PushService to drive a Java background service's WebSocket. > I didn't understand why PushDB has to be custom on Android. Or did you mean > that PushDB should go into its own file? Sorry, I was suggesting that we isolate WebSockets from PushDB so Android could use PushDB without PushService's WebSockets.

(In reply to Mounir Lamouri (:mounir) from comment #30) > Comment on attachment 748378 [details] [diff] [review] > Sandbox + system messages > > Review of attachment 748378 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would be interested to know more about the system message hack and why it > has been preferred against adding non-app support to system messages. I'm > worried that as long as the code [of system message] is written with the app > context in mind, this hack is going to be terribly weak. > > ::: dom/messages/SystemMessageManager.js > @@ +201,5 @@ > > + if (principal.appId == Ci.nsIScriptSecurityManager.NO_APP_ID) { > > + // principal.origin doesn't have the trailing slash (http://example.com) > > + // while a URI.spec does. Since the SystemMessageInternal API expects > > + // URIs from callers, obtain a origin with a trailing slash. > > + this._manifest = Services.io.newURI(principal.origin, null, null).spec; > > I'm not a big fan of that hack. It seems more dangerous than rewriting the > system message code to make it assume that the manifest url can be the empty > string. More or less how you handled this for Push.js. I'd like a discussion on how we can get System Messages to websites. I'm sure I'm missing some of the context. Let's continue on the mozilla.dev.webapi thread and see how this plays out. > > @@ +147,5 @@ > > + } > > + > > + Services.scriptSecurityManager.checkSameOriginURI(this._pageURI, > > + wakeupURI, > > + true); > > I think there was a confusion regarding my previous comment. I was wondering > if you really intended to pass 'true' as a last argument. You should > definitely throw if checkSameOriginURI() fails. After re-thinking, passing > true is probably the right thing so it should look like: > try { > Services.scriptSecurityManager.checkSameOriginURI(this._pageURI, > wakeupURI, true); > } catch (e) { > throw Components.results.NS_ERROR_DOM_SECURITY_ERR; > } I disagree with catching and then throwing again. 1) The exception throw by checkSameOriginURI is quite descriptive 2) Cr.NS_ERROR_DOM* are not accessible from JS. > > ::: dom/push/src/PushService.jsm > @@ +202,5 @@ > > + getAllByOrigin: function(aPageURL, aSuccessCb, aErrorCb) { > > + debug("getAllByOrigin() " + aPageURL); > > + if (!aPageURL) { > > + if (typeof aErrorCb == "function") { > > + aErrorCb("PushDB.getAllByOrigin: Got undefined aPageURL"); > > It's not clear to me what this message is for. Isn't this going to call > _onRegistrationsError() ultimately? How is this trying being used there? By > concern is that we shouldn't have that kind of stuff in DOMError.name. I'm sorry, I didn't know that DOMRequest.fireError only supported name and not a description. > > @@ -488,5 @@ > > - > > - // At this point, profile-change-net-teardown has already fired, so the > > - // WebSocket has been closed with NS_ERROR_ABORT (if it was up) and will > > - // try to reconnect. Stop the timer. > > - this._stopAlarm(); > > Why are you removing this code? Wrong patch. > > @@ +1078,5 @@ > > + } > > + > > + // Add push permissions to the wakeup URL. > > + Services.perms.add(wakeupURI, 'push', Ci.nsIPermissionManager.ALLOW_ACTION, > > + Ci.nsIPermissionManager.EXPIRE_SESSION); > > I would prefer if you remove this. If you really need that for debugging, > add it locally but I don't think it's fine to allow any web page to have a > permission that we don't even give to every applications. Again this is required for system messages and not for Push. So I'll wait for the system messages story. > > @@ +1274,5 @@ > > + // Checking that a manifestURL exists in the record but not in the caller > > + // ensures that a normal web page on the same domain as a hosted app > > + // cannot delete the hosted app's endpoints. > > + if (record.manifestURL && record.manifestURL !== aPageRecord.manifestURL) { > > + fakeSuccess(); > > I think you should return a "SecurityError" instead of faking a success. Ok.

(In reply to Nikhil Marathe from comment #34) > (In reply to Mounir Lamouri (:mounir) from comment #30) > > Comment on attachment 748378 [details] [diff] [review] > > Sandbox + system messages > > > > Review of attachment 748378 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I would be interested to know more about the system message hack and why it > > has been preferred against adding non-app support to system messages. I'm > > worried that as long as the code [of system message] is written with the app > > context in mind, this hack is going to be terribly weak. > > > > ::: dom/messages/SystemMessageManager.js > > @@ +201,5 @@ > > > + if (principal.appId == Ci.nsIScriptSecurityManager.NO_APP_ID) { > > > + // principal.origin doesn't have the trailing slash (http://example.com) > > > + // while a URI.spec does. Since the SystemMessageInternal API expects > > > + // URIs from callers, obtain a origin with a trailing slash. > > > + this._manifest = Services.io.newURI(principal.origin, null, null).spec; > > > > I'm not a big fan of that hack. It seems more dangerous than rewriting the > > system message code to make it assume that the manifest url can be the empty > > string. More or less how you handled this for Push.js. > > I'd like a discussion on how we can get System Messages to websites. I'm > sure I'm missing some of the context. Let's continue on the > mozilla.dev.webapi thread and see how this plays out. Sure, feel free to get the discussion started there. > > @@ +147,5 @@ > > > + } > > > + > > > + Services.scriptSecurityManager.checkSameOriginURI(this._pageURI, > > > + wakeupURI, > > > + true); > > > > I think there was a confusion regarding my previous comment. I was wondering > > if you really intended to pass 'true' as a last argument. You should > > definitely throw if checkSameOriginURI() fails. After re-thinking, passing > > true is probably the right thing so it should look like: > > try { > > Services.scriptSecurityManager.checkSameOriginURI(this._pageURI, > > wakeupURI, true); > > } catch (e) { > > throw Components.results.NS_ERROR_DOM_SECURITY_ERR; > > } > > I disagree with catching and then throwing again. 1) The exception throw by > checkSameOriginURI is quite descriptive 2) Cr.NS_ERROR_DOM* are not > accessible from JS. Isn't |throw Components.results.NS_ERROR_DOM_SECURITY_ERR;| end up throwing an exception in the content script? Like |return NS_ERROR_DOM_SECURITY_ERR;| would do? > > ::: dom/push/src/PushService.jsm > > @@ +202,5 @@ > > > + getAllByOrigin: function(aPageURL, aSuccessCb, aErrorCb) { > > > + debug("getAllByOrigin() " + aPageURL); > > > + if (!aPageURL) { > > > + if (typeof aErrorCb == "function") { > > > + aErrorCb("PushDB.getAllByOrigin: Got undefined aPageURL"); > > > > It's not clear to me what this message is for. Isn't this going to call > > _onRegistrationsError() ultimately? How is this trying being used there? By > > concern is that we shouldn't have that kind of stuff in DOMError.name. > > I'm sorry, I didn't know that DOMRequest.fireError only supported name and > not a description. Please, don't be sorry. This is a very common mistake and if people are making this mistake that often it is very likely because of the API that is exposed to pass an error to a DOMRequest. We should have put some ways to enforce a correct usage.

This patch moves out much of the system messages changes into bug 868322. Added support for prompt on desktop. Added registerPage() calls to get system messages working. Changed DOMRequest errors to use DOM spec error names. Removed fakeSuccess and instead fail with SecurityError. Mounir: throw NS_ERROR_DOM* does not work in JS because the DOM error codes are not exposed to Components.results in JS.

Comment on attachment 751937 [details] [diff] [review] Desktop push. Review of attachment 751937 [details] [diff] [review]: ----------------------------------------------------------------- r=me with my comments applied and the register() method signature discussed a bit more. I guess Doug need to review this code too. You should also ask a browser/ peer to review the changes in nsBrowserGlue.js and browser.properties. You should also ask a ui-review for the strings in browser.properties. ::: browser/locales/en-US/chrome/browser/browser.properties @@ +300,5 @@ > +push.alwaysAllow=Always allow > +push.alwaysAllow.accesskey=A > +push.neverAllow=Never allow > +push.neverAllow.accesskey=N > +push.allowFromSite=Do you want to allow push notifications from %S? You should ask ui-review for that I believe. FWIW, I wonder why the accesskey for "allowForSession" is lowercase while the other a uppercase. It might change nothing though. Also, I think the description will never be understood by a real human being: I doubt most our users have any idea what "push notifications" are and what would be the consequences of allowing them. ::: dom/interfaces/push/nsIDOMPushManager.idl @@ +21,5 @@ > * > * On success, the DOMRequest's result field will be a string URL. This URL > * is the endpoint that can be contacted to wake up the application. > */ > + nsIDOMDOMRequest register([optional] in ACString wakeupURL); I'm not terribly convinced that we should bind the system message mechanism to every API that will require it. I'm also not a super fan of having two different method signatures, one for application context and one for browser context. I will not r- the patch for that but I would appreciate some discussions before pushing this forward. ::: dom/permission/PermissionPromptService.js @@ +71,5 @@ > let type = aRequest.access !== "unused" ? aRequest.type + "-" + aRequest.access > : aRequest.type; > let perm = > permissionManager.testExactPermissionFromPrincipal(aRequest.principal, type); > + dump(type + " " + perm); I guess this debug code should go away. ::: dom/push/src/Push.js @@ +168,5 @@ > + }, > + allow: function() { > + // Add push permissions to the wakeup URL. > + Services.perms.add(wakeupURI, 'push', Ci.nsIPermissionManager.ALLOW_ACTION, > + Ci.nsIPermissionManager.EXPIRE_NEVER); I believe this should be done by nsIContentPermissionPrompt, shouldn't it? I'm afraid you might end up over-writting session permission with a always permission here. ::: dom/push/src/PushService.jsm @@ +505,5 @@ > this._requestTimeout = prefs.get("requestTimeout"); > > this._udpPort = prefs.get("udp.port"); > > this._db.getAllChannelIDs( nit: a comment saying what this method is doing would be pretty awesome. @@ -506,5 @@ > - > - // At this point, profile-change-net-teardown has already fired, so the > - // WebSocket has been closed with NS_ERROR_ABORT (if it was up) and will > - // try to reconnect. Stop the timer. > - this._stopAlarm(); Is that chunk here on purpose? @@ +1265,5 @@ > + return; > + } > + // If endpoint was created by a web page, only allow pages with the same > + // origin permission to delete the endpoint. > + else if (!record.manifestURL) { No need for "else" here. @@ +1274,5 @@ > + " is trying to delete endpoint set by " + recordPageURI.spec); > + try { > + Services.scriptSecurityManager.checkSameOriginURI(callerPageURI, > + recordPageURI, > + false); If the call is developer initiated, having true as a third parameter would be better so developers can debug more easily.

Comment on attachment 751937 [details] [diff] [review] Desktop push. Review of attachment 751937 [details] [diff] [review]: ----------------------------------------------------------------- madhava - please look at the browser.properties change. We are adding a door hanger with the text: Do you want to allow push notifications from <some website>? ::: dom/interfaces/push/nsIDOMPushManager.idl @@ +26,1 @@ > +1 Nsm, can you bring this up at the next public WebAPI meeting and get a discussion going?

(In reply to Doug Turner (:dougt) from comment #38) > Comment on attachment 751937 [details] [diff] [review] > Desktop push. > > Review of attachment 751937 [details] [diff] [review]: > ----------------------------------------------------------------- > > madhava - please look at the browser.properties change. We are adding a > door hanger with the text: > > Do you want to allow push notifications from <some website>? Would it make more sense to re-label this as "This website wants to monitor/run in the background. Allow?". "Push notifications" is not part of the vernacular. > > ::: dom/interfaces/push/nsIDOMPushManager.idl > @@ +26,1 @@ > > > > +1 Nsm, can you bring this up at the next public WebAPI meeting and get a > discussion going? Yes.

I am working on a set of video chat projects that all needs push notifications. I would say Push notifications are increasingly part of the vernacular due to over a billion smartphones in use. I think "Website wants to monitor/run" is misleading, as the web page does not actually run in the background, but is being woken up by notifications. It would also be good to keep the same terminalogy on desktop, Android and Firefox OS since the user may get the same email/chat/call notification both on desktop and mobile.

The UI seems to expose only the host instead of exposing the whole origin. Is the expectation that scheme and port would be too subtle for the user to police anyway?

henri, this follows what geolocation does, right?

(In reply to Mounir Lamouri (:mounir) from comment #37) > Comment on attachment 751937 [details] [diff] [review] > Desktop push. > > Review of attachment 751937 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with my comments applied and the register() method signature discussed > a bit more. > > I guess Doug need to review this code too. You should also ask a browser/ > peer to review the changes in nsBrowserGlue.js and browser.properties. You > should also ask a ui-review for the strings in browser.properties. > > ::: browser/locales/en-US/chrome/browser/browser.properties > @@ +300,5 @@ > > +push.alwaysAllow=Always allow > > +push.alwaysAllow.accesskey=A > > +push.neverAllow=Never allow > > +push.neverAllow.accesskey=N > > +push.allowFromSite=Do you want to allow push notifications from %S? > > You should ask ui-review for that I believe. > FWIW, I wonder why the accesskey for "allowForSession" is lowercase while > the other a uppercase. It might change nothing though. Also, I think the > description will never be understood by a real human being: I doubt most our > users have any idea what "push notifications" are and what would be the > consequences of allowing them. That's because the character which will be the accesskey 's' in 'session' is lowercase. > > ::: dom/interfaces/push/nsIDOMPushManager.idl > @@ +21,5 @@ > > * > > * On success, the DOMRequest's result field will be a string URL. This URL > > * is the endpoint that can be contacted to wake up the application. > > */ > > + nsIDOMDOMRequest register([optional] in ACString wakeupURL); > > I'm not terribly convinced that we should bind the system message mechanism > to every API that will require it. I'm also not a super fan of having two > different method signatures, one for application context and one for browser > context. I will not r- the patch for that but I would appreciate some > discussions before pushing this forward. Probably moving to a System Messages API. > > @@ -506,5 @@ > > - > > - // At this point, profile-change-net-teardown has already fired, so the > > - // WebSocket has been closed with NS_ERROR_ABORT (if it was up) and will > > - // try to reconnect. Stop the timer. > > - this._stopAlarm(); > > Is that chunk here on purpose? stopAlarm() is already called in shutdownWS() which is called just above this line.

(In reply to Doug Turner (:dougt) from comment #42) > henri, this follows what geolocation does, right? So it seems.

Bug 868322 comment 14 has been updated with things relevant to this feature.

Comment on attachment 749580 [details] [diff] [review] encapsulate-WebSocket-in-PushSocket.patch Obsoleted since cpeterson forked PushService for android.

Adds Page Info Dialog entry to allow/disallow page for the origin. @Doug, do I need to ask for sr on this, it modifies FX UI. Whom should I ask?

Changed to use { worker: 'script.js' } for desktop.

Comment on attachment 762382 [details] [diff] [review] Add pageinfo dialog entry for SimplePush Review of attachment 762382 [details] [diff] [review]: ----------------------------------------------------------------- we need a browser peer to review this. Gavin, can you review this simple patch or find someone?

Comment on attachment 762384 [details] [diff] [review] Desktop push. Review of attachment 762384 [details] [diff] [review]: ----------------------------------------------------------------- with fixes, and new bugs filed, and r+ from sicking. ::: browser/components/nsBrowserGlue.js @@ +1859,5 @@ > + > + let originString = requestingURI.schemeIs("file") ? requestingURI.path : requestingURI.host; > + let message = browserBundle.formatStringFromName("push.allowFromSite", [originString], 1); > + > + let actions = [ in some of these functions, we use var. in others we use let. :( @@ +1875,5 @@ > + }, > + ]; > + > + this._showPrompt(aRequest, message, "push", actions, "push", > + "push-notification-icon", null); what is the push-notification-icon? did we get something from ux here? ::: browser/locales/en-US/chrome/browser/browser.properties @@ +298,5 @@ > +push.alwaysAllow=Always allow > +push.alwaysAllow.accesskey=A > +push.neverAllow=Never allow > +push.neverAllow.accesskey=N > +push.allowFromSite=Do you want to allow push notifications from %S? madhava should signoff on this text. lgtm (although the case we use in browser.properties is inconsistent. For example: webNotifications.alwaysShow=Always Show Notifications pointerLock.alwaysAllow=Always allow hiding Notice the case of the second and third word. Nikhil, can you file a follow up bug and assign it to Madhava for now? He'll tell us how to make things consistent or if this is a real issue. ::: dom/interfaces/push/nsIDOMPushManager.idl @@ +28,5 @@ > + * a push is received. > + * This page receives a push system message. > + * > + * eventPage (string) This URL is loaded in a hidden headless window and > + * delivered the push system message. Do we need to define a bit what an eventPage is? @@ +36,1 @@ > trailing whitespace ::: dom/push/src/Push.js @@ +56,4 @@ > > + let perm = Services.perms.testExactPermissionFromPrincipal(principal, > + "push"); > + if (perm != Ci.nsIPermissionManager.ALLOW_ACTION) if () { stmt; } @@ +60,5 @@ > + return null; > + } else { > + // If we are in a non-URI principal like the system principal there is > + // no origin or manifest. > + if (!principal.URI) same style nit. @@ +63,5 @@ > + // no origin or manifest. > + if (!principal.URI) > + throw Components.results.NS_ERROR_FAILURE; > + > + this._isWebPage = true; Could you use a variable name more descriptive? how about this._useEventPageTarget or something? @@ +128,5 @@ > } > }, > > + register: function(aOptions) { > + debug("register() " + aOptions); does aOptions have to be serialized to a string (or it will just print [Object]) @@ +132,5 @@ > + debug("register() " + aOptions); > + let req = this.createRequest(); > + > + if (this._isWebPage) { > + if (!aOptions.worker) { in the IDL, you didn't mentioned .worker. @@ +145,5 @@ > + debug(e.message); > + throw Components.results.NS_ERROR_INVALID_ARG; > + } > + > + Services.scriptSecurityManager.checkSameOriginURI(this._pageURI, add a comment above this same origin check on what exactly you want to ensure. @@ +174,5 @@ > + requestID: this.getRequestId(req) > + }); > + }.bind(this) > + }); > + } else { I think you can flip this if around and return early to avoid one level of indentation. if (!this._isWebPage) { whatever; return; } do the stuff for isWebPage. ::: dom/push/src/PushService.jsm @@ +48,5 @@ > + "nsISystemMessagesInternal"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "ppmm", > + "@mozilla.org/parentprocessmessagemanager;1", > + "nsIMessageListenerManager"); make sure you don't leak ppmm. I remember having problems with a lazy getter and leaking. @@ +212,5 @@ > + } > + return; > + } > + > + let pageURI = Services.io.newURI(aPageURL, null, null); this can be null, right? need test? @@ +234,5 @@ > + } > + > + try { > + let recordPageURI = Services.io.newURI(cursor.value.pageURL, > + null, null); test needed @@ +236,5 @@ > + try { > + let recordPageURI = Services.io.newURI(cursor.value.pageURL, > + null, null); > + > + Services.scriptSecurityManager.checkSameOriginURI(pageURI, add comment about what kind of test this is for.

(In reply to Doug Turner (:dougt) from comment #52) > Comment on attachment 762384 [details] [diff] [review] > Desktop push. > > Review of attachment 762384 [details] [diff] [review]: > ----------------------------------------------------------------- > > with fixes, and new bugs filed, and r+ from sicking. > > ::: browser/components/nsBrowserGlue.js > @@ +1859,5 @@ > > + > > + let originString = requestingURI.schemeIs("file") ? requestingURI.path : requestingURI.host; > > + let message = browserBundle.formatStringFromName("push.allowFromSite", [originString], 1); > > + > > + let actions = [ > > in some of these functions, we use var. in others we use let. :( Filed 884625. > > @@ +1875,5 @@ > > + }, > > + ]; > > + > > + this._showPrompt(aRequest, message, "push", actions, "push", > > + "push-notification-icon", null); > > what is the push-notification-icon? did we get something from ux here? It's blank. Leaving in here for now, will follow up with Madhava on email. > > ::: browser/locales/en-US/chrome/browser/browser.properties > @@ +298,5 @@ > > +push.alwaysAllow=Always allow > > +push.alwaysAllow.accesskey=A > > +push.neverAllow=Never allow > > +push.neverAllow.accesskey=N > > +push.allowFromSite=Do you want to allow push notifications from %S? > > madhava should signoff on this text. lgtm (although the case we use in > browser.properties is inconsistent. For example: > > > webNotifications.alwaysShow=Always Show Notifications > > pointerLock.alwaysAllow=Always allow hiding > > > Notice the case of the second and third word. Nikhil, can you file a follow > up bug and assign it to Madhava for now? He'll tell us how to make things > consistent or if this is a real issue. Filed 884629. > > ::: dom/interfaces/push/nsIDOMPushManager.idl > @@ +28,5 @@ > > + * a push is received. > > + * This page receives a push system message. > > + * > > + * eventPage (string) This URL is loaded in a hidden headless window and > > + * delivered the push system message. > > Do we need to define a bit what an eventPage is? I really don't know what more to say here. As far as the webdev is concerned, this is it. MDN could use a longer description talking about lifetimes or something. > @@ +128,5 @@ > > } > > }, > > > > + register: function(aOptions) { > > + debug("register() " + aOptions); > > does aOptions have to be serialized to a string (or it will just print > [Object]) Removed. JSON serialization is expensive and this isn't so important. > > @@ +132,5 @@ > > + debug("register() " + aOptions); > > + let req = this.createRequest(); > > + > > + if (this._isWebPage) { > > + if (!aOptions.worker) { > > in the IDL, you didn't mentioned .worker. Replacing with eventPage. > > @@ +174,5 @@ > > + requestID: this.getRequestId(req) > > + }); > > + }.bind(this) > > + }); > > + } else { > > I think you can flip this if around and return early to avoid one level of > indentation. Obsoleted by {page:} support for apps. > > ::: dom/push/src/PushService.jsm > @@ +48,5 @@ > > + "nsISystemMessagesInternal"); > > + > > +XPCOMUtils.defineLazyServiceGetter(this, "ppmm", > > + "@mozilla.org/parentprocessmessagemanager;1", > > + "nsIMessageListenerManager"); > > make sure you don't leak ppmm. I remember having problems with a lazy > getter and leaking. Taken care of in uninit()

Carrying forward dougt's r+. Add |{page:}| and |{eventPage:}| support to register()

Attachment 764871 [details] shows how this will look. Doug. We have two prefs 1) services.push.enabled - When set to false prevents push from starting at all. Toggling the pref in the UI would require a restart. 2) services.push.connection.enabled - PushService starts, but no connection and hence no push notifications. Attempting to call register() results in NetworkError. Calls to registrations() and unregister() are allowed. The current patch toggles (2). A patch on my machine to obsolete 747914 also moves the RELEASE_BUILD guards from (1) to (2) meaning the push service is enabled in all builds, but no registrations are allowed in Beta and Release until the *user* flips the pref. Is this appropriate?

Comment on attachment 747914 [details] [diff] [review] Preferences Shouldn't at least some of these preferences live in all.js instead of firefox.js?

Comment on attachment 764487 [details] [diff] [review] Desktop push with event page support Can you split the browser/ portions of this patch into a separate patch and get them reviewed by a Firefox peer?

I guess ideally the app-related parts of these patches (i.e. code changing under browser/) should be in their own Firefox::General bug, rather than grouped together with changes to the DOM code.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #57) > Comment on attachment 747914 [details] [diff] [review] > Preferences > > Shouldn't at least some of these preferences live in all.js instead of > firefox.js? I am holding of from that until Android also has Push enabled.

Comment on attachment 762382 [details] [diff] [review] Add pageinfo dialog entry for SimplePush Moved to bug 885093

Moves most prefs into all.js Since services.push.connection.enabled should be false for desktop release builds, it stays in firefox.js. connection.enabled and udp.wakeupEnabled are both true by default on B2G.

Moved browser changes into bug 885093. navigator.push is now null in private browsing mode. This should stay this way until SimplePush is modified to use multiple IndexedDBs. Filed 885108.

Comment on attachment 765066 [details] [diff] [review] Preferences Review of attachment 765066 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/app/b2g.js @@ +400,3 @@ > // Is the network connection allowed to be up? > // This preference should be used in UX to enable/disable push. > pref("services.push.connection.enabled", true); This is such a confusing preference. push.userSuspend? ::: browser/app/profile/firefox.js @@ +1277,5 @@ > +// SimplePush > +// Is the network connection allowed to be up? > +// This preference should be used in UX to enable/disable push. > +#ifdef RELEASE_BUILD > +pref("services.push.connection.enabled", false); This worries me. So, in RELEASE_BUILD, we want it so that push and all of its "stuff" isn't exposed to web content at all. That is, if you enumerate the global window and navigator, there will be no interface names, objects, etc that are related to push. nit: we should rename all of the preferences from ^services.* to ^push.*. This is not sync, nor are we mconnor :) ::: modules/libpref/src/init/all.js @@ +4117,5 @@ > // WebAlarms > pref("dom.mozAlarms.enabled", false); > > // SimplePush > +pref("services.push.enabled", true); This is probably the pref that should govern if we expose anything to web content. @@ +4120,5 @@ > // SimplePush > +pref("services.push.enabled", true); > +// serverURL to be assigned by services team > +pref("services.push.serverURL", "wss://push.services.mozilla.com"); > +pref("services.push.userAgentID", ""); please add a short comment above each of these to indicate what they do.

Comment on attachment 765071 [details] [diff] [review] Desktop push with event page support Review of attachment 765071 [details] [diff] [review]: ----------------------------------------------------------------- sr=me assuming you've hooked this up to the webapps-clear-data notification? ::: dom/push/src/Push.js @@ +160,5 @@ > + type = 'eventpage'; > + url = aOptions.eventPage; > + } else { > + type = 'page'; > + url = this._pageURI.spec; I think we should remove this default and simply throw an exception here. It's probably the less common case to actually want a page to pop up in response an incoming push message, then to run some logic and only open a tab after that. So making that the default doesn't seem good.

Renamed connection.enabled to userEnabled. Set push.enabled to false on release builds.

Comment on attachment 765071 [details] [diff] [review] Desktop push with event page support Unconventional, ui-review=madhava https://twitter.com/madhava/status/348226852927700992

Comment on attachment 765071 [details] [diff] [review] Desktop push with event page support This patch doesn't have any UI, so not sure why it was marked ui-review+.

Comment on attachment 765997 [details] [diff] [review] Preferences Looks fine in general (enabled pref per-app, other prefs global, #ifdef RELEASE_BUILD in Firefox), but I didn't look at the specifics (I assume dougt will).

Comment on attachment 765997 [details] [diff] [review] Preferences Review of attachment 765997 [details] [diff] [review]: ----------------------------------------------------------------- I don't like "services." prefix. Leave it up to you if you really want to keep this.

(In reply to Doug Turner (:dougt) from comment #70) > Comment on attachment 765997 [details] [diff] [review] > Preferences > > Review of attachment 765997 [details] [diff] [review]: > ----------------------------------------------------------------- > > I don't like "services." prefix. Leave it up to you if you really want to > keep this. bug 887880

Rebased on changes from 867632 and 889984

Gavin, I'd like to land this in desktop Firefox *disabled in all builds* as soon as 868322 has landed. Ok?

We discussed this on IRC, and as I recall you were going to post an updated patch that matches what you actually want to land.

Looking at the patches, I presume that _startListeningIfChannelsPresent() only creates a socket connection to the push servers IF the client has known, channels registered, correct? Otherwise r- I don't want another situation like b2g where devices maintain an idle socket open to the servers.

(In reply to JR Conlin [:jrconlin,:jconlin] from comment #76) > Looking at the patches, I presume that _startListeningIfChannelsPresent() > only creates a socket connection to the push servers IF the client has > known, channels registered, correct? That is correct. This should be the behaviour on b2g too, are you not seeing that based on server activity?

removed as direct dependency from Loop_MVP client bug, 971986. Only some of this bug's dependencies will be blocking for 971986. Those should be put on bug 976789, the [meta] bug for the Push Server for Loop MVP desktop as needed.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #77) > (In reply to JR Conlin [:jrconlin,:jconlin] from comment #76) > > Looking at the patches, I presume that _startListeningIfChannelsPresent() > > only creates a socket connection to the push servers IF the client has > > known, channels registered, correct? > > That is correct. This should be the behaviour on b2g too, are you not seeing > that based on server activity? Yes, unless SimplePush is far more successful than we'd suspect. I've seen fairly constant, regular growth of users without a great deal more traffic. Since I don't report a lot of metric data for privacy reasons, I can only conclude that clients are connecting regardless of if they have a channel.

We're shipping the standard Push API now (bug 1153503) and I doubt we'll ever ship SimplePush on desktop. Feel free to re-open if I'm incorrect here.