Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Description • 11 years ago

The system messages API is required for Push notifications.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 1 • 11 years ago

This patch enables it in the build and sets the pref.

It also fixes a shutdown leak in ActivitiesService.jsm.

	Gene Lian (I already quit Mozilla)
	Comment 2 • 11 years ago

Alarm API for desktop also needs this.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 3 • 11 years ago

Removed MOZ_SYS_MSG.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 6 • 11 years ago

Based on the dev.webapi discussion [1] and requirements for Push, I've made some changes to the system messages API to get it working for webpages. As per mounir's comments on bug 857464 - system messages should work properly with a empty manifest.

To that end the patch:
* Makes manifests optional at the IDL level.
* Uses PermissionManager to check permissions for webpages.
* Loads the handler page for the system message in a hidden iframe (Sandbox from toolkit/identity) to simulate the 'eventPage' behaviour outlined in [1].

Fabrice,
I don't like how I've used 'web-page' as an index into _listeners. Semantically it didn't make sense to index by page URL instead. Any suggestions?

I've done some clean up too. There was confusion about the types of variable names ending in URI. All variables expected to be nsIURI now end in URI, while strings end in URL. I may have missed some.

Mounir, Jonas:

Is this on the right track? Good enough to land for push while preserving backwards compatibility?

[1] https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/diddFoHYLVs

	Mounir Lamouri (:mounir)
	Comment 7 • 11 years ago

Comment on attachment 751936 [details] [diff] [review]
Allow system messages to work with webpages

Review of attachment 751936 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds generally good but I feel that something might be missing. How would a web page register for a system message? I would expect that a web page could specify some system messages in a manifest (as an installed application) and have it working even if the application isn't installed. I'm afraid that if we want to make that work your patch might not be enough. However, if we don't care about this use case, it should be fine.

Another thing that would be nice would be to add a programmatic register/unregister methods but I guess that part could be done later.

Finally, I think Gene knows a bit about our system message implementation, he might have interesting feedback too.

::: dom/messages/SystemMessageManager.js
@@ -167,5 @@
>        // so a re-launched app won't handle it again, which is redundant.
>        cpmm.sendAsyncMessage(
>          "SystemMessageManager:Message:Return:OK",
>          { type: msg.type,
> -          manifest: msg.manifest,

Is there a reason why you are no longer sending the manifest?

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +193,1 @@
>      }

This else statement doesn't seem needed.

@@ +195,5 @@
> +    for (let permName in permNames) {
> +      if (!newManifest) {
> +        let originURI = Service.io.newURI(aOrigin, null, null);
> +        let permissionManager = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
> +        let permValue = permissionManager.testExactPermission(originURI, permName);

Could you use a principal to test the permission?

@@ +269,3 @@
>      for (let permName in permNames) {
> +      if (!aManifestURL) {
> +        let permValue = Services.perms.testExactPermission(pageURI, permName);

Ditto, could you use a principal?

	Gene Lian (I already quit Mozilla)
	Comment 8 • 11 years ago

Comment on attachment 751936 [details] [diff] [review]
Allow system messages to work with webpages

Review of attachment 751936 [details] [diff] [review]:
-----------------------------------------------------------------

It seems we still have a long way to go but it's a very brave start!

In addition to the following comments, I don't see the corresponding handle for manifest === "web-page" in the SystemMessageInternal.js:

case "SystemMessageManager:Unregister":
  ...
  this._removeTargetFromListener(...)
  ...

::: dom/messages/SystemMessageInternal.js
@@ +95,5 @@
>      // clean it up from the pending message queue when apps receive it.
>      let messageID = gUUIDGenerator.generateUUID().toString();
>  
>      debug("Sending " + aType + " " + JSON.stringify(aMessage) +
> +      " for " + aPageURI.spec + " @ " + (aManifestURI ? aManifestURI.spec : ""));

aManifestURI ? aManifestURI.spec : "web-page"

@@ +109,5 @@
>      }
>  
>      let pagesToOpen = {};
>      this._pages.forEach(function(aPage) {
> +      if (!this._isPageMatched(aPage, aType, aPageURI.spec, aManifestURI ? aManifestURI.spec : undefined)) {

I know it's just a WIP. Just a reminder that you need to fold this line, which is too long.

@@ +118,5 @@
>        this._queueMessage(aPage, aMessage, messageID);
>  
>        // Open app pages to handle their pending messages.
>        // Note that we only need to open each app page once.
>        let key = this._createKeyForPage(aPage);

Not sure what's happening in the ._createKeyForPage(...). I'm wondering it's not working when aPage.manifest === undefined.

@@ +124,5 @@
> +        if (aPage.manifest) {
> +          this._openAppPage(aPage, aMessage);
> +        } else {
> +          this._openEventPage(aPage, aMessage);
> +        }

Can we just just the same function like _openPage(...) and diverge the branches by |aPage.manifest| within that function?

@@ +134,5 @@
>    broadcastMessage: function broadcastMessage(aType, aMessage) {
>      // Buffer system messages until the webapps' registration is ready,
>      // so that we can know the correct pages registered to be broadcasted.
> +    // FIXME: sending to web pages should still work even if registry isn't
> +    // ready. Ideally just check that this isn't on B2G.

Sounds good. That would be perfect if you could check this by a platform configuration but I don't where it is for now.

@@ +172,5 @@
> +          if (aPage.manifest) {
> +            this._openAppPage(aPage, aMessage);
> +          } else {
> +            this._openEventPage(aPage, aMessage);
> +          }

Ditto.

@@ +180,5 @@
>      }, this);
>    },
>  
>    registerPage: function registerPage(aType, aPageURI, aManifestURI) {
> +    if (!aPageURI) {

If you're able to identify the platform type (see below), it'd be better keep the sanity check of |aManifestURI| for webapps.

@@ +185,4 @@
>        throw Cr.NS_ERROR_INVALID_ARG;
>      }
>  
> +    // TODO: add uniqueness check

We had a bug for this at Bug 874339 around here.

@@ +266,1 @@
>                                               winCount: 1 }];

Alignment.

@@ +382,5 @@
>          Services.obs.removeObserver(this, "xpcom-shutdown");
>          Services.obs.removeObserver(this, "webapps-registry-start");
>          Services.obs.removeObserver(this, "webapps-registry-ready");
>          ppmm = null;
> +        this._sandboxes.forEach(function(sandbox) {

s/sandbox/aSandbox

@@ +434,5 @@
>  
> +  _openEventPage: function _openEventPage(aPage, aMessage) {
> +    let page = { uri: aPage.uri,
> +                 type: aPage.type,
> +                 target: aMessage.target };

It's not very clear to me what the |aMessage.target| is. Can we really access that?

@@ +436,5 @@
> +    let page = { uri: aPage.uri,
> +                 type: aPage.type,
> +                 target: aMessage.target };
> +    debug("Asking to open event page " + JSON.stringify(page));
> +    

Trailing spaces.

@@ +448,3 @@
>      return (aPage.type === aType &&
> +            aPage.manifest === aManifestURL &&
> +            aPage.uri === aPageURL)

Yeap... the terminologies are really chaotic here due to the history. Well, that would be a bonus if you could also do:

s/aPage.manifest/aPage.manifestURL
s/aPage.uri/aPage.pageURL

where we often use |manifest| as the JSON object of the manifest data and use |manifestURL| as the link to the manifest page.

Well, if you don't like to correct naming issues in this bug. It's OK.

::: dom/messages/SystemMessageManager.js
@@ +153,5 @@
>    },
>  
>    receiveMessage: function sysMessMgr_receiveMessage(aMessage) {
>      debug("receiveMessage " + aMessage.name + " for [" + aMessage.data.type + "] " +
> +          "with uri = " + aMessage.data.uri + " (" + this._uri + ")");

Somehow, we still need to dump the manifest URL for B2G debugging.

Btw, if you're going to receive manifest === "web-page" here, the following check cannot pass:

  if (msg.manifest != this._manifest || msg.uri != this._uri) {
    return;
  }

@@ +162,2 @@
>        return;
>      }

OK. Here it is as mentioned above.

@@ +166,5 @@
>        // Send an acknowledgement to parent to clean up the pending message,
>        // so a re-launched app won't handle it again, which is redundant.
>        cpmm.sendAsyncMessage(
>          "SystemMessageManager:Message:Return:OK",
>          { type: msg.type,

Wait! Where is the manifest URL? In the parent, we need to use it for 2 purposes at least:

1. It's an index to identify the pending queue to clean up the message in the queue.

2. It's a sanity check to prevent hacked child process from sending malicious IPC message.

May be you should keep sending "web-page" here and check that in the SystemMessageInternal to not to run through the .assertContainApp() check.

@@ +201,5 @@
> +      let appsService = Cc["@mozilla.org/AppsService;1"]
> +                          .getService(Ci.nsIAppsService);
> +
> +      this._manifest = appsService.getManifestURLByLocalId(principal.appId);
> +    }

else {
  this._manifest = "web-page";
}

maybe.

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +187,5 @@
> +                        "Cannot decide the app's status. Install cancelled.");
> +        break;
> +      }
> +
> +      newManifest = new ManifestHelper(aManifest, aOrigin);

Why do you want to use ManifestHelper(...) here? I don't see its purpose.

@@ +195,5 @@
> +    for (let permName in permNames) {
> +      if (!newManifest) {
> +        let originURI = Service.io.newURI(aOrigin, null, null);
> +        let permissionManager = Cc["@mozilla.org/permissionmanager;1"].getService(Ci.nsIPermissionManager);
> +        let permValue = permissionManager.testExactPermission(originURI, permName);

As Mounir's suggesting, it'd be better to use principal.

@@ +245,5 @@
>     * Check if the system message is permitted to be sent to the given
>     * app's page at run-time based on the current app's permissions.
>     * @param string aSysMsgName
>     *        The system messsage name.
>     * @param string aPageURI

s/aPageURI/aPageURL

	Gene Lian (I already quit Mozilla)
	Comment 9 • 11 years ago

(In reply to Nikhil Marathe from comment #6)
> Fabrice,
> I don't like how I've used 'web-page' as an index into _listeners.
> Semantically it didn't make sense to index by page URL instead. Any
> suggestions?

Agreed. It'd be better to not to use the 'web-page' as a keyword. I think a better solution could be to examine the platform configuration. When it's for desktop, then the manifest URL is allowed to be undefined. For gonk, the manifest URL has always to be a valid URL and we should keep all the sanity checks for that. Please correct me if I'm wrong.

> 
> I've done some clean up too. There was confusion about the types of variable
> names ending in URI. All variables expected to be nsIURI now end in URI,
> while strings end in URL. I may have missed some.

Exactly right. The terminologies are really confusing. It's also occurring in the attributes of IPC messages. I'd suggest let's fix all the terminology things in an independent patch and have your patches for fixing this issue on top of that.

	Mounir Lamouri (:mounir)
	Comment 10 • 11 years ago

(In reply to Gene Lian [:gene] from comment #9)
> (In reply to Nikhil Marathe from comment #6)
> > Fabrice,
> > I don't like how I've used 'web-page' as an index into _listeners.
> > Semantically it didn't make sense to index by page URL instead. Any
> > suggestions?
> 
> Agreed. It'd be better to not to use the 'web-page' as a keyword. I think a
> better solution could be to examine the platform configuration. When it's
> for desktop, then the manifest URL is allowed to be undefined. For gonk, the
> manifest URL has always to be a valid URL and we should keep all the sanity
> checks for that. Please correct me if I'm wrong.

I disagree. There is no reason to make the behaviour per-platform. We could definitely have applications in Firefox Desktop as in Firefox OS and we could imagine the opposite too. I would prefer if the backend would be ready for various scenarios instead of targeting the easiest path.

	Gene Lian (I already quit Mozilla)
	Comment 11 • 11 years ago

Ah, good point! Mounir is right.

	[:fabrice] Fabrice Desré
	Comment 12 • 11 years ago

Comment on attachment 751930 [details] [diff] [review]
Fix leaks in ActivityService

Review of attachment 751930 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nit fixed.

::: dom/activities/src/ActivitiesService.jsm
@@ +178,5 @@
>      }, this);
>      ppmm = null;
>  
> +    if (this.db)
> +      this.db.close();

nit: use { } even for single line if's

	[:fabrice] Fabrice Desré
	Comment 13 • 11 years ago

Comment on attachment 751931 [details] [diff] [review]
Patch 1: Flip the pref.

Review of attachment 751931 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but of course we need the main piece first...

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 14 • 11 years ago

I've modified the earlier patch with some suggestions from [1]. There is a lot of cruft in the patch so asking for feedback instead of review. This patch adds eventPage support to system messages. 'sandboxes' are now SystemMessageFrameWorker which is a slight modification of the Social API's FrameWorker [2]. In retrospect this should be called 'worker' and not 'eventPage' because it is a JS file and most available APIs are what workers should eventually support. 

There is a sendMessageToEventPage, rather than overloading sendMessage() because I could not find a good API that encapsulates both approaches. Mainly, for sendMessage(), we want to be able to pass just a manifestURL to deal with the situation of bug 800431, whereas for sendMessageToEventPage(), we want to be able to pass just a pageURL. I've tried to explain in [1] why it is necessary to maintain this distinction at the individual WebAPI level (Push, Alarms) and not in system messages.

The SystemMessageFrameWorker implements the launch() function which is a open-unique-window open() and also dispatches a system message called 'launched' to the opened tab/window. This will be modified to instead launch the app on b2g.

At this stage I'd like feedback on:
1) Am I messing things up too much? Do we need to start thinking from scratch?
2) Is moving responsibility to WebAPIs for tracking the type of receiver (app or webpage) a good idea?
3) How would an eventPage/worker work in the context of FxOS. This is more of a technical question, I have no idea how to launch a headless webpage on B2G but with the permissions and sandboxing set to that defined by the manifest.

[1]: https://groups.google.com/d/msg/mozilla.dev.webapi/diddFoHYLVs/69ayu8U8jiMJ
[2]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/FrameWorker.jsm

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 15 • 11 years ago

Fix leaks in ActivityService - https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d4292301de

	Ed Morley [:emorley]
	Comment 16 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/c6d4292301de

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 17 • 11 years ago

Since its a JS script with the available APIs those which workers have available, better to let this be used as worker, so navigator.push.register({ worker: 'script.js' }).

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 18 • 11 years ago

The 'worker' launched by system messages is not allowed to interact with windows on the same origin right now. It can only use 'launch()' to start a new window. This can be enabled later if we have a use for this.

	Gene Lian (I already quit Mozilla)
	Comment 19 • 11 years ago

Comment on attachment 762387 [details] [diff] [review]
JS should be loaded as 'worker' argument

Review of attachment 762387 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Nikhil,

Summary for why I gave review-

1. The case of "SystemMessageManager:Unregister" isn't properly handled.
2. I don't understand why do you hard-code the "SimplePush.
3. Please try to refactorize the common part of .sendMessageToWorker(...) and sendMessage(...).

The rest looks good to me. Nice patch!

I have to say sorry I'm afraid I'm not right person to review the SystemMessageWorker.jsm part. Could you please find the original designer or reviewer of the original FrameWorker.jsm to review your codes? Btw, in my opinion, I don't think it's good to do such a big copy-and-paste like this. We should try to reuse or refactorize the existing FrameWorker.jsm codes.

::: dom/messages/SystemMessageInternal.js
@@ +181,5 @@
>        }
>      }, this);
>    },
>  
> +  sendMessageToWorker: function sendMessageToWorker(aType, aMessage, aPageURI) {

Lots of codes within this function can be shared with .sendMessage(...). Please try to refactorize the common part. It can help the maintenance in the future.

@@ +219,5 @@
>    broadcastMessage: function broadcastMessage(aType, aMessage) {
>      // Buffer system messages until the webapps' registration is ready,
>      // so that we can know the correct pages registered to be broadcasted.
> +    // FIXME: sending to web pages should still work even if registry isn't
> +    // ready. Ideally just check that this isn't on B2G.

I'm afraid you need to fix this. What's happening if we're wanting to broadcast system messages to the web pages? The |._webappsRegistryReady| is never to be true because it cannot catch the "webapps-registry-start" for a pure Desktop environment. Right?

I'm fine to keep as it is if we don't have chance to use the broadcast by far. Please fire a bug for it and put the bug number here:

// TODO: bug XXXXXX - ...

@@ +353,5 @@
>          return true;
>          break;
>        case "SystemMessageManager:Register":
>        {
> +        let manifest = msg.manifest || "web-page";

This is good but I don't think you've done the same thing for the "SystemMessageManager:Unregister" case properly. This is one of the main reasons to give review-.

@@ +545,5 @@
> +                   manifest: aPage.manifest,
> +                   type: aPage.type,
> +                   target: aMessage.target };
> +      Services.obs.notifyObservers(this, "system-messages-open-app", JSON.stringify(page));
> +    } 

TS.

@@ +550,4 @@
>    },
>  
> +  _openWorker: function _openWorker(aPage, aMessage) {
> +    getSystemMessageWorkerHandle(aPage.uri, null, 'SimplePush:'+aPage.uri, aPage.uri);

I don't understand this part. Why do you hard-code the 'SimplePush:' keyword here?

If my understanding is correct, the main purpose of sending system messages to a worker is attempting to let the worker handle the system messages instead of opening an app page to do that. The only difference is *who's going to do the handle* and should have nothing to do with the SimplePush. Right?

@@ +553,5 @@
> +    getSystemMessageWorkerHandle(aPage.uri, null, 'SimplePush:'+aPage.uri, aPage.uri);
> +  },
> +
> +  _isPageMatched: function _isPageMatched(aPage, aType, aPageURL, aManifestURL) {
> +    debug("isPageMatched " + aPage.type + " " + aPage.manifest + " " + aPage.uri + " " + aType + " " + aManifestURL + " " + aPageURL);

Rewrite this to be:

debug("isPageMatched: [" + aPage.type + ", " + aPage.manifest + ", " + aPage.uri + "] " +
      "[" + aType + ", " + aManifestURL + ", " + aPageURL + "]");

@@ +571,5 @@
>  
>      // add uri and action to the hash
>      ["type", "manifest", "uri"].forEach(function(aProp) {
> +      if (!aPage[aProp])
> +        return;

Add { } for one-line if-block.

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +193,3 @@
>      }
>  
> +    for (let permName in permNames) {

Please add a debug message here:

  debug("Checking permission name: " + permName);

so that it can help both manifest and non-manigest apps.

@@ +254,5 @@
>     * @returns bool
>     *        Is permitted or not.
>     **/
>    isSystemMessagePermittedToSend:
> +    function isSystemMessagePermittedToSend(aSysMsgName, aPageURL, aManifestURL) {

Ah... why did I do this?! Good to correct the naming.

@@ +270,1 @@
>      for (let permName in permNames) {

Please add a debug message here:

  debug("Checking permission name: " + permName);

so that it can help both manifest and non-manigest apps.

@@ +270,5 @@
>      for (let permName in permNames) {
> +      if (!aManifestURL) {
> +        let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(pageURI);
> +        let permValue = Services.perms.testExactPermissionFromPrincipal(principal, permName);
> +        debug("Non-manifest check " + aPageURL + " " + permName + " " + permValue);

And you don't need the above line for debugging.

@@ +272,5 @@
> +        let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(pageURI);
> +        let permValue = Services.perms.testExactPermissionFromPrincipal(principal, permName);
> +        debug("Non-manifest check " + aPageURL + " " + permName + " " + permValue);
> +        if (permValue != Ci.nsIPermissionManager.ALLOW_ACTION) {
> +          debug("DENIED");

Could you replace this line by:

debug("'" + aSysMsgName + "' isn't permitted by '" + permName + "'. " +
      "Please add the permission for app: '" + pageURI.prePath + "'.");

just like below.

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +16,5 @@
>     * Allow any internal user to broadcast a message of a given type.
>     * @param type        The type of the message to be sent.
>     * @param message     The message payload.
>     * @param pageURI     The URI of the page that will be opened.
>     * @param manifestURI The webapp's manifest URI.

* @param manifestURI The webapp's manifest URI (optional).

@@ +28,5 @@
> +   * @param type        The type of the message to be sent.
> +   * @param message     The message payload.
> +   * @param pageURI     The URI of the script that will be opened.
> +   */
> +  void sendMessageToWorker(in DOMString type, in jsval message, in nsIURI pageURI);

It seems you're also hoping to solve the issue of sending system messages to a worker as well. I have a question here: why can't we send the system messages to a worker of an *App* (i.e. by specifying the manifestURI)? Or do you think it should be going to be a separate bug?

Also, I think it's worth merging this function with the existing .sendMessage(...) because almost all the logic can be shared except for the tiny difference of opening worker. Maybe, we can do:

  void sendMessage(in DOMString type, in jsval message, in nsIURI pageURI, [optional] in nsIURI manifestURI, [optional] in boolean handledByWorker);

If you still like to keep them separate, please try to refactorize the common part in .sendMessage(...) and  .sendMessageToWorker(...) by a local function.

@@ +42,5 @@
>    /*
>     * Registration of a page that wants to be notified of a message type.
>     * @param type          The message type.
>     * @param pageURI       The URI of the page that will be opened.
>     * @param manifestURI   The webapp's manifest URI.

* @param manifestURI The webapp's manifest URI (optional).

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 20 • 11 years ago

@Mark: Could you look at SystemMessageWorker.jsm that this patch introduces. It is based on the Social API's FrameWorker, but the ClientPort logic has been stripped out (asking for review on the other patch too)

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 21 • 11 years ago

(In reply to Gene Lian [:gene] from comment #19)
> 
> I have to say sorry I'm afraid I'm not right person to review the
> SystemMessageWorker.jsm part. Could you please find the original designer or
> reviewer of the original FrameWorker.jsm to review your codes? Btw, in my
> opinion, I don't think it's good to do such a big copy-and-paste like this.
> We should try to reuse or refactorize the existing FrameWorker.jsm codes.

I removed a lot of code form the worker, hence the copy. The other patch is the removal.
If we decide to add the same functionality in the future, we can make the component reusable.

> 
> ::: dom/messages/SystemMessageInternal.js
> @@ +181,5 @@
> >        }
> >      }, this);
> >    },
> >  
> > +  sendMessageToWorker: function sendMessageToWorker(aType, aMessage, aPageURI) {
> 
> Lots of codes within this function can be shared with .sendMessage(...).
> Please try to refactorize the common part. It can help the maintenance in
> the future.

I have done that in the new patch.

> 
> @@ +219,5 @@
> >    broadcastMessage: function broadcastMessage(aType, aMessage) {
> >      // Buffer system messages until the webapps' registration is ready,
> >      // so that we can know the correct pages registered to be broadcasted.
> > +    // FIXME: sending to web pages should still work even if registry isn't
> > +    // ready. Ideally just check that this isn't on B2G.
> 
> I'm afraid you need to fix this. What's happening if we're wanting to
> broadcast system messages to the web pages? The |._webappsRegistryReady| is
> never to be true because it cannot catch the "webapps-registry-start" for a
> pure Desktop environment. Right?

Filed bug.

> 
> @@ +353,5 @@
> >          return true;
> >          break;
> >        case "SystemMessageManager:Register":
> >        {
> > +        let manifest = msg.manifest || "web-page";
> 
> This is good but I don't think you've done the same thing for the
> "SystemMessageManager:Unregister" case properly. This is one of the main
> reasons to give review-.

Fixed.

> 
> @@ +550,4 @@
> >    },
> >  
> > +  _openWorker: function _openWorker(aPage, aMessage) {
> > +    getSystemMessageWorkerHandle(aPage.uri, null, 'SimplePush:'+aPage.uri, aPage.uri);
> 
> I don't understand this part. Why do you hard-code the 'SimplePush:' keyword
> here?

This was a mistake, fixed.

> 
> ::: dom/messages/interfaces/nsISystemMessagesInternal.idl
> @@ +16,5 @@
> >     * Allow any internal user to broadcast a message of a given type.
> >     * @param type        The type of the message to be sent.
> >     * @param message     The message payload.
> >     * @param pageURI     The URI of the page that will be opened.
> >     * @param manifestURI The webapp's manifest URI.
> 
> * @param manifestURI The webapp's manifest URI (optional).

Actually, removed this change, see explanation below.

> 
> @@ +28,5 @@
> > +   * @param type        The type of the message to be sent.
> > +   * @param message     The message payload.
> > +   * @param pageURI     The URI of the script that will be opened.
> > +   */
> > +  void sendMessageToWorker(in DOMString type, in jsval message, in nsIURI pageURI);
> 
> It seems you're also hoping to solve the issue of sending system messages to
> a worker as well. I have a question here: why can't we send the system
> messages to a worker of an *App* (i.e. by specifying the manifestURI)? Or do
> you think it should be going to be a separate bug?

It's not 'also', but 'only' for now. That is, on desktop, the worker is the only way to use system messages, which is why their is an explicit method call. This should not be used (and currently push is the only user, and it does the right thing) to wake up apps simply because I haven't figured out how app workers would run. Since apps run in a separate process, but when the worker runs, it should be UI-less but still be in a child process, and then probably spawn another process to actually launch the app when that is required, this is something that I need to figure out before workers for apps are allowed. I will be adding support for |{ page: }| instead of |{ worker: }| soon, which will fix 800431 and also allow the same syntax to be used on desktop and FxOS, although that will not be a very nice experience on desktop since it will open a new tab.

To get to the point, yes it should be a separate bug :)

> 
> Also, I think it's worth merging this function with the existing
> .sendMessage(...) because almost all the logic can be shared except for the
> tiny difference of opening worker. Maybe, we can do:
> 
>   void sendMessage(in DOMString type, in jsval message, in nsIURI pageURI,
> [optional] in nsIURI manifestURI, [optional] in boolean handledByWorker);
> 
> If you still like to keep them separate, please try to refactorize the
> common part in .sendMessage(...) and  .sendMessageToWorker(...) by a local
> function.

I'm not fond of optional arguments, especially in weakly typed languages, since it is very easy to make mistakes. I'd suggest

    void sendMessage(in DOMString type, in jsval message, in DOMString how, in nsIURI pageURI, [optional] in nsIURI manifestURI);

where how would be set('page', 'worker', 'eventPage').

Right now I'd like to keep it separate as sendMessageToWorker() to save me the trouble of having to ensure that I'm not breaking existing code. Once we have a good idea a few months down the line of the worker approach and add support for eventPages and watch how they get used with the APIs, this can be refactored as required without affecting public APIs.

> 
> @@ +42,5 @@
> >    /*
> >     * Registration of a page that wants to be notified of a message type.
> >     * @param type          The message type.
> >     * @param pageURI       The URI of the page that will be opened.
> >     * @param manifestURI   The webapp's manifest URI.
> 
> * @param manifestURI The webapp's manifest URI (optional).

Forgot to fix this before I uploaded the new patch, but its fixed in my local copy which will get committed.

	Mark Hammond [:markh] [:mhammond]
	Comment 22 • 11 years ago

I'm lacking some context here - even after reading the references etc - so please excuse naive questions or comments...

For the social API, we did experiment with using real workers, and the blocker for us was that our first partner really wanted blocking WebSockets - but they aren't supported in our workers and we couldn't come up with a way to simulate them sanely.  So FrameWorker was an implementation that did it's best to expose only stuff you would expect to find in a real shared worker, but do so in an iframe.  FWIW, we also made the mistake of exposing local storage to our worker implementation when the current consensus is that it will end up in real workers (so at a minimum I'd suggest you avoid exposing that to the sandbox)

As far as the social project is concerned, this is technical debt.  The obvious downside is that we are pretending to offer all the nice features of workers, but on the main thread - so if workers manage to block, they block the world.  As soon as WebSockets appear in workers, we will try and start the process of converting to a real worker.  Even if dedicated workers are still the only flavour of workers supported, we could probably simulate shared workers well enough for our requirements.

So my initial question is why a real worker can't be used here?  Is it because there is a hard requirement for websockets to work from day 1?  Also, without any of the message port functionality, how is this "worker" expected to communicate with "real" pages from the app?  It would be nice if we can avoid doubling the debt that our frameworker imposed...

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 23 • 11 years ago

(In reply to Mark Hammond (:markh) from comment #22)
> For the social API, we did experiment with using real workers, and the
> blocker for us was that our first partner really wanted blocking WebSockets
blocking WebSockets? WebSockets aren't supported in workers, but there isn't supposed to be
any kind of blocking WebSocket API.

> FWIW, we also made the mistake of exposing
> local storage to our worker implementation when the current consensus is
> that it will end up in real workers (so at a minimum I'd suggest you avoid
> exposing that to the sandbox)
I assume you mean "it will _not_ end up in real workers"

	Mark Hammond [:markh] [:mhammond]
	Comment 24 • 11 years ago

(In reply to Olli Pettay [:smaug] from comment #23)
> (In reply to Mark Hammond (:markh) from comment #22)
> > For the social API, we did experiment with using real workers, and the
> > blocker for us was that our first partner really wanted blocking WebSockets
> blocking WebSockets? WebSockets aren't supported in workers,

I believe they are supported in Chrome, and bug 504553 is for our impl.

> but there isn't supposed to be any kind of blocking WebSocket API.

Yes, sorry for being misleading.  The synchronous constructor was actually what meant we couldn't implement them via a shim back to the main thread.  You are obviously correct that the bit-moving parts are all async.

> > FWIW, we also made the mistake of exposing
> > local storage to our worker implementation when the current consensus is
> > that it will end up in real workers (so at a minimum I'd suggest you avoid
> > exposing that to the sandbox)
> I assume you mean "it will _not_ end up in real workers"

oops - yes.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 25 • 11 years ago

(In reply to Mark Hammond (:markh) from comment #22)
> I'm lacking some context here - even after reading the references etc - so
> please excuse naive questions or comments...
> 
> For the social API, we did experiment with using real workers, and the
> blocker for us was that our first partner really wanted blocking WebSockets
> - but they aren't supported in our workers and we couldn't come up with a
> way to simulate them sanely.  So FrameWorker was an implementation that did
> it's best to expose only stuff you would expect to find in a real shared
> worker, but do so in an iframe.  FWIW, we also made the mistake of exposing
> local storage to our worker implementation when the current consensus is
> that it will end up in real workers (so at a minimum I'd suggest you avoid
> exposing that to the sandbox)

I think you mean localStorage will *not* end up in real workers?

The reason I'm using the worker iframe shim is that what system messages needs is a shared worker, with only one instance running, which is currently not implemented in Gecko.

> 
> As far as the social project is concerned, this is technical debt.  The
> obvious downside is that we are pretending to offer all the nice features of
> workers, but on the main thread - so if workers manage to block, they block
> the world.  As soon as WebSockets appear in workers, we will try and start
> the process of converting to a real worker.  Even if dedicated workers are
> still the only flavour of workers supported, we could probably simulate
> shared workers well enough for our requirements.

Hmm, now that you mention it, I might be able to use normal workers for my current use case too. I'll look into that. My major requirement is to be able to introduce JS functions into the 'worker' that is run. It seems Cu.Sandbox is the only safe way to do this, which rules out allowing developers to use complete HTML pages like chrome [eventPage]. Any advice on this front would be great (I want to be able to expose a function or have a message system so that the worker can send a message to *chrome code* in some way).

[eventPage]: http://developer.chrome.com/extensions/event_pages.html

> 
> So my initial question is why a real worker can't be used here?  Is it
> because there is a hard requirement for websockets to work from day 1? 
> Also, without any of the message port functionality, how is this "worker"
> expected to communicate with "real" pages from the app?  It would be nice if
> we can avoid doubling the debt that our frameworker imposed...

WebSocket is not a hard requirement. Nor is being able to communicate with real pages at this point, although it would be a nice to have. In which case using a worker with no ports might be possible to do.

	Mark Hammond [:markh] [:mhammond]
	Comment 26 • 11 years ago

(In reply to Nikhil Marathe from comment #25)
> (In reply to Mark Hammond (:markh) from comment #22)

> I think you mean localStorage will *not* end up in real workers?

Yep.

> Hmm, now that you mention it, I might be able to use normal workers for my
> current use case too. I'll look into that. My major requirement is to be
> able to introduce JS functions into the 'worker' that is run. It seems
> Cu.Sandbox is the only safe way to do this, which rules out allowing
> developers to use complete HTML pages like chrome [eventPage]. Any advice on
> this front would be great (I want to be able to expose a function or have a
> message system so that the worker can send a message to *chrome code* in
> some way).
> 
> [eventPage]: http://developer.chrome.com/extensions/event_pages.html

Right - so my understanding is that you can use importScripts in a worker which can introduce JS functions.  You'd probably bootstrap the worker with your framework code supplying such functions, then importScripts again to load the actual target script.  The only effective way of communicating back to the chrome code on the main thread would be passing messages over.

A quick look at [eventPage] implies that isn't using a worker.  It talks about events, visible views, window.setTimeout() etc - all of which aren't available in workers - but critically, also aren't going to be available in the frameworker.  So - if you want "complete HTML pages", you probably don't want *either* real-workers or frame-workers.

But if you would prefer complete HTML pages, I'm a little confused why you are looking at frameworker at all and why simply hanging an iframe of the hidden DOM Window would work better?  I don't see why introducing your own JS functions into this environment would be a problem.  Note that if this general approach would work, an even better implementation might be to host it in a remote <browser> meaning alot of the work is still moved off the main process (and therefore off the main thread).  The background thumbnail service (in toolkit/components/thumbnails) uses this approach, so it's not without precedent.

I'm (obviously) still lacking some context and don't want to be stop-energy for your efforts - if you decide that all you really want out of me is a review, let me know :)

	Gene Lian (I already quit Mozilla)
	Comment 27 • 11 years ago

Comment on attachment 762880 [details] [diff] [review]
JS should be loaded as 'worker' argument

Review of attachment 762880 [details] [diff] [review]:
-----------------------------------------------------------------

The system message part looks good to me. r=gene with the following fixes or verifications.

Btw, recently we had a big refactoring work at Bug 874339. I'm afraid you have to rewrite you patch on top of that. Although I'm giving you review+, I'm glad to take another look on your rebased patch. :)

Nice patch!

::: dom/messages/SystemMessageInternal.js
@@ +132,5 @@
>        }
>      }
>    },
>  
> +  _sendMessageTo: function _sendMessageTo(aType, aMessage, aPageURI, aManifestURI, opener) {

s/opener/aOpener

@@ +164,5 @@
>        // Open app pages to handle their pending messages.
>        // Note that we only need to open each app page once.
>        let key = this._createKeyForPage(aPage);
>        if (!pagesToOpen.hasOwnProperty(key)) {
> +        opener(aPage, aMessage);

Ditto: s/opener/aOpener

@@ +173,5 @@
>  
> +  sendMessage: function sendMessage(aType, aMessage, aPageURI, aManifestURI) {
> +    // Buffer system messages until the webapps' registration is ready,
> +    // so that we can know the correct pages registered to be sent.
> +    if (aManifestURI && !this._webappsRegistryReady) {

So if the |aManifestURI| always has a valid value in your new design, you don't need to check the |aManifestURI| here. Right?

@@ +182,5 @@
> +                                   manifestURI: aManifestURI });
> +      return;
> +    }
> +
> +    this._sendMessageTo(aType, aMessage, aPageURI, aManifestURI, this._openAppPage);

Are you sure you don't need to bind this for |this._openAppPage| which is using |this| in its function?

  this._openAppPage.bind(this)

I didn't test that. Just suspecting this doesn't work. Please fix and test it. Thanks!

@@ +194,4 @@
>    broadcastMessage: function broadcastMessage(aType, aMessage) {
>      // Buffer system messages until the webapps' registration is ready,
>      // so that we can know the correct pages registered to be broadcasted.
> +    // TODO: Bug 883215.

Please add some words here to describe the purpose or maybe just directly put the bug title here.

TODO: Bug 883215 - Unify SystemMessageInternal.broadcastMessage to work with webpages

::: dom/messages/interfaces/nsISystemMessagesInternal.idl
@@ +42,5 @@
>    /*
>     * Registration of a page that wants to be notified of a message type.
>     * @param type          The message type.
>     * @param pageURI       The URI of the page that will be opened.
>     * @param manifestURI   The webapp's manifest URI.

Just a reminder:

The webapp's manifest URI (optional).

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 28 • 11 years ago

Gene: rebased patch, it made things a lot easier.

Drew: Asking for review since I'm using something similar to the thumbnails component. Can you feedback? on jar.mn, SystemMessageWorker.jsm (sorry for the bad file name) and SystemMessageEventPageContentScript.js. I've described below what this does.

Jonas: Feedback on the general idea please.

Event pages use a xul:browser on the hiddenDOMWindow to load the page, then uses loadFrameScript to implement launch() within the 'eventPage'. This way we get access to all standard APIs. I still have to lockdown the browser so a lot of things like media, window.open and popups are disabled.

When a push notification is received for a channel registered with navigation.push.register({eventPage: '/path.html' }), the page is loaded into a xul:browser where it uses mozSetMessageHandler to get pending push/push-register messages. The page can then do whatever it wishes to, including requesting a launch()

The launch() uses sendMessageToPage() to launch the url and then sends it a 'launched' system message.

Why I dropped workers: Based on mhammond's feedback and talking with jlebar, making a frame masquerade as a worker and then have extra APIs is not a good idea. We can wait on this until a real worker can access all the DOM APIs.

	Drew Willcoxon :adw
	Comment 29 • 11 years ago

Comment on attachment 764471 [details] [diff] [review]
Switch to xul:browser and eventpages

Looks OK to me.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 30 • 11 years ago

Updated patch to unload event pages that haven't received a system message in 5 minutes.

Also cleaned up some of the event page setup code.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 31 • 11 years ago

Comment on attachment 765135 [details] [diff] [review]
Switch to xul:browser and eventpages

Review of attachment 765135 [details] [diff] [review]:
-----------------------------------------------------------------

Is there any documentation of how an event-page can interact with the outside world? I.e. how it would enumerate existing windows, how it opens new windows, etc.

It doesn't look like the code here does anything special, which I guess would mean that there is no way to enumerate existing windows, and new windows can be opened using the normal window.open syntax?

I suspect we need someone from toolkit to review this code too. Justin Dolske or Neal Deakin are good candidates.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 32 • 11 years ago

There is no way to enumerate current windows, and window.open is disabled as well. Instead a function called launch() is introduced which checks that the URL conforms to the same origin and only opens the URL in a tab if that URL is not already open. Otherwise the open tab is simply sent a system message.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 33 • 11 years ago

Comment on attachment 765135 [details] [diff] [review]
Switch to xul:browser and eventpages

Review of attachment 765135 [details] [diff] [review]:
-----------------------------------------------------------------

I've kind'a just skimmed this as there's too much code here that I'm not familiar with. You definitely should have a toolkit peer review this stuff.

::: dom/messages/SystemMessageEventPageContentScript.js
@@ +12,5 @@
> +  }
> +};
> +
> +addEventListener("DOMWindowCreated", function(e) {
> +  content.wrappedJSObject.launch = function(url, message) {

Please make sure that this event listener is only run for the top-level page of the event page. So no other windows that happen to be created at the same time, and no  <iframe>s that are created by the event page.

@@ +13,5 @@
> +};
> +
> +addEventListener("DOMWindowCreated", function(e) {
> +  content.wrappedJSObject.launch = function(url, message) {
> +    if (typeof url !== "string") {

Don't do this. Convert whatever value was passed in to a string instead.

@@ +19,5 @@
> +      return;
> +    }
> +
> +    let uri;
> +    dump("System message origin is " + this.origin);

Probably want to make this debug-only.

@@ +21,5 @@
> +
> +    let uri;
> +    dump("System message origin is " + this.origin);
> +    try {
> +      uri = Services.io.newURI(url, null, Services.io.newURI(SystemMessageContent.origin, null, null));

You should resolve the passed in url against the baseURI of the calling page. You can use document.baseURIObject to get a nsIURI object of the baseURI which you can pass to the newURI function as the base.

@@ +32,5 @@
> +                                                        Services.io.newURI(SystemMessageContent.origin, null, null),
> +                                                        true);
> +    } catch(e) {
> +      Cu.reportError(e);
> +      return;

Why not let this exception bubble out to the page?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 34 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #33)
> Comment on attachment 765135 [details] [diff] [review]
> Switch to xul:browser and eventpages
> 
> Review of attachment 765135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've kind'a just skimmed this as there's too much code here that I'm not
> familiar with. You definitely should have a toolkit peer review this stuff.
> 
> ::: dom/messages/SystemMessageEventPageContentScript.js
> @@ +12,5 @@
> > +  }
> > +};
> > +
> > +addEventListener("DOMWindowCreated", function(e) {
> > +  content.wrappedJSObject.launch = function(url, message) {
> 
> Please make sure that this event listener is only run for the top-level page
> of the event page. So no other windows that happen to be created at the same
> time, and no  <iframe>s that are created by the event page.

The eventPage is forbidden from having iframes or opening windows. I changed the code to allow iframes to check if launch() was available to an iframe, it was not available.

> 
> @@ +13,5 @@
> > +};
> > +
> > +addEventListener("DOMWindowCreated", function(e) {
> > +  content.wrappedJSObject.launch = function(url, message) {
> > +    if (typeof url !== "string") {
> 
> Don't do this. Convert whatever value was passed in to a string instead.

I'm using toString(). Is that the right way? Because it gives weird but valid URLs when something like |function() {}| is passed as the url parameter.

> 
> @@ +19,5 @@
> > +      return;
> > +    }
> > +
> > +    let uri;
> > +    dump("System message origin is " + this.origin);
> 
> Probably want to make this debug-only.
Done.
> 
> @@ +21,5 @@
> > +
> > +    let uri;
> > +    dump("System message origin is " + this.origin);
> > +    try {
> > +      uri = Services.io.newURI(url, null, Services.io.newURI(SystemMessageContent.origin, null, null));
> 
> You should resolve the passed in url against the baseURI of the calling
> page. You can use document.baseURIObject to get a nsIURI object of the
> baseURI which you can pass to the newURI function as the base.

Thanks for the tip!

> 
> @@ +32,5 @@
> > +                                                        Services.io.newURI(SystemMessageContent.origin, null, null),
> > +                                                        true);
> > +    } catch(e) {
> > +      Cu.reportError(e);
> > +      return;
> 
> Why not let this exception bubble out to the page?
Done

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 35 • 11 years ago

Comment on attachment 751930 [details] [diff] [review]
Fix leaks in ActivityService

Mark as having been checked in.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 36 • 11 years ago

Added fixes suggested by Jonas.

EventPages watch for the 'unload' event on hiddenDOMWindow and clean themselves up to avoid shutdown leaks.

Justin, Could you please review this code based on toolkit/components/thumbnails/{BackgroundPageThumbs.jsm,content/backgroundPageThumbsContent.js}.

An 'EventPage' creates a xul:browser which loads a third-party URL with restricted permissions which is sent a system message. The URL is an event page, and has a function on its window called 'launch()' which accepts a URL and some data, and sends it through to system messages to send a message to that URL, optionally opening the URL in a new tab (if it isn't already open).

EventPages have a lifetime of 5 minutes, which is extended every time the system messages API sends a page a message (to be precise, the lifetime is extended every time getSystemMessageEventPageHandle() is called). When a EventPage's lifetime is done, the xul:browser is destroyed.

I would like to know if the code I've written is as safe as a normal browser tab.

	Gene Lian (I already quit Mozilla)
	Comment 37 • 11 years ago

Comment on attachment 765721 [details] [diff] [review]
Switch to xul:browser and eventpages

Review of attachment 765721 [details] [diff] [review]:
-----------------------------------------------------------------

The rebased patch looks good to me.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 38 • 11 years ago

Btw, what happens with other mechanisms that open new windows? Like calling .click() on an <a target="..." href="...">? Or calling .submit() on a form that has a target? I'm sure there are more, but I believe they all go through some sort of link handler that lives on the docshell.

Also, I suspect that we should prevent navigating the document. I think navigations also go through said link handler mechanism.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 39 • 11 years ago

Added some comments and minor cleanup.

Carrying forward sr+ from :sicking and r+ from :gene.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 40 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #38)
> Btw, what happens with other mechanisms that open new windows? Like calling
> .click() on an <a target="..." href="...">? Or calling .submit() on a form
> that has a target? I'm sure there are more, but I believe they all go
> through some sort of link handler that lives on the docshell.
> 
> Also, I suspect that we should prevent navigating the document. I think
> navigations also go through said link handler mechanism.

The latest patch adds sandbox flags to the docShell which will prevent form submissions.
It is still possible to navigate the page by setting window.location, but I can't
find any way to prevent this.

	Shane Caraveo (:mixedpuppy)
	Comment 41 • 11 years ago

Comment on attachment 768408 [details] [diff] [review]
Switch to xul:browser and eventpages

Just a drive-by as I'm getting familiar with what is happening for desktop push.

>+    // It seems like on a Mac, the top level hiddenDOMWindow can host
>+    // a xul:browser directly, but not so on other platforms.  Much of this code
>+    // is from toolkit/components/thumbnails/BackgroundPageThumbs.jsm
>+    if (!canHostBrowser(this._parentWindow)) {
>+      // Otherwise, create an html:iframe, stick it in the parent document, and
>+      // use it to host the browser.  about:blank will not have the system
>+      // principal, so it can't host, but a document with a chrome URI will.
>+      let iframe =
>+        Services.appShell.hiddenDOMWindow.document.createElementNS(HTML_NS,
>+                                                                   "iframe");
>+      iframe.setAttribute("src", "chrome://global/content/mozilla.xhtml");
>+      let onLoad = function onLoadFn() {
>+        iframe.removeEventListener("load", onLoad, true);
>+        this._parentWindow = iframe.contentWindow;
>+        this._iframe = iframe;
>+        this._createBrowser();
>+        cb(this._browser);
>+      }.bind(this);
>+
>+      iframe.addEventListener("load", onLoad, true);
>+      Services.appShell.hiddenDOMWindow.document.documentElement.appendChild(iframe);
>+    } else {
>+      this._createBrowser();
>+      cb(this._browser);
>+    }
>+  },

You can use an html iframe on all platforms which would simplify this, and avoid potentially different behavior across platforms (e.g. xul:browser adds some event listeners in the constructor).  See makeHiddenFrame in https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/FrameWorker.jsm#339

Sorry, I haven't researched all the bugs/conversation around this yet.  Is there a reason this needs to allow content to be loaded?  Could the Frameworker be used instead, since we may need to make that work for social anyway.

>   // The set of listeners. This is a multi-dimensional object. The _listeners
>   // object itself is a map from manifest ID -> an array mapping proccesses to
>   // windows. We do this so that we can track both what processes we have to
>   // send system messages to as well as supporting the single-process case
>   // where we track windows instead.
>+  //
>+  // For webpages the manifest is the string "web-page".

I'm going to need to figure out how to make push work for waking socialapi frameworkers.  Is this magic string going to lock us out of that functionality?  I feel like push for web pages should make a fake manifest object with a flag for the type rather than having '|| "web-page"' in multiple locations.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 42 • 11 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #41)
> Comment on attachment 768408 [details] [diff] [review]
> Switch to xul:browser and eventpages
> 
> Just a drive-by as I'm getting familiar with what is happening for desktop
> push.
> 
> >+    // It seems like on a Mac, the top level hiddenDOMWindow can host
> >+    // a xul:browser directly, but not so on other platforms.  Much of this code
> >+    // is from toolkit/components/thumbnails/BackgroundPageThumbs.jsm
> >+    if (!canHostBrowser(this._parentWindow)) {
> >+      // Otherwise, create an html:iframe, stick it in the parent document, and
> >+      // use it to host the browser.  about:blank will not have the system
> >+      // principal, so it can't host, but a document with a chrome URI will.
> >+      let iframe =
> >+        Services.appShell.hiddenDOMWindow.document.createElementNS(HTML_NS,
> >+                                                                   "iframe");
> >+      iframe.setAttribute("src", "chrome://global/content/mozilla.xhtml");
> >+      let onLoad = function onLoadFn() {
> >+        iframe.removeEventListener("load", onLoad, true);
> >+        this._parentWindow = iframe.contentWindow;
> >+        this._iframe = iframe;
> >+        this._createBrowser();
> >+        cb(this._browser);
> >+      }.bind(this);
> >+
> >+      iframe.addEventListener("load", onLoad, true);
> >+      Services.appShell.hiddenDOMWindow.document.documentElement.appendChild(iframe);
> >+    } else {
> >+      this._createBrowser();
> >+      cb(this._browser);
> >+    }
> >+  },
> 
> You can use an html iframe on all platforms which would simplify this, and
> avoid potentially different behavior across platforms (e.g. xul:browser adds
> some event listeners in the constructor).  See makeHiddenFrame in
> https://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/
> FrameWorker.jsm#339
> 
> Sorry, I haven't researched all the bugs/conversation around this yet.  Is
> there a reason this needs to allow content to be loaded?  Could the
> Frameworker be used instead, since we may need to make that work for social
> anyway.

The earlier implementation (see obsoletes) did use a FrameWorker derivative. There are a couple of reasons I switched to using a xul:browser.

1) As :mhammond mentioned, xul:browsers can be run remote. After implementation I found out that remote doesn't work very well on desktop (especially on Windows if I remember a conversation with jlebar correctly), which sort of obsoletes the xul:browser, and yes maybe an iframe can be used on all platforms.

2) Event pages have access to the full DOM APIs, which Workers (and FrameWorkers) do not. Event pages in particular need access to Push, System Messages and Desktop Notifications. I believe FrameWorker's themselves are temporary implementations until real Workers support the required APIs.
> 
> >   // The set of listeners. This is a multi-dimensional object. The _listeners
> >   // object itself is a map from manifest ID -> an array mapping proccesses to
> >   // windows. We do this so that we can track both what processes we have to
> >   // send system messages to as well as supporting the single-process case
> >   // where we track windows instead.
> >+  //
> >+  // For webpages the manifest is the string "web-page".
> 
> I'm going to need to figure out how to make push work for waking socialapi
> frameworkers.  Is this magic string going to lock us out of that
> functionality?  I feel like push for web pages should make a fake manifest
> object with a flag for the type rather than having '|| "web-page"' in
> multiple locations.

Could you describe in more detail how push will interact with social and how the manifest hack will affect that. Yes I agree having the check at every point isn't the best, but wouldn't having a flag do the same thing?

	Shane Caraveo (:mixedpuppy)
	Comment 43 • 11 years ago

(In reply to Nikhil Marathe from comment #42)

> The earlier implementation (see obsoletes) did use a FrameWorker derivative.
> There are a couple of reasons I switched to using a xul:browser.

heh, sorry about that.  I dove into the patch primarily to see what was changing and how it would work then started commenting on it prior to reading all the bug backlog. /me facepalm


> > I'm going to need to figure out how to make push work for waking socialapi
> > frameworkers.  Is this magic string going to lock us out of that
> > functionality?  I feel like push for web pages should make a fake manifest
> > object with a flag for the type rather than having '|| "web-page"' in
> > multiple locations.
> 
> Could you describe in more detail how push will interact with social and how
> the manifest hack will affect that. Yes I agree having the check at every
> point isn't the best, but wouldn't having a flag do the same thing?

I really should familiarize myself with the background before commenting.  Anyway, with use cases for webrtc video chats (talkilla) and a need to support multiple providers active at the same time, we need to remove our "always running" frameworker and only wake it when necessary (to avoid multiple long lived workers in memory).

When a provider has active content (e.g. sidebar), we can wake the frameworker.  But for "background" providers, such as talkilla, we need push notifications for incoming calls/chats that would wake the frameworker logic.  We could support this by requiring a notification worker in addition to the frameworker, but it would be nice if we could avoid that.  It seems, if all the push events are actually dom events, that we may not be able to avoid having two workers per provider.

	Justin Dolske [:Dolske]
	Comment 44 • 11 years ago

I don't understand what this patch is doing, or why it's mucking about with the hidden window and sandboxes. It seems extremely hacky. Could you explain a bit about what you're trying to do?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 45 • 11 years ago

(In reply to Justin Dolske [:Dolske] from comment #44)
> I don't understand what this patch is doing, or why it's mucking about with
> the hidden window and sandboxes. It seems extremely hacky. Could you explain
> a bit about what you're trying to do?

Comment 28 gives a good overview of the patch. eventPages are headless pages loaded on the hiddenDOMWindow, and they have access to a few additional APIs (right now just |launch()|), to request opening a tab. eventPages act as the receiver for system messages. That is applications can request WebAPIs to deliver system messages to event pages instead of UI pages, and do background processing. The primary user right now is SimplePush (bug 857464). This eventPage should have access to all the standard DOM APIs, which is why the iframe on the hiddenDOMWindow. The reason for the xul:browser inside it is noted above (comment 42).

I agree it is hacky, but it seems like the best available option.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 46 • 11 years ago

Rather than re-implementing the hidden dom window stuff, can you use toolkit/identity/Sandbox.jsm? That should hopefully simplify things and remove some code duplication.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 47 • 11 years ago

(In reply to Nikhil Marathe [:nsm] from comment #42)
> 1) As :mhammond mentioned, xul:browsers can be run remote. After
> implementation I found out that remote doesn't work very well on desktop
> (especially on Windows if I remember a conversation with jlebar correctly),
> which sort of obsoletes the xul:browser, and yes maybe an iframe can be used
> on all platforms.

What problems did you run into with a remote browser? BackgroundPageThumbs uses them and seems to work fine. If they're problematic in some way it would be good to know why.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 48 • 11 years ago

Comment on attachment 768408 [details] [diff] [review]
Switch to xul:browser and eventpages

>+  // On certain platforms the hiddenDOMWindow cannot directly host a xul:browser
>+  // and requires a iframe wrapper.

>+    // It seems like on a Mac, the top level hiddenDOMWindow can host
>+    // a xul:browser directly, but not so on other platforms.  Much of this code
>+    // is from toolkit/components/thumbnails/BackgroundPageThumbs.jsm

On Mac, the hidden window loads a privileged XUL document (chrome://browser/content/hiddenWindow.xul).

On Windows/Linux, it loads an unprivileged HTML document (resource://gre-resources/hiddenWindow.html).

Unprivileged HTML documents can't contain XUL stuff since bug 546857.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 49 • 11 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #46)
> Rather than re-implementing the hidden dom window stuff, can you use
> toolkit/identity/Sandbox.jsm? That should hopefully simplify things and
> remove some code duplication.

I want to introduce custom functions into the page which will interact with chrome a bit. I can't figure out how to do that safely with just an iframe. Do I simply set objects on the contentWindow's wrappedJSObject?

	Shane Caraveo (:mixedpuppy)
	Comment 50 • 11 years ago

(In reply to Nikhil Marathe [:nsm] from comment #49)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #46)
> > Rather than re-implementing the hidden dom window stuff, can you use
> > toolkit/identity/Sandbox.jsm? That should hopefully simplify things and
> > remove some code duplication.
> 
> I want to introduce custom functions into the page which will interact with
> chrome a bit. I can't figure out how to do that safely with just an iframe.
> Do I simply set objects on the contentWindow's wrappedJSObject?

You can see an example in toolkit/components/social/MozSocialAPI.jsm.

I'd [personally] be a little concerned about providing a way for any website to open windows/tabs without user interaction, have you gone over that use case with anyone in UX?  Is the push api permission prompt enough of a barrier that users will connect that to tabs suddenly opening on them?

We do allow social providers to open chatwindows from the background, but there is a little more specificity in having the user install a social provider.  As well, on mobile, users have to install apps right now (I'm not sure if they are allowed to open UI directly upon a notification).

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 51 • 11 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #47)
> (In reply to Nikhil Marathe [:nsm] from comment #42)
> > 1) As :mhammond mentioned, xul:browsers can be run remote. After
> > implementation I found out that remote doesn't work very well on desktop
> > (especially on Windows if I remember a conversation with jlebar correctly),
> > which sort of obsoletes the xul:browser, and yes maybe an iframe can be used
> > on all platforms.
> 
> What problems did you run into with a remote browser? BackgroundPageThumbs
> uses them and seems to work fine. If they're problematic in some way it
> would be good to know why.

This is the log when a page is loaded in the remote frame on Linux:

Security problem: Content process does not have `undefined'.  It will be killed.
-- SystemMessageInternal 1372872224172 : Got message from a child process containing illegal manifest URL.
Security problem: Content process does not have `undefined'.  It will be killed.
-- SystemMessageInternal 1372872224173 : Got message from a child process containing illegal manifest URL.
[Parent 2638] WARNING: waitpid failed pid:2753 errno:10: file /home/nikhil/mozilla-central-push-b2g/ipc/chromium/src/base/process_util_posix.cc, line 260

###!!! [Parent][AsyncChannel] Error: Channel error: cannot send/recv

I haven't tried to track down the cause.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 52 • 11 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #50)
> (In reply to Nikhil Marathe [:nsm] from comment #49)
> > (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> > #46)
> > > Rather than re-implementing the hidden dom window stuff, can you use
> > > toolkit/identity/Sandbox.jsm? That should hopefully simplify things and
> > > remove some code duplication.
> > 
> > I want to introduce custom functions into the page which will interact with
> > chrome a bit. I can't figure out how to do that safely with just an iframe.
> > Do I simply set objects on the contentWindow's wrappedJSObject?
> 
> You can see an example in toolkit/components/social/MozSocialAPI.jsm.

AFAIK this allows JS only content (as in the 'event page' will be page.js, not page.html). Which is perfectly fine. I just don't want any confusion with workers.

> 
> I'd [personally] be a little concerned about providing a way for any website
> to open windows/tabs without user interaction, have you gone over that use
> case with anyone in UX?  Is the push api permission prompt enough of a
> barrier that users will connect that to tabs suddenly opening on them?

I agree. It might make sense to not have a direct launch(), but force the event page to show a desktop notification, whose onclick will be allowed to launch the app/page. I'm willing to land this patch without the launch() since event pages even without UI have a lot of value, allowing apps to run offline and update their content silently in the background on push notifications or alarms.

> 
> We do allow social providers to open chatwindows from the background, but
> there is a little more specificity in having the user install a social
> provider.  As well, on mobile, users have to install apps right now (I'm not
> sure if they are allowed to open UI directly upon a notification).

I've never used the social API, so I'm not aware of how the UI looks in that, I thought the chat windows appeared sort of inline inside the sidebar.

event pages are not available for mobile, in fact background services and so on are relevant to this, and something we'll discuss next week at the DOM/Content work week.

	Shane Caraveo (:mixedpuppy)
	Comment 53 • 11 years ago

(In reply to Nikhil Marathe [:nsm] from comment #52)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #50)
> > (In reply to Nikhil Marathe [:nsm] from comment #49)
> > > (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> > > #46)
> > > > Rather than re-implementing the hidden dom window stuff, can you use
> > > > toolkit/identity/Sandbox.jsm? That should hopefully simplify things and
> > > > remove some code duplication.
> > > 
> > > I want to introduce custom functions into the page which will interact with
> > > chrome a bit. I can't figure out how to do that safely with just an iframe.
> > > Do I simply set objects on the contentWindow's wrappedJSObject?
> > 
> > You can see an example in toolkit/components/social/MozSocialAPI.jsm.
> 
> AFAIK this allows JS only content (as in the 'event page' will be page.js,
> not page.html). Which is perfectly fine. I just don't want any confusion
> with workers.

MozSocialAPI is injected into the sidebar, chat windows, etc. as navigator.mozSocial.  It is not injected into the Frameworkers.  WorkerAPI.jsm is the postMessage based api for Frameworkers.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 54 • 11 years ago

(In reply to Nikhil Marathe [:nsm] from comment #51)
> Security problem: Content process does not have `undefined'.  It will be
> killed.
> -- SystemMessageInternal 1372872224173 : Got message from a child process
> containing illegal manifest URL.

> I haven't tried to track down the cause.

That's a SystemMessageInternal-specific check failing:
http://hg.mozilla.org/mozilla-central/annotate/85d75ed04851/dom/messages/SystemMessageInternal.js#l304

AssertAppProcess is printf'ing that it's not finding "undefined", so it looks like msg.manifest is undefined at
http://hg.mozilla.org/mozilla-central/annotate/85d75ed04851/dom/messages/SystemMessageInternal.js#l310

If you fix that do things work with a remote browser?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 55 • 11 years ago

This one uses sandbox.jsm.
I've removed the launch function() under the intention that we can land this in 25 so the Social API can use it.
As per the discussion me and Shane had, he is investigating how this can be shipped in release builds but disabled for webpages and only allowed for Social API and its providers.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 56 • 11 years ago

Comment on attachment 771629 [details] [diff] [review]
Using Sandbox.jsm

This looks a lot better.

The manual timer management seems error prone, it seems like you should be able to simplify using a closure.

getSystemMessageEventPageHandle is a very strange API, it looks like you copied FrameWorker, which is a bad idea because FrameWorker has all sorts of hysterical raisins for the weird API :) I would just call it openSystemMessagePage or some such, and remove the unused arguments.

I didn't review the non-SystemMessageEventPage.jsm bits.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 57 • 11 years ago

Wrapped Sandbox+nsITimer in EventPage to clean up code.

Asking Jonas for sr again since launch() is removed (see comment 55)

Justin can you go over the EventPage code please, which is now substantially simplified.

r=gene for System Messages changes is carried forward.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 58 • 11 years ago

Rebased SystemMessagesInternal.js against m-c.

Gene, please give it a once over due to changes from 888926.

Everyone else, comment 57 still in effect.

	Gene Lian (I already quit Mozilla)
	Comment 59 • 11 years ago

Comment on attachment 774190 [details] [diff] [review]
Using Sandbox.jsm

Review of attachment 774190 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: dom/messages/SystemMessageInternal.js
@@ +181,5 @@
> +    if (page) {
> +      // Queue this message in the corresponding pages.
> +      this._queueMessage(page, aMessage, messageID);
> +
> +        if (result === MSG_SENT_FAILURE_APP_NOT_RUNNING) {

nit: I know it's not your fault. Please fix the alignment for this if-block. Thanks!

	Justin Dolske [:Dolske]
	Comment 60 • 11 years ago

Comment on attachment 774190 [details] [diff] [review]
Using Sandbox.jsm

Review of attachment 774190 [details] [diff] [review]:
-----------------------------------------------------------------

Code in SystemMessageEventPage.jsm basically looks fine.

But has this feature gone through a privacy or security review? I must admit I'm a little wary about having random web pages being open to keep a semi-persistent iframe (sandbox) around without it being obvious to the user. (True, it's behind a permission prompt, that's only going to stop the simplest of drivebys.).

A few random "hmmms":

Looks like a registered page could send a notification to itself,and thus stay alive forever?

What happens if a registration happens in a private-browsing window, but a not-private window sends a notification (or vice-versa)? Does postMessage do anything special here?

If a user allows mail.comcast.com to get notifications, can comcast use that to window.open() or launch() ads without the user knowing where they're coming from?

Do we care about possible web-compat issues, due to the things that a sandbox doesn't do (like allow plugins, load images, etc).

I see the 5-minute timeout in the JSM, but how will the permission bit work in practice? Does the user have to re-allow a site to receive notifications after every browser relaunch? Or is this going to grant a permanent permission?

::: dom/messages/SystemMessageEventPage.jsm
@@ +62,5 @@
> +  }
> +}
> +
> +this.openSystemMessageEventPage = function openSystemMessageEventPage(url) {
> +  let page = cache[url];

Should something be making this a canonical URL? Or is that intentional?

EG, consider differences like "http://cats.com/#foo" vs "http://cats.com/#bar". Or "cats.com" vs "CATS.COM".

@@ +67,5 @@
> +  if (!page) {
> +    cache[url] = new EventPage(url);
> +  } else {
> +    page.keepAlive();
> +  }

You could drop the keepAlive() call from the constructor, and just call it unconditionally here. Your call, fine either way.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 61 • 11 years ago

(In reply to Justin Dolske [:Dolske] from comment #60)
> Comment on attachment 774190 [details] [diff] [review]
> Using Sandbox.jsm
> 
> Review of attachment 774190 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Code in SystemMessageEventPage.jsm basically looks fine.
> 
> But has this feature gone through a privacy or security review? I must admit
> I'm a little wary about having random web pages being open to keep a
> semi-persistent iframe (sandbox) around without it being obvious to the
> user. (True, it's behind a permission prompt, that's only going to stop the
> simplest of drivebys.).
> 
> A few random "hmmms":
> 
> Looks like a registered page could send a notification to itself,and thus
> stay alive forever?

Yes. I'm not sure how to fix this though.

> 
> What happens if a registration happens in a private-browsing window, but a
> not-private window sends a notification (or vice-versa)? Does postMessage do
> anything special here?

For now, push is disabled in private-browsing, so the window cannot register in a PB window. But a registration in a non-private window, could lead to a push notification in a PB window. I'll fix this.

> 
> If a user allows mail.comcast.com to get notifications, can comcast use that
> to window.open() or launch() ads without the user knowing where they're
> coming from?

launch() is disabled in the latest patch until the API is thought out better. window.open() is disabled. So ads are unlikely. This still makes push very useful to update client-side data.
> 
> Do we care about possible web-compat issues, due to the things that a
> sandbox doesn't do (like allow plugins, load images, etc).

I've no experience in this. I think the documentation can be pretty explicit in what event pages are for, to avoid such things.

> 
> I see the 5-minute timeout in the JSM, but how will the permission bit work
> in practice? Does the user have to re-allow a site to receive notifications
> after every browser relaunch? Or is this going to grant a permanent
> permission?

The permission is granted forever, until revoked by the user. This is a requirement for 'normal' operation of push. If an app is using push and the browser is restarted, I'd expect apps to continue functioning without having to prompt the user again, especially since the user may not visit the page that prompts after restarting the browser.

	Justin Dolske [:Dolske]
	Comment 62 • 11 years ago

(In reply to Nikhil Marathe [:nsm] from comment #61)

> > Looks like a registered page could send a notification to itself,and thus
> > stay alive forever?
> 
> Yes. I'm not sure how to fix this though.

Me either. :)

> > Do we care about possible web-compat issues, due to the things that a
> > sandbox doesn't do (like allow plugins, load images, etc).
> 
> I've no experience in this. I think the documentation can be pretty explicit
> in what event pages are for, to avoid such things.

That can help in the general "hey why are event pages different" sense, but I'm asking more about all the subtle (or maybe not-so-subtle) things that will come up due to differences in how various browsers implement this. That is, we're effectively turning Sandbox.jsm implementation details into webcompat issues, and should be cautions about that.

(I don't think this sinks the current patch, but I'd like our web-standards gurus to think about what the long-term plan is.)

> > I see the 5-minute timeout in the JSM, but how will the permission bit work
> > in practice? Does the user have to re-allow a site to receive notifications
> > after every browser relaunch? Or is this going to grant a permanent
> > permission?
> 
> The permission is granted forever, until revoked by the user.

OK, that's going to need some careful thinking before turning this feature on. A vague "do you want notifications" prompt is inadequate if the result is "this site can run script and open tabs at any time for the rest of your life, and you can't tell what site is doing it or how to stop it.".

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 63 • 11 years ago

Splitting the patch up. I'd like to land Patch 2 as soon as possible to avoid
bitrot. Patch 2 only makes manifests optionals.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 64 • 11 years ago

:dolske, I've made the timer change. I'd like to land this patch too.

By itself, this patch only sets up the infrastructure for event pages.
Until an API (likely to be Push) lands that uses it, shipping it in Firefox
will not cause any problems. In the very near future, we're going to need
some sort of event page mechanism anyway, especially for Firefox OS
and ideally for Firefox too. It might change away from Sandbox to a SharedWorker
like thing, but system message support will still be needed.

	Gene Lian (I already quit Mozilla)
	Comment 65 • 11 years ago

Comment on attachment 783231 [details] [diff] [review]
Patch 2: Allow manifest to be optional for System Messages.

Review of attachment 783231 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Btw, please make sure this patch is rebased on top of [1]. And please be careful all the activities and system message are still working well. We're having someone working on the system message test framework but not yet ready.

[1] https://hg.mozilla.org/mozilla-central/rev/6f6a4ea042c0

::: dom/messages/SystemMessageInternal.js
@@ +648,5 @@
>      }
>  
>    },
>  
> +  _getManifestString: function(aManifestURI) {

Could you please use _getManifestPlace or just _getManifestURL?

_getManifestString sounds something about the content of the manifest.

	[:fabrice] Fabrice Desré
	Comment 66 • 11 years ago

(In reply to Gene Lian [:gene] from comment #65)
> Comment on attachment 783231 [details] [diff] [review]
> Patch 2: Allow manifest to be optional for System Messages.
> 
> Review of attachment 783231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Btw, please make sure this patch is rebased on top of [1]. And
> please be careful all the activities and system message are still working
> well. We're having someone working on the system message test framework but
> not yet ready.

And that's why this patch can't land before we have tests. Any change done lately in this code had regressions, and it's not a risk I want us to take. There is no way to check that "activities and system message are still working" apart from doing a full QA smoketest.

	(no longer active)
	Comment 67 • 11 years ago

Comment on attachment 783257 [details] [diff] [review]
Patch 3: Add event page support to system messages.

You need to rev the iid of nsISystemMessagesInternal.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 68 • 11 years ago

:dolske, Please see comment 64.

	Justin Dolske [:Dolske]
	Comment 69 • 11 years ago

Comment on attachment 783257 [details] [diff] [review]
Patch 3: Add event page support to system messages.

Review of attachment 783257 [details] [diff] [review]:
-----------------------------------------------------------------

I was assuming a revised patch was still forthcoming, so the same concerns from comment 60 apply (since this is basically the same patch with the same issues). If nothing but privileged / whitelisted code ever made us of this, it's fine. But if this is meant to be the foundation for a web-exposed API (eg push notifications and the other deps on this bug), then I'd seriously question if this is the right approach. (Or perhaps if the API is needs revised in order to make it possible to implement safely.)

So r+ in the sense that this patch in isolation is ok, r- in the sense that I don't understand how we can actually build on top of it. :/

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 70 • 10 years ago

It seems like APIs requiring this should transition to ServiceWorkers, at least the Push spec has arrived a consensus regarding it.

Should this be marked invalid?

	Andrew Overholt [:overholt]
	Comment 71 • 10 years ago

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #70)
> It seems like APIs requiring this should transition to ServiceWorkers, at
> least the Push spec has arrived a consensus regarding it.
> 
> Should this be marked invalid?

If we're abandoning the approach here should we just make your patches obsolete, make this depend upon Service Workers, and try a new approach here that uses them?

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?) Reporter
	Comment 72 • 10 years ago

Some of the patches are still relevant to deal with apps and some underlying Gecko infrastructure. Even if the API were to change, we would probably reuse some of this. I'm marking the event page attachment as obsolete.

	Florian Bender
	Comment 73 • 9 years ago

What's the plan here now? Is this bug obsolete? What are the follow-up bugs that continue the plumbing work needed for mozAlarms/Background Sync? Push seems to be working now.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 74 • 9 years ago

This is obsolete now I think. We should use service workers instead.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 75 • 9 years ago

Please reopen if you disagree