Please see [1] and [2] for the original motivation and the initiative proposal. This API allows an application to send/get information to/from another application. [1] https://wiki.mozilla.org/WebAPI/Inter_App_Communication [2] https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.webapi/H2qobP0qzHM

Just having a meeting with Fernando, Thinker, Borja and Fred to achieve the following conclusions: 1. We don't need |.registerConnections(...)| anymore. The |.connect(...)| will be in charge of popping up the app selector and directly return the selected apps in its callback. 2. The Gecko can use the chrome event to launch the Gaia's selector and the Gaia can use the content event to return the selected results to the Gecko. 3. We decide to remove the |.accept()| and directly pass an |MessagePort| object in the system message. In the end, the system message to be handled is going to be: Dictionary ConnectionRequest { MessagePort port; DOMString keyword; DOMString origin; }; 4. We decide to remove the |.start()| from the |MessagePort|, because the assignment of |port.onmessage| implicitly implies the start. In the end, the |MessagePort| only has the following two capabilities: interface MessagePort { void postMessage(...); attribute jsval onmessage; }; 5. We decide to maintain a queue in the Gecko to save the sender's messages if the receiver's end doesn't set its |port.onmessage| yet. To avoid exploding the memory, we might need to use DB to maintain the queue. 6. Fernando suggested we should specify CRUD (i.e., Create/Read/Update/Delete) for each keyword in the app's manifest so that the receiver can decide which kinds of requesters are allowed to do the connection. Fernando will elaborate more on the design later. Please correct me if I misunderstood. Thanks!

(In reply to Gene Lian [:gene] from comment #1) > 1. We don't need |.registerConnections(...)| anymore. The |.connect(...)| > will be in charge of popping up the app selector and directly return the > selected apps in its callback. The idea is that the user will not be involved with selecting the application so having an application selector like in Web Activities in breaking that requirement. I think you should revisit the other changes based on that.

(In reply to Mounir Lamouri (:mounir) from comment #2) > The idea is that the user will not be involved with selecting the > application so having an application selector like in Web Activities in > breaking that requirement. Sorry I don't get it. So, you mean we shouldn't pop up an application selector in the end? But how can the users allow the connection at run-time? Note what we're just hoping to do is we let |.connect(...)| directly pop up the app selector. Calling another |.registerConnections(...)| is redundant and unnecessary.

(In reply to Gene Lian [:gene] from comment #3) > (In reply to Mounir Lamouri (:mounir) from comment #2) > > The idea is that the user will not be involved with selecting the > > application so having an application selector like in Web Activities in > > breaking that requirement. > > Sorry I don't get it. So, you mean we shouldn't pop up an application > selector in the end? But how can the users allow the connection at run-time? I posted some relevant thoughts here https://groups.google.com/d/msg/mozilla.dev.webapi/xFGpbZsWUhw/d92_Cjddp2wJ

Thanks for the summary Gene! (In reply to Gene Lian [:gene] from comment #1) > 6. Fernando suggested we should specify CRUD (i.e., > Create/Read/Update/Delete) for each keyword in the app's manifest so that > the receiver can decide which kinds of requesters are allowed to do the > connection. Fernando will elaborate more on the design later. Thinking about this twice, we can probably go without it. The idea behind this point is basically to allow apps to specify a security policy for each keyword. I would like to reword this and propose a change in the app manifest to specify the following: "connect": ["keyword1": { "allowed-apps": "certified", "permission-details": "Allow app to do whatever keyword1 does" }, "keywordN": { "allowed-apps": "installed", "permission-details": "Allow app to do whatever keywordN does" }] Where: - "connect" is the list of keywords that the app can connect with, as in the original proposal. With "keyword1" and "keywordN" as the names of these keywords. - "allowed-apps" is the minimum level of access (one of [1]) that an app requires to communicate with the receiver via that specific keyword. So for example an app declaring: "commlog": { "allowed-apps": "privileged", "permission-details": "Allow apps to send communication logs" } will only receive connection requests if the caller of |connect("commlog", {})| is a privileged app. - "permission-details" is the message to be shown to the user within the connection request UI to explain what is the app expecting to receive from a caller so the user can decide to allow or not the connection request. Apart from that, I would also like to propose one last modification to allow apps to defer connection requests (related with point 5 from comment 1). This will also require an addition to the "connect" app manifest field: "connect": ["keyword1": { "allowed-apps": "certified", "permission-details": "Allow app to do whatever keyword1 does", "defer": true }] where "defer" is a flag that indicates if the app can wait or not for handling the potential messages that will be sent within the connection. Once the application wakes up, it will be notified with the messages that were sent while it wasn't being executed. This will avoid waking up apps that doesn't really need to be woken up. We might need to add a getter to fetch this messages on demand. [1] https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest#type

(In reply to Mounir Lamouri (:mounir) from comment #2) > (In reply to Gene Lian [:gene] from comment #1) > > 1. We don't need |.registerConnections(...)| anymore. The |.connect(...)| > > will be in charge of popping up the app selector and directly return the > > selected apps in its callback. > > The idea is that the user will not be involved with selecting the > application so having an application selector like in Web Activities in > breaking that requirement. > > I think you should revisit the other changes based on that. As I said via IRC, I really don't understand how this can work in a secure way without user interaction :(. I might have misunderstood the API proposal.

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #6) > Thanks for the summary Gene! > > (In reply to Gene Lian [:gene] from comment #1) > > 6. Fernando suggested we should specify CRUD (i.e., > > Create/Read/Update/Delete) for each keyword in the app's manifest so that > > the receiver can decide which kinds of requesters are allowed to do the > > connection. Fernando will elaborate more on the design later. > > Thinking about this twice, we can probably go without it. The idea behind > this point is basically to allow apps to specify a security policy for each > keyword. I would like to reword this and propose a change in the app > manifest to specify the following: > > "connect": ["keyword1": { > "allowed-apps": "certified", > "permission-details": "Allow app to do whatever keyword1 does" > }, "keywordN": { > "allowed-apps": "installed", > "permission-details": "Allow app to do whatever keywordN does" > }] > > Where: > - "connect" is the list of keywords that the app can connect with, as in the > original proposal. With "keyword1" and "keywordN" as the names of these > keywords. > - "allowed-apps" is the minimum level of access (one of [1]) that an app > requires to communicate with the receiver via that specific keyword. So for > example an app declaring: > > "commlog": { > "allowed-apps": "privileged", > "permission-details": "Allow apps to send communication logs" > } > > will only receive connection requests if the caller of |connect("commlog", > {})| is a privileged app. I'm still confused about the need for such granular permissions. Are we building a RPC API or a Message Passing API? If it is a message passing API, can't the receiving app make much better decisions on this when it receives the message? > > Apart from that, I would also like to propose one last modification to allow > apps to defer connection requests (related with point 5 from comment 1). > This will also require an addition to the "connect" app manifest field: > > "connect": ["keyword1": { > "allowed-apps": "certified", > "permission-details": "Allow app to do whatever keyword1 does", > "defer": true > }] > > where "defer" is a flag that indicates if the app can wait or not for > handling the potential messages that will be sent within the connection. > Once the application wakes up, it will be notified with the messages that > were sent while it wasn't being executed. This will avoid waking up apps > that doesn't really need to be woken up. We might need to add a getter to > fetch this messages on demand. Is the purpose of the API "I'll advertise the interesting things I (this app) am doing", or "I'll make other apps do things for me". If it is the latter, then yes it does make sense to launch the other app (the receiver) if it is not running. What is a good use-case for defer? It would be great if a lot of the decisions around this API were updated on the wiki or in some mailing list thread, because it is a difficult API to understand right now.

(In reply to Nikhil Marathe [:nsm] from comment #8) Hi Nikhil! Sorry for the late reply. > > I'm still confused about the need for such granular permissions. Are we > building a RPC API or a Message Passing API? If it is a message passing API, > can't the receiving app make much better decisions on this when it receives > the message? > How would the receiving app make that decisions? Would you trust messages coming from an unknown source (i.e. a non-installed app)? How would you filter invalid messages? For the communications log example that I mentioned in comment 6, if this app listens for messages with the 'commlog' keyword coming from every unknown source it might consume messages such us "My beautiful spam message" or "Visit http://russianblondegirls.com" and show an interesting list of logs. As a third party app developer I would feel safer if I could limit the source of the messages that my app consumes. > > > > Apart from that, I would also like to propose one last modification to allow > > apps to defer connection requests (related with point 5 from comment 1). > > This will also require an addition to the "connect" app manifest field: > > > > "connect": ["keyword1": { > > "allowed-apps": "certified", > > "permission-details": "Allow app to do whatever keyword1 does", > > "defer": true > > }] > > > > where "defer" is a flag that indicates if the app can wait or not for > > handling the potential messages that will be sent within the connection. > > Once the application wakes up, it will be notified with the messages that > > were sent while it wasn't being executed. This will avoid waking up apps > > that doesn't really need to be woken up. We might need to add a getter to > > fetch this messages on demand. > > Is the purpose of the API "I'll advertise the interesting things I (this > app) am doing", > or "I'll make other apps do things for me". If it is the latter, then yes it > does make sense to launch the other app (the receiver) if it is not running. None of them entirely :) The way I see this API, it's purpose is more like: "I advertise myself as able to receive messages of type X" > What is a good use-case for defer? 'defer' will help to avoid performance issues when many (or not that many) apps listens for the same type of connection keyword. For example, two apps listening for the "twitternotification" keyword: - App A is a communication hub for all your social network activity. - App B is a watcher for Twitter unfollows like Sayonara iPhone app [1]. - App A doesn't need to be woken up with every new "twitternotification" that is sent and can probably wait for its next execution to consume all the messages received while it was not been executed, so it can fill its content with the new tweets and other Twitter activity. - App B might want to trigger a visual notification as soon as a "twitternotification" is received saying that you have lost a Twitter follower. In this case, app B needs to be woken up as soon as possible. > It would be great if a lot of the > decisions around this API were updated on the wiki or in some mailing list > thread, because it is a difficult API to understand right now. Absolutely! Once we get to an agreement, I'll update the wiki with the results of the discussions and with what we intend to implement. [1] http://www.sayonarapp.com/

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #9) > (In reply to Nikhil Marathe [:nsm] from comment #8) > Hi Nikhil! > > Sorry for the late reply. > > > > > I'm still confused about the need for such granular permissions. Are we > > building a RPC API or a Message Passing API? If it is a message passing API, > > can't the receiving app make much better decisions on this when it receives > > the message? > > > > How would the receiving app make that decisions? Would you trust messages > coming from an unknown source (i.e. a non-installed app)? How would you > filter invalid messages? What I meant was that you could do this filtering in the app code. In both cases, you'd need to update the receiving app to accept new senders, so I don't understand why it has to be in the manifest. > > For the communications log example that I mentioned in comment 6, if this > app listens for messages with the 'commlog' keyword coming from every > unknown source it might consume messages such us "My beautiful spam message" > or "Visit http://russianblondegirls.com" and show an interesting list of > logs. > > As a third party app developer I would feel safer if I could limit the > source of the messages that my app consumes. There are problems with a app-filter system. It depends a lot on what the receiver does with the data. But consider these 2 cases: 1) Lock screen Now Playing display - This initially filters to only display from the inbuilt music player. - We have a few apps on the app store that play from different sources, like Internet Radio or subscription based services or a music player that plays local music with some really fancy effects or something. I'm sure the user would like these secondary players to show their Now Playing on the lock screen, so the app will accept all 'trackchanged' requests. This does mean it is susceptible to receiving 'trackchanged' from a rogue app, but there is nothing it can do about it. 2) History app This wants to keep track of all my history. And in that case I *do* want it to log russianblondegirls.com, even though its NSFW, unless I'm running it from a private browsing session, in which case the *sender* wouldn't send the message. > > > > > > > Apart from that, I would also like to propose one last modification to allow > > > apps to defer connection requests (related with point 5 from comment 1). > > > This will also require an addition to the "connect" app manifest field: > > > > > > "connect": ["keyword1": { > > > "allowed-apps": "certified", > > > "permission-details": "Allow app to do whatever keyword1 does", > > > "defer": true > > > }] > > > > > > where "defer" is a flag that indicates if the app can wait or not for > > > handling the potential messages that will be sent within the connection. > > > Once the application wakes up, it will be notified with the messages that > > > were sent while it wasn't being executed. This will avoid waking up apps > > > that doesn't really need to be woken up. We might need to add a getter to > > > fetch this messages on demand. > > > > Is the purpose of the API "I'll advertise the interesting things I (this > > app) am doing", > > or "I'll make other apps do things for me". If it is the latter, then yes it > > does make sense to launch the other app (the receiver) if it is not running. > > None of them entirely :) The way I see this API, it's purpose is more like: > "I advertise myself as able to receive messages of type X" So its pub-sub :) > > > What is a good use-case for defer? > > 'defer' will help to avoid performance issues when many (or not that many) > apps listens for the same type of connection keyword. > > For example, two apps listening for the "twitternotification" keyword: > - App A is a communication hub for all your social network activity. > - App B is a watcher for Twitter unfollows like Sayonara iPhone app [1]. > - App A doesn't need to be woken up with every new "twitternotification" > that is sent and can probably wait for its next execution to consume all the > messages received while it was not been executed, so it can fill its content > with the new tweets and other Twitter activity. > - App B might want to trigger a visual notification as soon as a > "twitternotification" is received saying that you have lost a Twitter > follower. In this case, app B needs to be woken up as soon as possible. So would App A declare 'defer'. So in this case, *if* App B is installed, it gets the message, otherwise App A gets the message. What happens in the case of multiple apps, none of which declare defer? I assume all of them get woken up. Considering that every message will be very specific, I'd expect only a small number of listeners for any kind of message, so this doesn't make much sense. In the specific case you've highlighted above, wouldn't App A just NOT declare any receiver, since it can just wait until it is launched.

(In reply to Nikhil Marathe [:nsm] from comment #12) > > > > How would the receiving app make that decisions? Would you trust messages > > coming from an unknown source (i.e. a non-installed app)? How would you > > filter invalid messages? > > What I meant was that you could do this filtering in the app code. I don't think you'd have enough information in the receiver app code to verify if the message is worth trusty or not. All that the receiver knows is the origin of the sender (which can be a hash for packaged apps), the keyword responsible of the connection and the content of the messages received within that connection. Note that the origin isn't enough to verify the source of the messages. > In both > cases, you'd need to update the receiving app to accept new senders, so I > don't understand why it has to be in the manifest. > Why would you need to update the receiver app to accept new senders? If the receiver's app manifest declares itself to be able to receive messages within connections done under the keyword 'X' *only* with, for example, certified apps, it won't need to update its code, unless the developer wants to modify the apps level of confidence. Are you by any chance suggesting a whitelist/blacklist of origins in the messages handlers code? I don't think that scales well and it certainly won't work for packaged apps that does not specify an origin. > > > > For the communications log example that I mentioned in comment 6, if this > > app listens for messages with the 'commlog' keyword coming from every > > unknown source it might consume messages such us "My beautiful spam message" > > or "Visit http://russianblondegirls.com" and show an interesting list of > > logs. > > > > As a third party app developer I would feel safer if I could limit the > > source of the messages that my app consumes. > > There are problems with a app-filter system. It depends a lot on what the > receiver does with the data. But consider these 2 cases: > > 1) Lock screen Now Playing display > - This initially filters to only display from the inbuilt music player. > - We have a few apps on the app store that play from different sources, > like Internet Radio or subscription based services or a music player that > plays local music with some really fancy effects or something. > I'm sure the user would like these secondary players to show their Now > Playing on the lock screen, so the app will accept all 'trackchanged' > requests. This does mean it is susceptible to receiving 'trackchanged' from > a rogue app, but there is nothing it can do about it. > Correct me if I am wrong, but it feels like you are suggesting that the developer of an app will need to update its apps code to add new origins of new music apps added to the marketplace. Is that correct? This use case can be done just by adding for example: "connect": ["trackchanged": { "allowed-apps": "privileged", "permission-details": "Allow app to add 'Now playing' information to the lock screen", "defer": false }] That will provide the receiver app the confidence that there is someone else (the app store signing the app) "watching" the senders of the "trackchanged" messages. A rough app will need to go through the app store review process and may be removed from the store if an abuse is detected. That feels like safer than forcing a developer to keep its own list of allowed origins as the only security mechanism. An app can of course still filter the messages by origin. > 2) History app > This wants to keep track of all my history. And in that case I *do* want it > to log russianblondegirls.com, even though its NSFW, unless I'm running it > from a private browsing session, in which case the *sender* wouldn't send > the message. This can be done like: "connect": ["history": { "allowed-apps": "web", "permission-details": "Allow app to send browsing history", "defer": true }] > > > > None of them entirely :) The way I see this API, it's purpose is more like: > > "I advertise myself as able to receive messages of type X" > > So its pub-sub :) > Yes :) > So would App A declare 'defer'. Yes, in the above example, App A declares defer true in its manifest and App B false (it can be the default though). > So in this case, *if* App B is installed, it > gets the message, otherwise App A gets the message. Both apps will get the message (if they are installed, of course). I'm not sure that I am understanding you properly, but an app cannot intercept a message and stop its propagation to other apps. > What happens in the case > of multiple apps, none of which declare defer? I assume all of them get > woken up. Yes. > Considering that every message will be very specific, I'd expect > only a small number of listeners for any kind of message, so this doesn't > make much sense. > Well, that's an opinion :) I prefer to be on the safe side of avoiding an app to consume CPU and memory when it is not actually needed :) > In the specific case you've highlighted above, wouldn't App A just NOT > declare any receiver, since it can just wait until it is launched. How would it receive "twitternotification" messages then?

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #13) > (In reply to Nikhil Marathe [:nsm] from comment #12) > > > > > > How would the receiving app make that decisions? Would you trust messages > > > coming from an unknown source (i.e. a non-installed app)? How would you > > > filter invalid messages? > > > > What I meant was that you could do this filtering in the app code. > > I don't think you'd have enough information in the receiver app code to > verify if the message is worth trusty or not. All that the receiver knows is > the origin of the sender (which can be a hash for packaged apps), the > keyword responsible of the connection and the content of the messages > received within that connection. Note that the origin isn't enough to verify > the source of the messages. So what sort of information would FxOS use in Gecko code to decide that a message is safe? > > > In both > > cases, you'd need to update the receiving app to accept new senders, so I > > don't understand why it has to be in the manifest. > > > > Why would you need to update the receiver app to accept new senders? If the > receiver's app manifest declares itself to be able to receive messages > within connections done under the keyword 'X' *only* with, for example, > certified apps, it won't need to update its code, unless the developer wants > to modify the apps level of confidence. > > Are you by any chance suggesting a whitelist/blacklist of origins in the > messages handlers code? I don't think that scales well and it certainly > won't work for packaged apps that does not specify an origin. This is what I thought it implied. But if the default case is that all apps are allowed to send a message to this app, then yes, no updates are required. What I'm looking for, is examples of messages that DO require this kind of 'security'. The commlog example wasn't very convincing because it was doing its job, of logging all communications, even if those were unsavoury in some cases. > > So would App A declare 'defer'. > > Yes, in the above example, App A declares defer true in its manifest and App > B false (it can be the default though). > > > So in this case, *if* App B is installed, it > > gets the message, otherwise App A gets the message. > > Both apps will get the message (if they are installed, of course). I'm not > sure that I am understanding you properly, but an app cannot intercept a > message and stop its propagation to other apps. I don't understand the use of defer then. In comment 9, you said App A would *NOT* be woken up since it has declared defer=true.

(In reply to Nikhil Marathe [:nsm] from comment #14) > So what sort of information would FxOS use in Gecko code to decide that a > message is safe? > Basically, the information provided by the receiver in its manifest. Gecko will filter a message according to the description provided by the receivers of this kind of messages. Where this description is the keyword associated with the message and the minimum level of access of the sender of the message. Receivers will rely on the same mechanisms used to decide if an app is trusty enough to use an API or not. (In fact, I am thinking about suggesting the same kind of security mechanism for senders... but I'll add my thoughts to the wiki page, as I don't want to add more confusion here :) ). > This is what I thought it implied. But if the default case is that all apps > are allowed to send a message to this app, then yes, no updates are required. > > What I'm looking for, is examples of messages that DO require this kind of > 'security'. The commlog example wasn't very convincing because it was doing > its job, of logging all communications, even if those were unsavoury in some > cases. > Of course it is doing its job :D! But it is certainly undesired to receive other things but valid and harmless communication logs. > I don't understand the use of defer then. In comment 9, you said App A would > *NOT* be woken up since it has declared defer=true. App A won't be woken up when the connection request is done and the sender start sending messages (if the user allows the connection) *but* it will receive the sent messages during it next execution. Gecko will need to save a queue of messages sent to a receiver that doesn't want to be woken up. (We still need to decide how the receiver will fetch these messages though. I have some thoughts that I'll add to the wiki). Since this is turning into a quite confusing discussion I'll dump this proposal in the WebAPI wiki page tomorrow.

Not directly related to the above discussions. I'd prefer adding another change for the manifest registrations. The original proposal is: { 'connect': [ 'keyword1', 'keyword2' ] } I'd prefer the following one (merged with Fernando's proposal): { 'connect': { 'keyword1': { 'handling_path': '/handler1.html', 'allowed_app_type': 'web', 'description': 'Do something for keyword1 connection', 'defer': true }, 'keyword2': { 'handling_path': '/handler2.html', 'allowed_app_type': 'certified', 'description': 'Do something for keyword2 connection', 'defer': false } } } ========== Note: 1. Lessons learned from system message [1]. The app developers will like to handle different connections by different pages. If 'handling_path' is absent, use the 'launch_patch' as default. 2. It'd be better to use "foo_bar" instead of "foo-bar" for property names because |object.foo-bar| is illegal to use. [1] https://developer.mozilla.org/en-US/docs/Web/Apps/Manifest?redirectlocale=en-US&redirectslug=Apps%2FManifest#messages

Summarizing, I have my own thoughts about these topics: 1- Explicit permissions related with the application level. First, I understand both points of view from Fernando and Nikhil. Nikhil is suggesting to implement filtering in the application while Fernando tries to move this to Gecko. Note anyway that Fernando's idea of explicitly indicate the minimum level of allowed applications lets Nikhil alternative by specifying the minimum type of application possible, then inspecting the origin inside the code application. In the other hand, Nikhil's approach does not allow all the possible cases from Fernando's idea, for instance, the case in which a certified application wants to filter messages from certified applications only. Consider the next US: US: Cost Control **only** trust on messages from Gaia's SMS app. SMS application communicates balance SMS to Cost Control application via "new-sms" keyword. Other non-certified applications could send deceiving "new-sms" messages but Cost Control application will only listen for those coming from a certified application. In the same way, INMHO, it make no sense to ask for a "reviewed" tag because each market could have its own reviewing policy but what make sense is to ask for the "installation origin". Consider this US: US: There is a marketplace called "nospamhere.com" where only deeply reviewed applications (you can ensure they don't spam the user anyway) land. Then, as application developer, I've created a background carrousel and want to accept messages from applications installed from that market because I know they never send advertising content. So, **safety is well defined under a context**, this context can be the origin, the installation source, the developer... Why not to return the Manifest/App/CustomMessageOrigin object instead of returning only the origin of the communication? A possible structure for this CustomMessageOrigin could be: CustomMessageOrigin = { origin: "myapplication.com", installationSource: "nospamhere.com", isCertified: false } (Consider returning the App object or the Manifest object as well) With this, we can remove the explicit permissions and we have enough context for "security" to mean something. --- 2- The use of `defer` keyword. The `defer` keyword seems critical to me. I thought it should be mandatory and require your application to have a special permission to turn `defer` to `false` as it is battery/memory friendly to minimize application wake ups. Once awake, an application can ask for pending messages and receive them as a whole, if needed. A US for this: US: The social hub does not need to be woke up every time a message is received. Only when the user open the application it receives the pending messages. After discussing with Fernando and Antonio, it is true you could require to be awaken every time a message is received. US for this: US: I have an application that perform face recognition, I need to be woke up as soon as possible in order to perform the calculations and return the answer to the demanding application. But, from my point of view, we should default `defer` to true. And make it `false` explicitly. Another important point to remark is how to deliver pending messages. Apart from implementation details, it should be independent on the number of messages. I.e. we could use a `cursor` with a property saying the number of pending messages, then allowing the application to consume the `cursor` progressively or not. Otherwise it could result in a delayed start-up and slow initialization due the cost of transmitting the messages from the main process to the child. --- 3- Sending restrictions One important thing is to allow some kind of constrain specification about what applications will be the receivers of a message I'm sending. So, when opening a connection for a keyword, it would be nice if I could provide some kind of restriction replicating the CustomMessageorigin such as: DeliveryConstrains = { "origin": "someapp.com", "installationSource": "trustedapps.com", "mustBeCertified": "true" } Removing a criterion is like removing the constrain and they could accept an array instead o f a literal to specify a list of options. --- 4- Some nomenclature issues. Here, I think we totally agree: this is pub-sub. From a certain point of view, we are providing our custom `system-messages` where the type is the keyword. So I suggest to change the nomenclature accordingly and instead of use `connections`, simply use `subscriptions`. Example: "subscriptions" : { "keyword1": { "details": "Do whatever keyword1 does", "defer": false }, "keyword2": { "details": "Do whatever keyword2 does", "defer": false } }

This bug blocks bug 891024. We really need inter-app api in order to implement that properly. We need bug 891024 to be feature complete by 9/16, so we'd like to see this bug be feature complete by 8/16. As a total hack, we're going to cobble something together using the settings API, which we'll replace with the mechanism from this bug, once it becomes available. Having a preliminary version of the API available, even if all of the permissions stuff hasn't been finialized yet, would be extremely useful.

Thanks for all the suggestions. I'll try to work out an initiative version ASAP. I'd believe there are too many details to be fully taken care of in the first place. Thinking about how much time we've spent refining the system message/activities! Let's move on and learn from mistakes.

With apologies for diving in and bug-crashing, but are y'all familiar with Android's broadcast intents? Manifest- and programmatically-controlled, permissions and signing, all the usual good stuff you get with intents, etc., and extendable to OrderedBroadcast if you need chained handling and return values.

Hi Mounir, These patches are just WIPs. Could you please take a glance on that to make sure we're in the right way? Please also check [1] for the latest API proposal made by Fernando. Thanks! [1] https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal

The way of using constructor for webidl in MessagePort and instanizing an MessagePort is wrong. I've already fixed in my local patches.

Hi Mounir, Although the patches are tentatively marked as WIP, they are actually working well. I'm aligning them with the latest proposal [1], refining the messaging mechanism and trying to work out test cases. Could you please return some feedback at the same time? Thanks! [1] https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal

Comment on attachment 783128 [details] [diff] [review] Part 1, IDL (WIP) Review of attachment 783128 [details] [diff] [review]: ----------------------------------------------------------------- Please, implement MessagePort in another bug. This is a DOM feature, it should not be dom/apps/.

(In reply to Mounir Lamouri (:mounir) from comment #44) > Please, implement MessagePort in another bug. This is a DOM feature, it ^^^^^^^^^^^^^^ Do you mean in another directory? > should not be dom/apps/.

Comment on attachment 783128 [details] [diff] [review] Part 1, IDL (WIP) Review of attachment 783128 [details] [diff] [review]: ----------------------------------------------------------------- Agree with Mounir. We may have too many MessagePort uses right now :) Shared Workers (bug 643325) also introduces a MessagePort.webidl, and the interfaces don't match exactly. Please coordinate with :bent

Comment on attachment 783129 [details] [diff] [review] Part 2, manifest registry (WIP) Review of attachment 783129 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/interfaces/apps/nsIInterAppCommService.idl @@ +11,5 @@ > +[scriptable, uuid(8f95ab30-e7c4-11e2-bd65-3fb128e2b5ea)] > +interface nsIInterAppCommService : nsISupports > +{ > + void registerConnection(in DOMString aKeyword, > + in nsIURI aPageURI, At location of use in Webapps.jsm, this is handler URI. Please change that here and add a relevant comment about exactly what page receives the message, because other APIs like Alarm and Push use aPageURI to signify the page that made the API call itself, and not the message receiver.

Comment on attachment 783131 [details] [diff] [review] Part 3, connect & getConnections (WIP) Review of attachment 783131 [details] [diff] [review]: ----------------------------------------------------------------- I'll add more comments once I read the alt proposal. ::: b2g/chrome/content/shell.js @@ +610,5 @@ > + callerID: data.callerID, > + keyword: data.keyword, > + manifestURL: data.manifestURL, > + appsToSelect: data.appsToSelect }); > +}, 'inter-app-comm-select-app', false); Has this prompt been implemented? Sorry, I can't find it in the code. The prompt will need a timeout after which to reject the connect() call for that app, because unlike most other prompts, this one may occur even when the user is not using the device. Please ask for UX on this. ::: dom/apps/src/InterAppCommService.js @@ +38,3 @@ > > this._registeredConns = {}; > + this._allowedConns = {}; allowedConnections and registeredConnections please. @@ +38,4 @@ > > this._registeredConns = {}; > + this._allowedConns = {}; > + this._promptCallers = {}; Can you add a comment for what each of the above three fields hold? Including sample object literals. @@ +196,5 @@ > + break; > + } > + } > + }, > + I'll have to go through this once again after I read the alt proposal carefully. The code is fine. It is extremely similar to system messages in implementation which only convinces me more that we should subsume system messages into this API. @@ +204,5 @@ > + Services.obs.removeObserver(this, "inter-app-comm-select-app-result"); > + kMessages.forEach(function(aMsg) { > + ppmm.removeMessageListener(aMsg, this); > + }, this); > + ppmm = null; Good catch freeing up ppmm. If possible please run this on desktop firefox and make sure it doesn't introduce leaks :) ::: dom/apps/src/Webapps.js @@ +498,5 @@ > + cpmm.sendAsyncMessage("Webapps:Connect", > + { keyword: aKeyword, > + manifestURL: this.manifestURL, > + oid: this._id, > + requestID: this.getRequestId(aResolver) }); Please modify DOMRequestIpcHelper in a separate bug to add methods |getPromiseResolverId()| and |getPromiseResolver(aId)|. I don't like to trust JS methods to be overloaded since the lack of type checking will bite at some point. I was intending to do this for converting SimplePush to Promises, but you got here first :) Since DOMRequestIpcHelper already has a reference to the window, you can refactor the API to be: return this.getPromise(function(aRequestID) { cpmm.sendAsyncMessage(..., { ..., requestID: aRequestID }); }) Does that sound acceptable? You can internally continue to use the same lookup table for both DOMRequests and PromiseResolvers at this point, since the table itself doesn't care whats inside it, but put an explicit comment in that file. It would be great if you also added a FIXME to DOMRequestIpcHelper, with a bug, to eventually deprecate the DOMRequest methods and rename it to PromiseResolverIpcHelper or similar. Thanks! @@ +661,5 @@ > + let messagePorts = []; > + messagePortIDs.forEach(function(aPortID) { > + messagePorts.push(new this._window.MessagePort(aPortID, true)); > + }, this); > + req.resolve(messagePorts); You'll have to use takePromiseResolver() for these set of receivers.

Comment on attachment 783132 [details] [diff] [review] Part 4, connection wrapper (WIP) Review of attachment 783132 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/ConnectionWrapper.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + Please comment explaining the purpose of ConnectionWrapper. Is this used to convert a system message to something that can be sent over a MessagePort?

(In reply to Nikhil Marathe [:nsm] from comment #46) > Agree with Mounir. We may have too many MessagePort uses right now :) > Shared Workers (bug 643325) also introduces a MessagePort.webidl, and the > interfaces don't match exactly. Please coordinate with :bent Thanks for sharing the info. I think I intend to rename the MessagePort to InterAppCommPort for the inter-app purpose, since they are quite different in many aspects, including the way of how it's used. For example, MessagePort uses close() to turn off the port and InterAppCommPort uses stop() to pause the messaging (the port can be restarted later). The most important thing is InterAppCommPort is more like a messaging mechanism for the communication between apps (crossing processes), not workers. I think they're too different to be considered under the same point of view. Also, the Inter-App Comm API is specifically aimed for the b2g purpose in the initiative step and targeted at the end of August (it's a P1 API). We've already had quite a lot discussions and work done here over months. We should keep moving on without being blocked by others.

(In reply to Nikhil Marathe [:nsm] from comment #49) > Please comment explaining the purpose of ConnectionWrapper. > Is this used to convert a system message to something that can be sent over > a MessagePort? Please see [1]. It's more like a pluginable way to wrap different types of system message based on that content window. :) [1] http://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageManager.js#76

(In reply to Nikhil Marathe [:nsm] from comment #48) > Has this prompt been implemented? Sorry, I can't find it in the code. It's at bug 897169.

Hi Mounir, how do you feel about comment #50? Fernando and I are also hoping to provide onstart/onstop event handler for InterAppCommPort, which helps the sender/receiver be aware of the port (on the other end) is started/stopped or not, so that it can explicitly know when is valid to post messages. Please see the examples at [1]. This mechanism is also different from MessagePort, because we need a way to know when a port is successfully delivered to the remote app by the System Message, and when an app is ready to receive messages thorough that port (i.e. the remote app can be OOM'ed or manually killed at any time points). I'd prefer defining another InterAppCommPort for the inter-app communication specific purpose, which can handle the interaction between apps more properly. [1] https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal#Lockscreen_and_Music

My feeling is that we should use the momentum to implement MessagePort in Gecko and maybe extend them for Inter App Communication. Anyway, you guys should speak with bent regarding this.

(In reply to Gene Lian [:gene] from comment #50) > (In reply to Nikhil Marathe [:nsm] from comment #46) > > Agree with Mounir. We may have too many MessagePort uses right now :) > > Shared Workers (bug 643325) also introduces a MessagePort.webidl, and the > > interfaces don't match exactly. Please coordinate with :bent > > Thanks for sharing the info. I think I intend to rename the MessagePort to > InterAppCommPort for the inter-app purpose, since they are quite different > in many aspects, including the way of how it's used. For example, > MessagePort uses close() to turn off the port and InterAppCommPort uses > stop() to pause the messaging (the port can be restarted later). The most > important thing is InterAppCommPort is more like a messaging mechanism for > the communication between apps (crossing processes), not workers. I think > they're too different to be considered under the same point of view. Also, > the Inter-App Comm API is specifically aimed for the b2g purpose in the > initiative step and targeted at the end of August (it's a P1 API). We've > already had quite a lot discussions and work done here over months. We > should keep moving on without being blocked by others. I think this is just a API naming difference. I don't see the use case of having a stop() which causes a paused connection. As far as the messaging between apps vs. workers is concerned, that should be carried in information in the message or similar. As for it being a B2G only API, I'd like to stop creating APIs which are device specific unless they need to be, and have a short timespan because in the end the API turns out to be non-ideal. With system messages and app launching in the background, we already need a modified API soon. I don't want to repeat those mistakes. If a P1 blocker requires this and it is only for certified apps right now, I'd drop all the elaborate rules logic for now and land a simple API that is available only to certified apps, but then I'd follow up with a blocker for 1.3 that made this API public. Having certified app only APIs does not help in opening up the platform to third party developers.

Additional comments on the alt API: 1) I found the API very confusing about its role as a pub-sub event-based protocol vs. a two-way communication channel (TCP sockets). Like in a event-based protocol (which is strictly one-way), the API uses a keyword for communication, but then it hands around MessagePorts which can presumably be used for two-way communication. The music lock-screen app uses (and states in a comment) two keywords, one from player to lock screen to display meta-data, and one from lock screen to player to control the player. In addition the original page [1] states that this is not a caller-handler API, but in the immediate next sentence says it is a 1:n api rather than a 1:1. Please make this clear in the spec and in the WebIDL files. [1]: https://wiki.mozilla.org/WebAPI/Inter_App_Communication 2) Am I correct in the assumption that to connect to other apps, my app will do the following? mozApps.getSelf().onsuccess = function (myApp) { myApp.connect('foo-keyword').then(...); } Would it make more sense to expose the Inter-App API on navigator rather than Application. Or even if it was on Application, have it in a separate object, which conveys that this is an IPC system, because right now it doesn't make it obvious where this 'connect()' comes from. 3) I'd split up broadcast and two-way communication. What I mean is, a message sent by an application either: a) Two-way communication - In this situation the publisher knows exactly who the subscriber is (e.g. Gaia Music Player), or at least the class of the subscriber (any of Gaia Music Player, Pandora, FM radio). In this context, the keyword approach, where the subscriber's manifest declares its public interface, can be used in a similar format by the publisher. Incidentally I'd replace |'connections'| with |'interfaces'| or something, indicating that the receiver supports a certain public interface. An example is a 'Bookmarks API', where the browser, an instapaper app, a pinterest app would all return their set of bookmarks to an app which requested it. Bookmark visualizer app: getSelf().systemBus.connect('bookmarks', /* optional ruleset */).then(function(ports) { ports.forEach(function(port) { port.sendMessage('get-all'). port.addMessageListener('message', function(data) { // do something with data.bookmarks. }) }), function() { /* no app granted permission */ } }) Subscriber manifest: 'interface': { 'bookmarks': <ruleset> } In this case, the systemBus would also have a onConnected, and onDisconnected so the app knows when a new listener/sender has joined, and get a message port for it. b) Indicate some event happened - In this case the publisher doesn't care about replies. to prevent unwanted apps from listening to the message, the publisher manifest is like: 'publishes': { 'playback-pause': <ruleset> } called as: getSelf().systemBus.broadcast('playback-pause', /* optional data */, /* optional ruleset */) The subscriber manifest is: 'subscribes' : { 'playback-pause': <ruleset> } Similarly the song metadata would be sent as: getSelf.systemBus.broadcast('playback-changed', { artist: "The Beatles", ... }, /* optional ruleset */) In this case the publisher does not need to know which apps are listening.

Hi Nikhil, I think Fernando's proposal is pretty much following the original design made by Mounir and others. The proposal you're suggesting (specifically for broadcasting part) sounds like a new mechanism. We need to talk to Mounir first since Fernando won't be available until the end of August. However, I'm a bit worrying about we would go through the API discussion cycle again. (In reply to Nikhil Marathe [:nsm] from comment #56) > In addition the original page states that this is not a caller-handler API, > but in the immediate next sentence says it is a 1:n api rather than a 1:1. > Please make this clear in the spec and in the WebIDL files. I think this API is definitely a kind of 1:n mechanism but it doesn't work like a caller-handler way. Instead, the desired design is aimed for n-port and 2-way communications. That is, the publisher will get one port for each app that subscribes for the same keyword, and each port can be used for the 2-way communication between two apps only. > 2) Am I correct in the assumption that to connect to other apps, my app will > do the following? > > mozApps.getSelf().onsuccess = function (myApp) { > myApp.connect('foo-keyword').then(...); > } Yes, we're hoping use that in this way. > > Would it make more sense to expose the Inter-App API on navigator rather > than Application. Or even if it was on Application, have it in a separate > object, which conveys that this is an IPC system, because right now it > doesn't make it obvious where this 'connect()' comes from. I don't see much benefit by delegating the APIs to a separate object which is still under mozIDOMApplication. Putting it under navigator even sounds better. However, since it's designed for the inter-*app* communication, it still makes sense to me to design it as an app-based API. > 3) I'd split up broadcast and two-way communication. In short, I think the two-way communication has already contained the scenario of broadcasting. In my opinion, broadcasting is just like we're using the two-way communication as an one-way channel. That is, the subscriber doesn't attempt to respond any message back to the publisher. Why do we want to treat them separately with different API/manifest syntax?

(In reply to Gene Lian [:gene] from comment #57) > Hi Nikhil, > > I think Fernando's proposal is pretty much following the original design > made by Mounir and others. The proposal you're suggesting (specifically for > broadcasting part) sounds like a new mechanism. We need to talk to Mounir > first since Fernando won't be available until the end of August. However, > I'm a bit worrying about we would go through the API discussion cycle again. > > (In reply to Nikhil Marathe [:nsm] from comment #56) > > In addition the original page states that this is not a caller-handler API, > > but in the immediate next sentence says it is a 1:n api rather than a 1:1. > > Please make this clear in the spec and in the WebIDL files. > > I think this API is definitely a kind of 1:n mechanism but it doesn't work > like a caller-handler way. Instead, the desired design is aimed for n-port > and 2-way communications. That is, the publisher will get one port for each > app that subscribes for the same keyword, and each port can be used for the > 2-way communication between two apps only. > > > 2) Am I correct in the assumption that to connect to other apps, my app will > > do the following? > > > > mozApps.getSelf().onsuccess = function (myApp) { > > myApp.connect('foo-keyword').then(...); > > } > > Yes, we're hoping use that in this way. > > > > > Would it make more sense to expose the Inter-App API on navigator rather > > than Application. Or even if it was on Application, have it in a separate > > object, which conveys that this is an IPC system, because right now it > > doesn't make it obvious where this 'connect()' comes from. > > I don't see much benefit by delegating the APIs to a separate object which > is still under mozIDOMApplication. Putting it under navigator even sounds > better. However, since it's designed for the inter-*app* communication, it > still makes sense to me to design it as an app-based API. Only because the call to connect() isn't being explicit in intent about what this foo-keyword is without looking at the documentation. When using navigator.contacts or something, it is explicit that this has something to do with contacts. Similarly the Inter-app API should make it clear that this |connect()| is between two apps. > > > 3) I'd split up broadcast and two-way communication. > > In short, I think the two-way communication has already contained the > scenario of broadcasting. In my opinion, broadcasting is just like we're > using the two-way communication as an one-way channel. That is, the > subscriber doesn't attempt to respond any message back to the publisher. Why > do we want to treat them separately with different API/manifest syntax? Yes it contains it, but again it is a matter of capturing intent in the API and simplifying things. The manifest syntax can probably stay the same, but the async setup of a MessagePort is too complicated for sending a one way, one datum message, and doesn't capture the 'fire-and-forget' nature of it at all.

Comment on attachment 783133 [details] [diff] [review] Part 5, post message (WIP) Review of attachment 783133 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/InterAppCommService.js @@ +310,5 @@ > + } > + case "child-process-shutdown": { > + // TODO We should fire MessagePort:OnClose to the other end. > + break; > + } MessagePort in the worker spec has some queueing semantics linked to start(). Does this stick to that? ::: dom/webidl/MessagePortEvent.webidl @@ +9,5 @@ > +[JSImplementation="@mozilla.org/dom/message-port-event;1", > + Constructor(DOMString type, optional MessagePortEventInit eventInitDict)] > +interface MessagePortEvent : Event { > + readonly attribute any data; > +}; http://www.whatwg.org/specs/web-apps/current-work/#messageevent ? It is specifically meant for cross-document messaging, which is what Inter-app communication is.

(In reply to Nikhil Marathe [:nsm] from comment #58) > (In reply to Gene Lian [:gene] from comment #57) > > > 3) I'd split up broadcast and two-way communication. > > > > In short, I think the two-way communication has already contained the > > scenario of broadcasting. In my opinion, broadcasting is just like we're > > using the two-way communication as an one-way channel. That is, the > > subscriber doesn't attempt to respond any message back to the publisher. Why > > do we want to treat them separately with different API/manifest syntax? > > Yes it contains it, but again it is a matter of capturing intent in the API > and simplifying things. > The manifest syntax can probably stay the same, but the async setup of a > MessagePort is too complicated for sending a one way, one datum message, and > doesn't capture the 'fire-and-forget' nature of it at all. If the publisher wants to do one-way communication, it doesn't need to set its onmessage event handler for its port, thus making it exactly like broadcasting. My feeling is, broadcasting is more like a bonus mechanism to have a simpler syntax to do one-way communication (i.e. we don't really need a port for each channel between two apps). I agree with you it would be nicer to have a special syntax for broadcasting but it doesn't block most of the scenarios which still need two-way communication. Needless to say what's happening if the publisher wants to communicate with some subscribers with two-way but to others with one-way by using the same syntax? I'd suggest let's keep working on the design of two-way communication, which should be able to satisfy all kinds of user cases for this moment. In the future, we can then extend the API to be capable of broadcasting by designing a simpler and better syntax. In the future, if any app wants to do fire-and-forget broadcasting for sure, it can further switch to the new syntax with backward-compatibility.

As Nikhil and Mounir suggested, I'm trying to talk to Bent to see how we can extend the MessagePort in the shared worker for Inter-App Communication. Btw, I'm joining the FFOS-Comm Work Week in Paris right now and working on some SMS/MMS issues. I might not have pretty much bandwidth working on this bug during this week. Will try to catch up!

Will upload new patches next Monday.

These patches are working well but hope to add more comments, refine the wikipage and fire follow-up bugs before asking for reviews.

Hi Mounir, Please refer to [1] to review the IDLs. Thanks! Per off-line discussion, after the MessagePort is done implemented at Bug 643325, we will start to refactorize the common logic of both Inter-App Communication and Shared Worker, in this way we won't be blocked by the MessagePort implementation of Shared Worker. For now, we hope to design an MozInterAppMessagePort to meet the timeline (by the end of August), which still follows exactly the same interface and semantic as the MessagePort is. In the future we can then align it back to MessagePort with backward compatibility. Fired Bug 907060 to keep track of the further work. [1] https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal ----- One issue needs your decision: Nikhil used to suggest we should put connect()/getConnections() under getSelf().systemBus to have a better semantic like: getSelf().systemBus.connect() getSelf().systemBus.getConnections() instead of getSelf().connect() getSelf().getConnections() However, this doesn't follow the original design made by you and others. Do we need to do that or it's just fine?

Hi Nikhil, I don't quite understand what Comment #47 says. Could you please explicitly point out what I should fix? Thanks!

Addressing comment #48.

Addressing comment #49.

Addressing comment #59. To meet the timeline, we hope to work out an InterAppMessagePort first, which still follows exactly the same interface and semantic like MessagePort in the Shared Worker. When bug 643325 is done implemented, we can then start to refactorize the common part with backward compatibility and diverge the implementation of postMessage() to deal with different kinds of messaging.

(In reply to Nikhil Marathe [:nsm] from comment #47) > Comment on attachment 783129 [details] [diff] [review] > Part 2, manifest registry (WIP) > > Review of attachment 783129 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/interfaces/apps/nsIInterAppCommService.idl > @@ +11,5 @@ > > +[scriptable, uuid(8f95ab30-e7c4-11e2-bd65-3fb128e2b5ea)] > > +interface nsIInterAppCommService : nsISupports > > +{ > > + void registerConnection(in DOMString aKeyword, > > + in nsIURI aPageURI, > > At location of use in Webapps.jsm, this is handler URI. Please change that > here and add a relevant comment about exactly what page receives the > message, because other APIs like Alarm and Push use aPageURI to signify the > page that made the API call itself, and not the message receiver. I meant that 'aPageURI' is pretty ambiguous about whether that page indicates the page that called this IDL method (which is the term used in PushService) or the page that is to receive the system message (as in Alarms). We should fix those too. But here if you could call it aHandlerURI or aHandlerPageURI that would make it more obvious that it was the receiving page. Also please match the term in the IDL with the term you use in the Webapps.jsm list of entrypoint registrations.

Comment on attachment 792771 [details] [diff] [review] Part 1, IDL, V3 Review of attachment 792771 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/InterAppMessagePort.manifest @@ +1,2 @@ > +component {c66e0f8c-e3cb-11e2-9e85-43ef6244b884} InterAppMessagePort.js > +contract @mozilla.org/dom/inter-app-message-port;1 {c66e0f8c-e3cb-11e2-9e85-43ef6244b884} Could you roll this file into InterAppConnection.manifest? ::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl @@ +95,5 @@ > + * Inter-App Communication APIs. > + * > + * https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal > + */ > + nsISupports connect(in DOMString keyword, [optional] in jsval rules); Please add a comment about this returning a Promise. @@ +96,5 @@ > + * > + * https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal > + */ > + nsISupports connect(in DOMString keyword, [optional] in jsval rules); > + nsISupports getConnections(); and here. ::: dom/webidl/InterAppConnection.webidl @@ +1,4 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + Add comment with link to alt spec please. The standard is something like '... the origin of this WebIDL file is ...' with a link to a W3 spec. @@ +8,5 @@ > + readonly attribute DOMString keyword; > + readonly attribute DOMString publisher; > + readonly attribute DOMString subscriber; > + > + [Throws] Throws not required. ::: dom/webidl/InterAppConnectionRequest.webidl @@ +1,4 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + Comment as before. ::: dom/webidl/InterAppMessagePort.webidl @@ +10,5 @@ > +// we can then align it back to MessagePort with backward compatibility. > + > +[Constructor, JSImplementation="@mozilla.org/dom/inter-app-message-port;1"] > +interface MozInterAppMessagePort : EventTarget { > + [Throws] JS implemented WebIDL does not require [Throws], it is implicit.

Gene, please also file a bug on tests for this API. We can land the API due to time constraints, but tests should be the next priority.

Comment on attachment 792772 [details] [diff] [review] Part 2, manifest registry, V3 Review of attachment 792772 [details] [diff] [review]: ----------------------------------------------------------------- What happened to the user prompt? Has that been dropped or is it in another patch? ::: dom/apps/src/InterAppCommService.js @@ +76,5 @@ > + > + debug("registerConnection: aKeyword: " + aKeyword + > + " manifestURL: " + manifestURL + " pageURL: " + pageURL + > + " aDescription: " + aDescription + " aAppStatus: " + aAppStatus + > + " aRules: " + JSON.stringify(aRules)); JSON.stringify() can be expensive to compute, best to remove it. @@ +86,5 @@ > + > + if (subApps[manifestURL]) { > + debug("Error! Duplicated registration.") > + return; > + } Nit: Whitespace @@ +97,5 @@ > + manifestURL: manifestURL > + }; > + > + debug("this._registeredConnections: " + > + JSON.stringify(this._registeredConnections)); Remove JSON.stringify() as above. ::: dom/apps/src/Webapps.jsm @@ +570,5 @@ > }, > > + // |aEntryPoint| is either the entry_point name or the null in which case we > + // use the root of the manifest. > + _registerConnectionsForEntryPoint: function(aManifest, aApp, aEntryPoint) { _registerInterAppCommConnectionsForEntryPoint() @@ +577,5 @@ > + root = aManifest.entry_points[aEntryPoint]; > + } > + > + let connections = root.connections; > + if (!connections || typeof(connections) !== "object") { Separate the two checks. In the second case, is there a way of warning app developers of an invalid manifest file in this case? At the least, add a debug. @@ +593,5 @@ > + // Resolve the handler path from origin. If |handler_path| is absent, > + // use |launch_path| as default. > + let handlerPathURI = launchPathURI; > + let handlerPath = connection.handler_path; > + if (handlerPath) { I have a feeling that resolveFromOrigin will throw at a lower level if the handlerPath is not a valid string due to the XPIDL implementation of nsIURI, so that this check is superfluous. The try block should be enough to catch this. @@ +598,5 @@ > + let fullPath; > + try { > + fullPath = manifest.resolveFromOrigin(handlerPath); > + } catch(e) { > + debug("Connection's handler path is invalid. Skipping."); Please print keyword and handler path. @@ +625,5 @@ > this._registerSystemMessagesForEntryPoint(aManifest, aApp, entryPoint); > } > }, > > + _registerConnections: function(aManifest, aApp) { _registerInterAppCommConnections() @@ +626,5 @@ > } > }, > > + _registerConnections: function(aManifest, aApp) { > + this._registerConnectionsForEntryPoint(aManifest, aApp, null); Please file a follow up bug to fix this nit. IMHO, the root is also a 'entry point'. Can we always pass a valid 'entry point URI' (which may be root) here rather than add a null check to the entry point function. Then add a non-null 'assertion' to the entry point function. registerSystemMessages() also makes the same assumption and should be fixed. ::: dom/interfaces/apps/nsIInterAppCommService.idl @@ +6,5 @@ > + > +interface nsIURI; > + > +// Implemented by the contract id @mozilla.org/inter-app-communication-service;1 > + Please add link to 'spec' and short info about what this should be used for. @@ +15,5 @@ > + in nsIURI aPageURI, > + in nsIURI aManifestURI, > + in DOMString aDescription, > + in DOMString aAppStatus, > + in jsval aRules); Please add a comment for the function.

Comment on attachment 792771 [details] [diff] [review] Part 1, IDL, V3 Review of attachment 792771 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/InterAppMessagePort.webidl @@ +8,5 @@ > +// MozInterAppMessagePort to meet the timeline, which still follows exactly > +// the same interface and semantic as the MessagePort is. In the future, > +// we can then align it back to MessagePort with backward compatibility. > + > +[Constructor, JSImplementation="@mozilla.org/dom/inter-app-message-port;1"] Forgot to point out, do you want this to be NoInterfaceObject so content JS can't create it?

Comment on attachment 792773 [details] [diff] [review] Part 3, connect, V3 Review of attachment 792773 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/shell.js @@ +619,1 @@ > Services.obs.addObserver(function(aSubject, aTopic, aData) { While you are here, can you give this function a name for consistency with the other observers and change its parameters to remove the a prefixes. ::: dom/apps/src/AppsUtils.jsm @@ +396,5 @@ > + return "certified"; > + default: > + return ""; > + } > + }, Nit: As in patch 2, can you just stick to the constants or is their something I'm missing? ::: dom/apps/src/InterAppCommService.js @@ +136,5 @@ > + // An example of the object literal is shown as below: > + // > + // { > + // "callerID1": { > + // oid: 12, What is an oid? In fact, please add a comment explaining all the fields. @@ +146,5 @@ > + // requestID: 78, > + // target: pubAppTarget2 > + // }, > + // } > + this._promptCallers = {}; Nit: callerPrompts seems to read better. @@ +186,5 @@ > debug("this._registeredConnections: " + > JSON.stringify(this._registeredConnections)); > }, > > + _matchMinAccessLvl: function _matchMinAccessLvl(aMinAccessLvl, aAppStatus) { Nit: s/Lvl/Level/g from here till the end of the function. Also change aAppStatus to int. @@ +221,5 @@ > + _matchRules: function _matchRules(aPubApp, aPubAppStatus, aPubRules, > + aSubApp, aSubAppStatus, aSubRules) { > + debug("_matchRules: aPubApp: " + aPubApp + " aSubApp: " + aSubApp + > + " aPubAppStatus: " + aPubAppStatus + " aSubAppStatus: " + aSubAppStatus + > + " aPubRules: " + JSON.stringify(aPubRules) + Nit: whitespace and no JSON.stringify() in debug. @@ +229,5 @@ > + // certified apps to meet the time line. Eventually, we need to make > + // it available for the non-certified apps as well. For now, only the > + // certified apps can match the rules. > + if (aPubAppStatus != "certified" || aSubAppStatus != "certified") { > + debug("Only certified apps are allowed to use IAC API for now."); Nit: s/IAC/Inter-App Communication @@ +252,5 @@ > + // Check manifestURLs. > + if ((aPubRules && Array.isArray(aPubRules.manifestURLs) && > + aPubRules.manifestURLs.indexOf(aSubApp) == -1) || > + (aSubRules && Array.isArray(aSubRules.manifestURLs) && > + aSubRules.manifestURLs.indexOf(aPubApp) == -1)) { Refactor: Can you pull out the 'X && Array.isArray(X.y) && X.y.indexOf(z) == -1' pattern into function doesNotMatchRule(RuleSet [X], Rule ['y'], Value [z]) {} @@ +253,5 @@ > + if ((aPubRules && Array.isArray(aPubRules.manifestURLs) && > + aPubRules.manifestURLs.indexOf(aSubApp) == -1) || > + (aSubRules && Array.isArray(aSubRules.manifestURLs) && > + aSubRules.manifestURLs.indexOf(aPubApp) == -1)) { > + debug("rules.manifestURLs is not matched."); Nit: It would be very useful to know in the debug line, which of the two checks failed. If that can be done simply, it would be great! @@ +268,5 @@ > + if ((aPubRules && Array.isArray(aPubRules.installOrigins) && > + aPubRules.installOrigins.indexOf(subInstallOrigin) == -1) || > + (aSubRules && Array.isArray(aSubRules.installOrigins) && > + aSubRules.installOrigins.indexOf(pubInstallOrigin) == -1)) { > + debug("rules.installOrigins is not matched."); Same thing about the debug statement as above. @@ +272,5 @@ > + debug("rules.installOrigins is not matched."); > + return false; > + } > + > + // Check developers. A developer check sounds very naive. What happens when an app is handed of to another maintainer/company? @@ +294,5 @@ > + } > + } > + > + switch (aMessage.name) { > + case "Webapps:Connect": { Best to pull the block logic out into a function. @@ +300,5 @@ > + let rules = msg.rules; > + let manifestURL = msg.manifestURL; > + let oid = msg.oid; > + let requestID = msg.requestID; > + I'm sorry, I don't understand this code block (The whole Webapps:Connect case). It pulls subApps from _registeredConnections and allowedPubApps from _allowedConnections. I'd appreciate it if you could explain this, and ask me to review again (with other issues fixed). @@ +304,5 @@ > + > + // If the app status is not specified, consider it as "web". > + let appStatus = AppsUtils.convertAppStatusToStr(msg.appStatus) || "web"; > + > + let subApps = this._registeredConnections[keyword]; subAppsManifestURLs? @@ +315,5 @@ > + return; > + } > + > + // To collect the apps that are used to be allowed to connect. > + let allowedPubApps = this._allowedConnections[keyword]; allowedPubAppsManifestURLs @@ +316,5 @@ > + } > + > + // To collect the apps that are used to be allowed to connect. > + let allowedPubApps = this._allowedConnections[keyword]; > + let allowedSubApps; you get the naming pattern now ;) @@ +325,5 @@ > + // To collect the peers that needs to be selected by the user. > + let appsToSelect = []; > + for (let subApp in subApps) { > + // If the registered app is exactly the publisher, skip it. > + if (subApp == manifestURL) { I don't understand why this check exists. @@ +338,5 @@ > + } > + > + // Only rule-matched publishers/subscribers are allowed to connect. > + let subInfo = subApps[subApp]; > + let matched = this._matchRules( I thought connections in _allowedConnections already had their rules checked. @@ +354,5 @@ > + } > + > + debug("appsToSelect: " + appsToSelect); > + > + // If no additional apps are going to be selcted, just return apps that Nit: typo, selected @@ +364,5 @@ > + target, oid, requestID); > + return; > + } > + > + // Fall through: launch a prompt to ask the user to select apps to In the context of a switch, fall through has a very specific meaning, could you reword this? @@ +375,5 @@ > + } > + > + debug("_promptCallers: " + JSON.stringify(this._promptCallers)); > + > + // TODO Bug 897169 Temproally disable the notification for poping up Nit: typo, popping. @@ +402,3 @@ > observe: function observe(aSubject, aTopic, aData) { > switch (aTopic) { > case "xpcom-shutdown": { No need for braces on the case @@ +410,1 @@ > Services.obs.removeObserver(this, "xpcom-shutdown"); Do you want to move this line up to just after the case. @@ +410,4 @@ > Services.obs.removeObserver(this, "xpcom-shutdown"); > break; > } > + case "inter-app-comm-select-app-result": { Move this block into a function. @@ +412,5 @@ > } > + case "inter-app-comm-select-app-result": { > + let data = JSON.parse(aData); > + > + debug("inter-app-comm-select-app-result: " + JSON.stringify(data)); Why don't you just print out aData :) @@ +470,5 @@ > + " aAllowedSubApps: " + aAllowedSubApps); > + > + let messagePortIDs = []; > + > + if (aAllowedSubApps && aAllowedSubApps.length != 0) { Please convert this into a negation and a return and unindent the block. In fact I probably missed more of these, but null checks and so on should be converted to quick returns so that the rest of the code can flow straight. @@ +471,5 @@ > + > + let messagePortIDs = []; > + > + if (aAllowedSubApps && aAllowedSubApps.length != 0) { > + let subApps = this._registeredConnections[aKeyword]; subAppsManifestURLs @@ +491,5 @@ > + } > + > + debug("messagePortIDs: " + messagePortIDs); > + > + // No subsribed apps are going to connect. Just return. Nit: typo, subscribed. ::: dom/apps/src/Webapps.js @@ +500,5 @@ > receiveMessage: function(aMessage) { > let msg = aMessage.json; > + let req; > + if (aMessage.name == "Webapps:Connect:Return:OK" || > + aMessage.name == "Webapps:Connect:Return:KO") { Ouch! DOMRequest vs Promises are a mess. (This is not part of the review.)

Comment on attachment 792774 [details] [diff] [review] Part 4, getConnections, V3 Review of attachment 792774 [details] [diff] [review]: ----------------------------------------------------------------- r=me with fixes and connection length issue clarified. ::: dom/apps/src/InterAppCommService.js @@ +422,5 @@ > + }); > + } > + } > + > + if (connections.length == 0) { I think connections being empty should not indicate failure. @@ +463,5 @@ > + > + // Clean up the parent entries if needed. > + if (allowedSubApps.length == 0) { > + delete allowedPubApps[pubApp]; > + if (this._isEmptyObject(allowedPubApps)) { Nit: Would Object.keys(allowedPubApps).length == 0 not work? ::: dom/apps/src/InterAppConnection.js @@ +41,5 @@ > classID: Components.ID("{9dbfa904-0718-11e3-8e77-0721a45514b8}"), > > contractID: "@mozilla.org/dom/inter-app-connection;1", > > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, From other code, this doesn't seem necessary if you have WeakRef below. Please ask someone more experienced on #content. @@ +57,5 @@ > + // Ci.nsIDOMGlobalPropertyInitializer implementation. > + init: function(aWindow) { > + debug("init"); > + > + this._window = aWindow; Remove since it is unused. @@ +63,5 @@ > + > + let principal = aWindow.document.nodePrincipal; > + > + this._manifestURL = appsService.getManifestURLByLocalId(principal.appId); > + this._pageURL = principal.URI.spec; Remove since it is unused. @@ +69,5 @@ > + > + // DOMRequestIpcHelper implementation. > + uninit: function() { > + debug("uninit"); > + }, Remove for now? ::: dom/apps/src/Webapps.js @@ +660,5 @@ > + }, this); > + req.resolve(connections); > + break; > + case "Webapps:GetConnections:Return:KO": > + req.reject("No connections allowed"); You may want to tweak/drop this based on what I said about connections length == 0 not being an error.

Comment on attachment 792775 [details] [diff] [review] Part 5, connection wrapper, V3 Review of attachment 792775 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed. ::: dom/apps/src/InterAppConnection.js @@ +111,5 @@ > }; > > +/** > + * InterAppConnectionRequestWrapper implementation. > + * Nit: whitespace @@ +113,5 @@ > +/** > + * InterAppConnectionRequestWrapper implementation. > + * > + * This implements nsISystemMessagesWrapper.wrapMessage(), which provides a > + * pluginable way to customizably wrap a "connection" type system message. Nit: plugable and drop 'to customizably'

I'll review the last patch tomorrow. Can you also ask one other person to review, it's a big chunk of code and a major API and I'd like more eyes. Thanks!

(In reply to Nikhil Marathe [:nsm] from comment #78) > Gene, please also file a bug on tests for this API. We can land the API due > to time constraints, but tests should be the next priority. Absolutely! I'm working on that at the same time. I'll either put that in this bug or a separate one. (In reply to Nikhil Marathe [:nsm] from comment #79) > Comment on attachment 792772 [details] [diff] [review] > Part 2, manifest registry, V3 > > Review of attachment 792772 [details] [diff] [review]: > ----------------------------------------------------------------- > > What happened to the user prompt? Has that been dropped or is it in another > patch? It's going to be designed in Bug 897169. The Gecko part is only in charge of firing chrome event carrying the data for showing that prompt. Since the UI/UX design for that prompt is not ready (Fernando is on vacation), I temporally disabled firing the chrome event (please see the codes at the end of Webapps:Connect in the part-3 patch). For now, all the connections are allowed to be established without the users' permission. This is safe because only certified apps are allowed to ask for connections. Btw, our government announced one day-off today due to the horrible typhoon. Will come back with new patches ASAP on Thursday. Thanks for all the detailed reviews!

I just glanced over, but I have a few comments: - in new files, there is no need to name function properties anymore. - it looks like you use the observer service as a glue between this api and the runtime. That's not great in general. Can you use an xpcom interface instead? (we did that for activities, payments and that helps a lot when adding support for other runtimes). - how is the permission checked? If this is not going through the ContentPermissionPrompt, why?

Comment on attachment 792777 [details] [diff] [review] Part 6, post message, V3 Review of attachment 792777 [details] [diff] [review]: ----------------------------------------------------------------- Dropping review until message queue length issue is cleared. Gene, also check for as much compliance as possible with the semantics of the MessagePort spec [1] and clearly document divergence in the Alt Proposal. Fernando, can you please update the alt proposal to comply with the actual implementation. I particularly don't see any mention of the InterAppMessagePortEvent interface. [1]: http://www.whatwg.org/specs/web-apps/current-work/multipage/web-messaging.html ::: dom/apps/src/InterAppCommService.js @@ +544,5 @@ > + let sender = isPublisher ? pair.publisher : pair.subscriber; > + let receiver = isPublisher ? pair.subscriber : pair.publisher; > + if (!receiver) { > + debug("The receiver's port is not ready yet. Queuing the message."); > + sender.messageQueue.push(msg.data); Seems like the messageQueue in unbounded. An application could quickly dispatch hundreds of messages and eat up memory. Can we put an upper bound without sacrificing functionality? @@ +580,5 @@ > + let receiver = isPublisher ? pair.subscriber : pair.publisher; > + if (receiver) { > + while (receiver.messageQueue.length) { > + let message = receiver.messageQueue.shift(); > + debug("Deliverying message: " + JSON.stringify(message)); delivering @@ +597,5 @@ > + let messagePortID = msg.messagePortID; > + delete this._messagePortPairs[messagePortID]; > + > + debug("_messagePortPairs: " + JSON.stringify(this._messagePortPairs)); > + break; Are unsent messages discarded? Please specify this in the documentation. ::: dom/apps/src/InterAppMessagePort.js @@ +35,5 @@ > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, > + Ci.nsIDOMGlobalPropertyInitializer]), > + > + // Ci.nsIDOMGlobalPropertyInitializer implementation. > + init: function(aWindow) { this._window = aWindow; }, You don't need a reference to window just yet. @@ +44,5 @@ > + aDict.cancelable || false); > + this._data = aDict.data; > + }, > + > + get data() { return this._data; } If the webidl attribute is readonly, you can drop this getter and rename _data to data and it will work the way you expect. @@ +64,5 @@ > classID: Components.ID("{c66e0f8c-e3cb-11e2-9e85-43ef6244b884}"), > > contractID: "@mozilla.org/dom/inter-app-message-port;1", > > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, Seems like you may not need nsISupports @@ +72,5 @@ > + initialize: function(aKeyword, aMessagePortID, aIsPublisher) { > + debug("initiailize: aKeyword: " + aKeyword + > + "aMessagePortID: " + aMessagePortID + > + " aIsPublisher: " + aIsPublisher); > + Nit: whitespace @@ +89,5 @@ > + // Ci.nsIDOMGlobalPropertyInitializer implementation. > + init: function(aWindow) { > + debug("init"); > + > + this._window = aWindow; DOMRequestIPCHelper already has a _window after calling initDOMRequestHelper, and it will overwrite this field anyway. @@ +155,4 @@ > return this.__DOM_IMPL__.getEventHandler("onmessage"); > }, > > set onmessage(aHandler) { From spec, when onmessage is set for the first time, the port should behave as if start() was called. ::: dom/webidl/InterAppMessagePort.webidl @@ +25,5 @@ > + > + [ChromeOnly] > + void initialize(DOMString keyword, > + DOMString messagePortID, > + boolean isPublisher); Can this be moved to the constructor or are the parameters unknown in certain cases?

(In reply to Fabrice Desré [:fabrice] from comment #86) > I just glanced over, but I have a few comments: > - in new files, there is no need to name function properties anymore. Sorry. I don't understand "function properties". I guess you're saying: Change: XXX: function XXX() { } to XXX: function() { } Right? > - it looks like you use the observer service as a glue between this api and > the runtime. That's not great in general. Can you use an xpcom interface > instead? (we did that for activities, payments and that helps a lot when > adding support for other runtimes). Ok. You mean ActivitiesGlue.js, for example. Right? Sounds good. I'll switch to that. Thanks! > - how is the permission checked? If this is not going through the > ContentPermissionPrompt, why? These APIs (connect()/getConnections()) are put under mozIDOMApplication, so if an app can get mozIDOMApplication object by calling getSelf(), getAll() or others with a proper permission, then it can use inter-app communication in nature.

(In reply to Gene Lian [:gene] from comment #88) > (In reply to Fabrice Desré [:fabrice] from comment #86) > > I just glanced over, but I have a few comments: > > - in new files, there is no need to name function properties anymore. > > Sorry. I don't understand "function properties". I guess you're saying: > > Change: > > XXX: function XXX() { } > > to > > XXX: function() { } > > Right? Yep, exactly. We have better logging now and don't need them for debugging anymore. > > > - how is the permission checked? If this is not going through the > > ContentPermissionPrompt, why? > > These APIs (connect()/getConnections()) are put under mozIDOMApplication, so > if an app can get mozIDOMApplication object by calling getSelf(), getAll() > or others with a proper permission, then it can use inter-app communication > in nature. Ha ok, I missed that part. Cool.

(In reply to Nikhil Marathe [:nsm] from comment #87) > Comment on attachment 792777 [details] [diff] [review] > Part 6, post message, V3 > > Review of attachment 792777 [details] [diff] [review]: > ----------------------------------------------------------------- > > Dropping review until message queue length issue is cleared. > > Fernando, can you please update the alt proposal to comply with the actual > implementation. I particularly don't see any mention of the > InterAppMessagePortEvent interface. I followed the W3C MessagePort spec [1] to implement the InterAppMessagePortEvent, which says: channel.port1.onmessage = handleMessage; function handleMessage(event) { // message is in event.data // ... } Indeed, we should also put that on the alt proposal to clarify. > Seems like the messageQueue in unbounded. An application could quickly > dispatch hundreds of messages and eat up memory. Can we put an upper bound > without sacrificing functionality? This is a very good point. Actually, we used to have a debate for that and we eventually decided to use onstart/onclose event handlers [2] to notify the publisher/subscriber when the other end is ready (or not), so that the content apps can know a better time points to post messages. However, since we need to align this to W3C MessagePort, we have to avoid using them for now. Therefore, we need a queue to handle that properly. To solve the memory issue, I think adding a upper bound is fine because this queue only works for the initialization when the message port is not yet delivered to the either end (System Message could take 100 years to deliver the message port). The content app should do hand-shaking first before actually posting the real messages. Another alternative might be adding a timeout mechanism to cancel the queue. Otherwise, we have to add onstart/onclose back but that won't follow the MessagePort spec. > From spec, when onmessage is set for the first time, the port should behave > as if start() was called. Thanks for pointing this out. :) > > ::: dom/webidl/InterAppMessagePort.webidl > @@ +25,5 @@ > > + > > + [ChromeOnly] > > + void initialize(DOMString keyword, > > + DOMString messagePortID, > > + boolean isPublisher); > > Can this be moved to the constructor or are the parameters unknown in > certain cases? Please refer to [3]. I think this is a good way to avoid polluting the content JS which shouldn't be able to initialize keyword/messagePortID/isPublisher by constructor. These metadata can only be assigned from internal calls. Btw, using [NoInterfaceObject] doesn't work either because [Constructor] and [NoInterfaceObject] are incompatible and we need [Constructor] to help us instantiate object by window. [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/web-messaging.html#introduction-11 [2] https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal#InterAppMessagePort [3] https://bugzilla.mozilla.org/attachment.cgi?id=791533&action=diff

Addressing comment #76 and comment #79.

Hi Nikhil! Addressing comment #81 except for the name of |_promptUICallers|. Actually, I think the original name is still better. I guess you might misunderstand its purpose so I added/corrected more comments to describe that. Regarding the logic of whole Webapps:Connect case, I've already added more comments in the codes to clarify the flow. Try to summarize as below as well: The main reason why we need to check |_allowedConnections| when doing connection is: we don't want to ask users to permit a connection again that used to be allowed before, which is redundant. Therefore, we need to pre-compare the existing *old* connections and simply put the *newly-requested* connections in the prompt. When the prompt UI result returns, we need to add the *newly-added* connections into the |_allowedConnections| and then eventually deliver the message ports for all the allowed connections, including the old ones and the newly-added ones. Hope it's clear to you. Please let me know if you have any other questions. Thanks for the review! :)

Comment on attachment 794013 [details] [diff] [review] Part 3, connect, V4 Review of attachment 794013 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/InterAppCommService.js @@ +227,5 @@ > + > + let manifestURLs = aRules.manifestURLs; > + if (manifestURLs.indexOf(aManifestURL) != -1) { > + return true; > + } nit: TS. Fixed in local build. @@ +246,5 @@ > + > + let installOrigins = aRules.installOrigins; > + if (installOrigins.indexOf(installOrigin) != -1) { > + return true; > + } nit: TS. Fixed in local build. @@ +371,5 @@ > + // users don't need to select/allow them again. That is, we only pop up > + // the prompt UI for the *new* connections. > + let allowedSubAppManifestURLs = []; > + let allowedPubAppManifestURLs = this._allowedConnections[keyword]; > + if (allowedPubAppManifestURLs) { This condition should be: if (allowedPubAppManifestURLs && allowedPubAppManifestURLs[pubAppManifestURL]) Fixed in my local build. The rest still works. Please go ahead to review. Thanks!

Comment on attachment 793993 [details] [diff] [review] Part 2, manifest registry, V4 Review of attachment 793993 [details] [diff] [review]: ----------------------------------------------------------------- r=me if the update isn't going to be an issue and with my matrix question clarified, otherwise it'll need a fix. ::: dom/apps/src/InterAppCommService.js @@ +16,5 @@ > +function InterAppCommService() { > + Services.obs.addObserver(this, "xpcom-shutdown", false); > + > + // This matrix is used for saving the inter-app connection info registered in > + // the app manifest. The object literal is defined as below: Seems like this matrix only saves subscribers. Or are publishers also 'subscribers' in this context of keeping a list of apps interested in the keyword? @@ +25,5 @@ > + // /* subscribed info */ > + // }, > + // "subAppManifestURL2": { > + // /* subscribed info */ > + // } Nit: Just add a line, "..." so that its obvious that there can be more than 2 apps. @@ +82,5 @@ > + subAppManifestURLs = this._registeredConnections[aKeyword] = {}; > + } > + > + if (subAppManifestURLs[manifestURL]) { > + debug("Error! Duplicated registration. Skipping."); Would it be better to let last registration win? In the current case, if an app was updated and updated its connections, it seems like this call will fail. Is that the case? In that case it should be fixed. ::: dom/apps/src/Webapps.jsm @@ +586,5 @@ > + if (!connections) { > + return; > + } > + > + if (typeof(connections) !== "object") { Nit: typeof is not a function so 'typeof connections' would be better. @@ +594,5 @@ > + > + let manifest = new ManifestHelper(aManifest, aApp.origin); > + let launchPathURI = Services.io.newURI(manifest.fullLaunchPath(aEntryPoint), > + null, null); > + let manifestURI = Services.io.newURI(aApp.manifestURL, null, null); Just out of curiosity, will the function reach this point with aEntryPoint being somehow invalid leading to the newURI() calls throwing? @@ +601,5 @@ > + let connection = connections[keyword]; > + > + // Resolve the handler path from origin. If |handler_path| is absent, > + // use |launch_path| as default. > + let handlerPath = connection.handler_path; Nit: replace this with connection.handler_path directly at all use sites. ::: dom/interfaces/apps/nsIInterAppCommService.idl @@ +5,5 @@ > +#include "domstubs.idl" > + > +interface nsIURI; > + > +/* Nit: s/*/**/ since this is a doc comment. @@ +8,5 @@ > + > +/* > + * Implemented by the contract id @mozilla.org/inter-app-communication-service;1 > + * > + * This interface contains helpers for the Inter-App Communication API [1] Nit: s/ the//

Comment on attachment 794013 [details] [diff] [review] Part 3, connect, V4 Review of attachment 794013 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the explanation! r=me with nits fixed and a follow up patch (if required) for refreshing the allowed connections. ::: dom/apps/src/InterAppCommService.js @@ +127,5 @@ > + // "app://subApp5.gaiamobile.org/manifest.webapp", > + // ] > + // } > + // } > + this._allowedConnections = {}; An app update and a call to registerConnections will also have to refresh this array. @@ +147,5 @@ > + // target: pubAppTarget2 > + // }, > + // } > + // > + // where |oid| is the outerWindowID of the window requesting the connection, Nit: Just call it outerWindowID :) @@ +403,5 @@ > + }); > + } > + > + // If no new apps are rules-matched to select, just return the *old* apps > + // that used to be allowed to connect before. Nit: You can drop this comment. @@ +421,5 @@ > + requestID: requestID, > + target: aTarget > + }; > + > + // TODO Bug 897169 Temproally disable the notification for popping up typo, temporarily @@ +447,5 @@ > + keyword: keyword, > + selectedApps: appsToSelect })); > + }, > + > + _handleSeletcedApps: function(aData) { Nit: Typo, selected @@ +528,5 @@ > + ppmm = null; > + break; > + case "inter-app-comm-select-app-result": > + debug("inter-app-comm-select-app-result: " + aData); > + this._handleSeletcedApps(JSON.parse(aData)); Nit: And correct that typo here ::: dom/apps/src/Webapps.js @@ +469,5 @@ > + cpmm.sendAsyncMessage("Webapps:Connect", > + { keyword: aKeyword, > + rules: aRules, > + manifestURL: this.manifestURL, > + oid: this._id, outerWindowID

There should be bugs for: 1) Allow a user to unselect selected apps later - It would be nice to merge this with the app permissions UI in settings. 2) Relatively unimportant messages like the music on lock screen thing hardly need user permission. As in, it would be annoying to me if an app asked me for permission for something trivial like this. Perhaps publishers could opt out of this in the ruleset?

(In reply to Nikhil Marathe [:nsm] (away till Aug 26) from comment #94) > Comment on attachment 793993 [details] [diff] [review] > Part 2, manifest registry, V4 > > Review of attachment 793993 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me if the update isn't going to be an issue and with my matrix question > clarified, otherwise it'll need a fix. > > ::: dom/apps/src/InterAppCommService.js > @@ +16,5 @@ > > +function InterAppCommService() { > > + Services.obs.addObserver(this, "xpcom-shutdown", false); > > + > > + // This matrix is used for saving the inter-app connection info registered in > > + // the app manifest. The object literal is defined as below: > > Seems like this matrix only saves subscribers. Or are publishers also > 'subscribers' in this context of keeping a list of apps interested in the > keyword? The definition of "Subcriber" in this context means the app that has registered connections in their manifest. We use this matrix to keep track of the manifest registration. If the publisher also wants to listen for a certain keyword, it must register connections in its manifest as well, then the publisher can also be a subscriber. (In reply to Nikhil Marathe [:nsm] (away till Aug 26) from comment #95) > Comment on attachment 794013 [details] [diff] [review] > Part 3, connect, V4 > > Review of attachment 794013 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for the explanation! > > r=me with nits fixed and a follow up patch (if required) for refreshing the > allowed connections. > > ::: dom/apps/src/InterAppCommService.js > @@ +127,5 @@ > > + // "app://subApp5.gaiamobile.org/manifest.webapp", > > + // ] > > + // } > > + // } > > + this._allowedConnections = {}; > > An app update and a call to registerConnections will also have to refresh > this array. Indeed, not only _allowedConnections but also _registeredConnections. Actually, System Message needs to have the similar fix. I'll fire separate bugs for all of them. (In reply to Nikhil Marathe [:nsm] (away till Aug 26) from comment #96) > There should be bugs for: > 1) Allow a user to unselect selected apps later - It would be nice to merge > this with the app permissions UI in settings. I think that's what the InterAppConnection.cancel() currently does. This function should have been supported on the Gecko side. I think the Gaia side (Setting UI) can also be considered at bug 897169. > 2) Relatively unimportant messages like the music on lock screen thing > hardly need user permission. As in, it would be annoying to me if an app > asked me for permission for something trivial like this. Perhaps publishers > could opt out of this in the ruleset? Good point. I think we can automatically allow all the connections among certified apps without popping out a prompt UI. We will have to revisit this when we want to expose this API to non-certified apps at bug 907068. I'll fix the rest you suggested in the comments. Thanks Nikhil for the detailed reviews!

Addressing comment #94, except for the handlerPath because I found out resolveFromOrigin() still works for undefined/null/empty string. If the handlerPath is not valid we should directly use launchPathURI. r=nsm

Addressing comment #95. r=nsm

Addressing comment #82. r=nsm

Addressing comment #83. r=nsm

Addressing comment #87. Will check the W3C spec again before asking for the second review.

Addressing comment #94, except for the handlerPath because I found out resolveFromOrigin() still works for undefined/null/empty string. If the handlerPath is not valid we should directly use launchPathURI. r=nsm

Will add more comments before asking for the second review.

Addressing comment #87 and please see my responds at comment #90. Thanks for the review!

Comment on attachment 795245 [details] [diff] [review] Part 6, post message, V4 Review of attachment 795245 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/InterAppCommService.js @@ +33,5 @@ > +// Limit the number of pending messages for posting message. > +let kMaxPendingMessages; > +try { > + kMaxPendingMessages = > + Services.prefs.getIntPref("dom.messages.maxPendingMessages"); I'd prefer not using the generic 'messages' namespace for this. Is dom.interappCommunicationAPI too verbose? @@ +35,5 @@ > +try { > + kMaxPendingMessages = > + Services.prefs.getIntPref("dom.messages.maxPendingMessages"); > +} catch(e) { > + debug("getIntPref throws because the preference is not set."); which preference :) ::: dom/apps/src/InterAppMessagePort.js @@ +156,5 @@ > + > + this._closed = true; > + this._messageQueue.length = 0; > + > + // When this method called on a port local port that is entangled with called on a local port @@ +176,5 @@ > this.__DOM_IMPL__.setEventHandler("onmessage", aHandler); > + > + // The first time a MessagePort object's onmessage IDL attribute is set, > + // the port's message queue must be enabled, as if the start() method had > + // been called. This will 'start()' on replacing the handler too. i.e. port.onmessage = handlerA; // start() called. port.onmessage = handlerB; // start() called again. I think that isn't correct. I'd like bent to review this MessagePort implementation, let's see what he thinks. ::: dom/webidl/InterAppMessageEvent.webidl @@ +8,5 @@ > + > +[JSImplementation="@mozilla.org/dom/inter-app-message-event;1", > + Constructor(DOMString type, > + optional MozInterAppMessageEventInit eventInitDict)] > +interface MozInterAppMessageEvent : Event { This will expose MozInterAppMessageEvent to content. You can have your own custom function to make sure the principal/window satisfies certain conditions and so on. Afaik there is also a way to make it so the instance is exposed to DOM but the constructor isn't. Don't remember it of the top of my head.

Sorry for replying to the needinfo? so late, I've been on PTO during August. You already figured out a lot of the missing details of the API and the implementation is quite advanced. Awesome job Gene and Nikhil! I'll try to catch up with the patches as soon as possible. It seems that Gene already replied to your questions :), so I'm clearing the needinfo flag. Feel free to set it back if needed.

Comment on attachment 792771 [details] [diff] [review] Part 1, IDL, V3 Hi Mounir, Jonas is having week-long vacation. Could you please retake the superreview? The Gaia folks are hoping to have an initiative version of APIs to work with before the end of August. Per off-line discussion, we decided to narrow down its scope to certified apps only and fire a couple of follow-ups to extend that.

Hi Nikhil, I add a customized function to let the constructors only be exposed to chrome codes. In this way, the content pages won't be aware of the existence of constructors and cannot create instances by themselves. This is inspired by your comment #106 and Bug 897913. Thanks for the review again!

Comment on attachment 797959 [details] [diff] [review] Part 7, turn on constructors for chrome only, V1 Review of attachment 797959 [details] [diff] [review]: ----------------------------------------------------------------- bz, is this the only way? What gene wants to do is to let MozInterAppConnection/MozInterAppMessageEvent have constructors which are only exposed to chrome, but the constructed objects should be accessible from content.

Is there a reason marking the interface [ChromeOnly] doesn't do what you want?

(In reply to Boris Zbarsky [:bz] from comment #111) > Is there a reason marking the interface [ChromeOnly] doesn't do what you > want? That was exactly the first thing I tried. It didn't work when I was implementing this. Magically, I just tried it again and it's working now... Maybe it's been fixed or I wrongly used that. Thank you guys! :)

Comment on attachment 797959 [details] [diff] [review] Part 7, turn on constructors for chrome only, V1 Then, we don't need this.

Addressing comment #77 and use [ChromeOnly] to avoid constructors being exposed to the content pages. Hi Mounir, please see comment #108 for why I need your super-review. Thanks!

For what it's worth, the behavior of [ChromeOnly] here should not have changed since bug 897913 was fixed....

Comment on attachment 798179 [details] [diff] [review] Part 1, Web IDLs, V4 Olli, do you think you could do the sr of this API?

Comment on attachment 798179 [details] [diff] [review] Part 1, Web IDLs, V4 This add MozInterAppMessagePort to global scope even on desktop. We should not do that. and wouldn't pass the current tests which make sure we don't add stuff to global scope randomly.

Why is MozInterAppMessagePort not [NoInterfaceObject]?

Yeah, that should be ok, but we still don't want to expose this stuff on desktop builds, right?

Whatever the web-visible API entry point is should not be exposed on builds that don't support this API, sure.

Is there a rough estimate for when we think this will land? The timeframe for that will partly determine whether I land bug 902974 now with the Settings API-based workaround, or wait until this is ready to land a less-hacky version.

Addressing comment #117. Note that the compiling process would complain interface has more than one of 'Pref', 'Func', and 'ChomeOnly' specified. Therefore, I think 'Func' is the best way to differ b2g-gonk and b2g-desktop builds and expose interface/constructor to chrome codes only at the same time. Thanks Olli for the review!

Comment on attachment 799570 [details] [diff] [review] Part 1, Web IDLs, V5 Should be marked as super-review.

(In reply to Jim Porter (:squib) from comment #121) > Is there a rough estimate for when we think this will land? The timeframe > for that will partly determine whether I land bug 902974 now with the > Settings API-based workaround, or wait until this is ready to land a > less-hacky version. The current patches are working. If the reviewers won't have strong opinions on the architecture design then it could probably be finished by this weekend. Personally, I'm also hoping we can land this ASAP so that the Gaia side can have something to work with at the same time and work together to fix issues during the coming Oslo work week. We can keep refining the architecture in the future, like sharing MessagePort with shared worker.

(In reply to Gene Lian [:gene] (please use needinfo?) from comment #124) > (In reply to Jim Porter (:squib) from comment #121) > > Is there a rough estimate for when we think this will land? > The current patches are working. If the reviewers won't have strong opinions > on the architecture design then it could probably be finished by this > weekend. I mean it depends on the results of (super)reviews for the remaining part-1 and part-6 patches. Ping a bit by the way. :) > Personally, I'm also hoping we can land this ASAP so that the Gaia side can > have something to work with at the same time and work together to fix issues > during the coming Oslo work week. We can keep refining the architecture in > the future, like sharing MessagePort with shared worker.

I've tried these patches, and after a little bit of fiddling, I got them to work (initially, MozInterAppCommsPort didn't have a constructor, and things failed). However, I can't seem to make things work with the system app as a subscriber. The "connection" system message never gets fired, and so the port is never readied. Is there anything different I'd need to do to make this work on the system app? Also, I'm curious about what should happen with communications when the subscribing app is opened after the publishing app. If, say, the music app is publishing "now playing" info, and another app which is opened after the music app wants to know what's now playing, how would it get that info?

Comment on attachment 799570 [details] [diff] [review] Part 1, Web IDLs, V5 >+/* static */ bool >+InterAppComm::EnabledForScope(JSContext* /* unused */, JSObject* /* unused */) >+{ >+ // Diable the constructors if they're disabled by the preference for sure. Disable >+ if (!Preferences::GetBool("dom.inter-app-communication-api.enabled", false)) { >+ return false; >+ } >+ >+ // Only expose the constructors to the chrome codes for Gecko internal uses. >+ // The content pages shouldn't be aware of the constructors. >+ nsIPrincipal* principal = nsContentUtils::GetSubjectPrincipal(); >+ return nsContentUtils::IsSystemPrincipal(principal); perhaps xpc::AccessCheck::isChrome(jsObject) >+const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components; Interesting. Never seen this syntax before, but I don't write too much JS :) >+ cancel: function() { >+ // TODO >+ } This is implemented in some other patch, I assume >+ postMessage: function(aMessage) { >+ // TODO >+ }, >+ >+ start: function() { >+ // TODO >+ }, >+ >+ close: function() { >+ // TODO >+ }, And there >+++ b/dom/apps/src/Makefile.in Technically adding a new Makefile.in needs a build peer review >+ /** >+ * Inter-App Communication APIs. >+ * >+ * https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal >+ */ >+ nsISupports connect(in DOMString keyword, >+ [optional] in jsval rules); // nsISupports is a Promise. Align the latter parameter >+[HeaderFile="mozilla/dom/InterAppComm.h", >+ Func="mozilla::dom::InterAppComm::EnabledForScope", >+ Constructor(DOMString keyword, DOMString publisher, DOMString subsriber), >+ JSImplementation="@mozilla.org/dom/inter-app-connection;1"] >+interface MozInterAppConnection { >+ readonly attribute DOMString keyword; >+ readonly attribute DOMString publisher; >+ readonly attribute DOMString subscriber; >+ >+ void cancel(); >+}; >+[HeaderFile="mozilla/dom/InterAppComm.h", >+ Func="mozilla::dom::InterAppComm::EnabledForScope", >+ Constructor(DOMString keyword, MozInterAppMessagePort port), >+ JSImplementation="@mozilla.org/dom/inter-app-connection-request;1"] >+interface MozInterAppConnectionRequest { >+ readonly attribute DOMString keyword; >+ >+ readonly attribute MozInterAppMessagePort port; >+}; >+[HeaderFile="mozilla/dom/InterAppComm.h", >+ Func="mozilla::dom::InterAppComm::EnabledForScope", >+ JSImplementation="@mozilla.org/dom/inter-app-message-port;1"] >+interface MozInterAppMessagePort : EventTarget { >+ void postMessage(any message); >+ >+ void start(); >+ >+ void close(); >+ >+ attribute EventHandler onmessage; Since you try to make this all behave like MessagePort, make sure to copy the odd special case there is for onmessage. When that EventHandler is set, start() is called implicitly if not called before (using addEventListener("message") doesn't have the same behavior) >+++ b/modules/libpref/src/init/all.js >@@ -4358,8 +4358,11 @@ pref("captivedetect.pollingTime", 3000); > pref("captivedetect.maxRetryCount", 5); > #endif > > #ifdef RELEASE_BUILD > pref("dom.forms.inputmode", false); > #else > pref("dom.forms.inputmode", true); > #endif >+ >+// DOM Inter-App Communication API. >+pref("dom.inter-app-communication-api.enabled", false); This shouldn't be needed since false is the default value for bool prefs.

(In reply to Jim Porter (:squib) from comment #126) > I've tried these patches, and after a little bit of fiddling, I got them to > work (initially, MozInterAppCommsPort didn't have a constructor, and things > failed). Oh! Actually, the uploaded patches are not synchronized because some of them are just fixed in my local builds. I intentionally did this because I'd like to let reviewers follow up other reviewers' comments. > However, I can't seem to make things work with the system app as a > subscriber. The "connection" system message never gets fired, and so the > port is never readied. Is there anything different I'd need to do to make > this work on the system app? Let me check this and come back to you later. It should be working for the system app as well. > Also, I'm curious about what should happen with communications when the > subscribing app is opened after the publishing app. If, say, the music app > is publishing "now playing" info, and another app which is opened after the > music app wants to know what's now playing, how would it get that info? The IAC API is currently working based on the System Message, which means when the Music App is publishing a message, all the apps registered for this connection must be forced to be opened to handle that. This might be a deficiency (or not) before the Event Page is available. I guess you're talking about the apps that are installed at run-time. Indeed, that might be a potential issue in the current mechanism. However, I think we can still think about this scenario in an opposite way. That is, the apps can call connect() to retrieve the now-playing music info of the Music App. The Music app can still get a port as a subscriber and use it to publish the updated now-playing music later. Note that the concept of message port is a two-way messaging. A publisher can be a subscriber and vice versa.

(In reply to Gene Lian [:gene] (please use needinfo?) from comment #128) > (In reply to Jim Porter (:squib) from comment #126) > > I've tried these patches, and after a little bit of fiddling, I got them to > > work (initially, MozInterAppCommsPort didn't have a constructor, and things > > failed). > > Oh! Actually, the uploaded patches are not synchronized because some of them > are just fixed in my local builds. I intentionally did this because I'd like > to let reviewers follow up other reviewers' comments. For you convenience, I uploaded the latest patches including the fix according to reviewers' comments. Please see 'Latest Patches' in the attachments. > > However, I can't seem to make things work with the system app as a > > subscriber. The "connection" system message never gets fired, and so the > > port is never readied. Is there anything different I'd need to do to make > > this work on the system app? > > Let me check this and come back to you later. It should be working for the > system app as well. I just checked the system app again. The patches are working well for the system app as a subscriber. Please see 'Gaia Sample Codes' in the attachments for your reference. :)

Ok, things seems to basically work now, although I think the touch event shim for running Gaia in desktop Firefox broke somehow, since it gets into an infinite loop. I'll try on a device later and see if it's better there. Anyway, one thing I'm not sure about is how do I manipulate the DOM from an IAC subscriber (specifically, changing the Now Playing elements in the system app when the music app starts a new song)? It seems like the IAC connection happens in a different document from the actual system app. I have the handler_path set to "/index.html", and all the communication is succeeding, but nothing happens to the DOM.

Ok, it seems like I can manipulate the DOM in regular apps, but sending messages to the system app is opening a new page.

I managed to get the build working on the device, and the above issues don't happen there. Hooray! One more question though: is there a way to tell when the message port is closed? When the music app exits, I need to be able to respond by hiding the "now playing" info. I see the proposal had an onclose attribute, but it sounds like that was removed. Thoughts/ideas?

For what it's worth, here's my branch: https://github.com/jimporter/gaia/tree/music-nowplaying-notification Sometimes it works great, and sometimes it just completely stops working until I close the app and start fresh. I'm not sure why.

Comment on attachment 795245 [details] [diff] [review] Part 6, post message, V4 Review of attachment 795245 [details] [diff] [review]: ----------------------------------------------------------------- This looks generally great, I'd just like to see the next version where the parent doesn't trust the child to always give it correct information (see below): ::: dom/apps/src/InterAppCommService.js @@ +38,5 @@ > +} catch(e) { > + debug("getIntPref throws because the preference is not set."); > + kMaxPendingMessages = 5; > +} > +debug("kMaxPendingMessages: " + kMaxPendingMessages); Please see my comment in InterAppMessagePort.js about debug() and do that here too. @@ +578,5 @@ > + messagePortIDs.push(messagePortID); > + } > + } > + messagePortIDs.forEach(function(aMessagePortID) { > + delete this._messagePortPairs[aMessagePortID]; Are JS arrays not mutation-safe? If so you can just delete inside the other block. @@ +584,5 @@ > + }, > + > + _registerMessagePort: function(aMessage, aTarget) { > + let keyword = aMessage.keyword; > + let messagePortID = aMessage.messagePortID; You shouldn't let the child process specify its own message port id. It could lie and tell you the message port of another child process, or something. Any kind of unique identifying information should be generated in the parent only. @@ +591,5 @@ > + let pageURL = aMessage.pageURL; > + > + let pair = this._messagePortPairs[messagePortID]; > + if (!pair) { > + pair = this._messagePortPairs[messagePortID] = { keyword: keyword }; These had better be unique. I'd bail out if they're not. @@ +623,5 @@ > + let manifestURL = aMessage.manifestURL; > + debug("Unregistering message port for " + manifestURL); > + > + let messagePortID = aMessage.messagePortID; > + delete this._messagePortPairs[messagePortID]; Similarly, you should be able to look this up based on target. Don't trust the child. @@ +642,5 @@ > + }, this); > + }, > + > + _postMessage: function(aMessage) { > + let messagePortID = aMessage.messagePortID; Don't trust the child. Look it up. ::: dom/apps/src/InterAppMessagePort.js @@ +29,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "appsService", > + "@mozilla.org/AppsService;1", > + "nsIAppsService"); > + > +const kMessages =["InterAppMessagePort:OnMessage"]; Nit: space after = @@ +45,5 @@ > + contractID: "@mozilla.org/dom/inter-app-message-event;1", > + > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports]), > + > + __init: function(aType, aDict) { Nit: One _. Two feels... reserved almost. @@ +68,5 @@ > > contractID: "@mozilla.org/dom/inter-app-message-port;1", > > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMGlobalPropertyInitializer, > + Ci.nsISupportsWeakReference]), I don't think you need weak reference support here, do you? @@ +142,5 @@ > + } > + > + // When a port's port message queue is enabled, the event loop must use it > + // as one of its task sources. > + this._started = true; Technically you don't need to do any of this if this._started is true already. @@ +177,5 @@ > + > + // The first time a MessagePort object's onmessage IDL attribute is set, > + // the port's message queue must be enabled, as if the start() method had > + // been called. > + this.start(); Only call this once, check this._started to see if it's needed. @@ +182,5 @@ > + }, > + > + _dispatchMessage: function _dispatchMessage(aMessage) { > + let wrappedMessage = ObjectWrapper.wrap(aMessage, this._window); > + debug("_dispatchMessage: wrappedMessage: " + JSON.stringify(wrappedMessage)); This is expensive and slows down optimized builds a lot. Please follow convention elsewhere and have: const DEBUG = false; function debug(aMsg) { if (DEBUG) { dump("-- InterAppMessagePort: " + Date.now() + ": " + aMsg + "\n"); } } And then, everywhere where you call debug() now, replace with: if (DEBUG) debug(JSON.stringify(foo)); @@ +202,5 @@ > + return; > + } > + > + switch (aMessage.name) { > + case "InterAppMessagePort:OnMessage": Have a default block that does a debug() warning below. ::: dom/webidl/InterAppMessageEvent.webidl @@ +8,5 @@ > + > +[JSImplementation="@mozilla.org/dom/inter-app-message-event;1", > + Constructor(DOMString type, > + optional MozInterAppMessageEventInit eventInitDict)] > +interface MozInterAppMessageEvent : Event { All events have a constructor nowadays, though you probably want to make this one ChromeOnly? ::: dom/webidl/InterAppMessagePort.webidl @@ +20,5 @@ > attribute EventHandler onmessage; > + > + [ChromeOnly] > + void initialize(DOMString keyword, > + DOMString messagePortID, You should not allow the child process to specify its own message port ID. You should instead generate one automatically in the parent.

Hi Bent, Thank you for the review very much! Actually, the message port ID is centrally generated in the parent (please refer to the part-3 patch). The child just uses that ID (passed from parent) to initialize the message port based on its window. Note that we can only do this in the chrome codes only so the content pages cannot do that and won't have chances to deceive the parent. Does that sound reasonable to you?

(In reply to Jim Porter (:squib) from comment #134) > One more question though: is there a way to tell when the message port is > closed? When the music app exits, I need to be able to respond by hiding the > "now playing" info. I see the proposal had an onclose attribute, but it > sounds like that was removed. Thoughts/ideas? We hope to align our spec to the formal W3C MessagePort which doesn't support such an onclose/onstart event handler unfortunately. I think you can let the Music App post a message like "stop showing now-playing" through the original port before exiting. Or let the System App post a message like "are you still alive" then stop showing now-playing if the Music App doesn't have response (after a timeout). (In reply to Jim Porter (:squib) from comment #135) > Sometimes it works great, and sometimes it just completely stops working > until I close the app and start fresh. I'm not sure why. That's weird. I'll take a look at your codes. However, I'm going to fly to the Oslo work week in a couple of hours. We can co-work to fix that there.

Hi Bent, what do you think about the comment #137?

Gene, it's not always possible for an app to send a message at quit, it might be killed under memory pressure or something. Would it make sense for the system to send a message signifying close on the other end of a channel when one end is closed. I should go and dig through the archives of some mailing list, surely the MessagePort designers thought of this or an alternative. bent, do you know?

(In reply to Nikhil Marathe [:nsm] from comment #140) > Gene, it's not always possible for an app to send a message at quit, it > might be killed under memory pressure or something. Would it make sense for > the system to send a message signifying close on the other end of a channel > when one end is closed. I should go and dig through the archives of some > mailing list, surely the MessagePort designers thought of this or an > alternative. > > bent, do you know? fwiw, we do that for activities. If the provider is killed without returning anything, the platform fires an error to the caller.

What I do right now is super-gross, but it seems to work: <https://github.com/mozilla-b2g/gaia/pull/11847/files#L5R33>.

Per comment #87, use Constructor to initialize the MozInterAppMessagePort instead of an extra initialize(). Note that the Constructor can only be used in the chrome codes only as well because of the check of EnabledForScope().

Hi Bent, As mentioned at the comment #137, I worked out another patch to clarify the mechanism that the message port ID is actually generated by the parent instead of by the child. The message port ID will be dispatched to both the sender and receiver ends for creating their own message ports respectively. Later, if the parent receives any message port ID that was not generated by the parent before, then we will bail it out and do nothing for it. Fixed comment #136 and comment #106 except for: 1. I'll fix the ways of calling debug() for all the patches at one shot before landing. Just hoping to make sure do you intentionally put the |if (DEBUG)| check both inside and outside the debug() function? 2. Please see DOMRequestHelper.jsm: /** * An object which "inherits" from DOMRequestIpcHelper and declares its own * queryInterface method MUST implement Ci.nsISupportsWeakReference. */ That's why we need Ci.nsISupportsWeakReference. 3. MozInterAppMessageEvent has already used EnabledForScope() to limit the constructor to be used for chrome codes only. Please see part-1 patch for how it works. 4. __init() is used for the WebIDL JS implementation. Please see [1]. [1] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Implementing_WebIDL_using_Javascript

(In reply to Gene Lian [:gene] (please use needinfo?) from comment #144) > 1. I'll fix the ways of calling debug() for all the patches at one shot > before landing. Just hoping to make sure do you intentionally put the |if > (DEBUG)| check both inside and outside the debug() function? Hm, it's probably not important to check DEBUG inside the function as long as you always check it outside. > That's why we need Ci.nsISupportsWeakReference. Ok! > 3. MozInterAppMessageEvent has already used EnabledForScope() to limit the > constructor to be used for chrome codes only. Makes sense! > 4. __init() is used for the WebIDL JS implementation. Please see [1]. Neat, I learn something everyday.

sr=smaug and address comment #127 except for removing the preference in the all.js. I'd prefer keeping that to avoid throwing exception when called by JS codes. I saw lots of bool preferences have been done in the same way, so it should be fine to go. Please feel free to let me know if you have strong opinion on that. Thanks! Hi Ted, Could you please take a look about the moz.build/Makefile.in changes? We need Build Peer's review on this. Thanks!

Comment on attachment 802422 [details] [diff] [review] Part 1, Web IDLs, V6 Review of attachment 802422 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/WebIDL.mk @@ +185,5 @@ > ImageDocument.webidl \ > InspectorUtils.webidl \ > + InterAppConnection.webidl \ > + InterAppConnectionRequest.webidl \ > + InterAppMessagePort.webidl \ bug 912197 bitrotted this, there's a moz.build file here now.

Comment on attachment 801496 [details] [diff] [review] Part 6, post message, V5 Review of attachment 801496 [details] [diff] [review]: ----------------------------------------------------------------- Hi Gene, This looks pretty good, just needs a little more: ::: dom/apps/src/InterAppCommService.js @@ +38,5 @@ > +} catch(e) { > + debug("Error! Preference 'dom.interAppComm.maxPendingMessages' is not set."); > + kMaxPendingMessages = 5; > +} > +debug("kMaxPendingMessages: " + kMaxPendingMessages); I don't think we should drop messages if the count exceeds some maximum. We should queue them as long as we have memory for it. @@ +395,2 @@ > let messagePortID = UUIDGenerator.generateUUID().toString(); > + this._messagePortPairs[messagePortID] = { keyword: aKeyword }; I think this isn't quite enough, you need to somehow store the uuids per-app. Otherwise a hacked app could guess a UUID for another app and send it a message. @@ +595,5 @@ > + let manifestURL = aMessage.manifestURL; > + let pageURL = aMessage.pageURL; > + > + let pair = this._messagePortPairs[messagePortID]; > + if (!pair) { You should also ensure |!(whichSide in pair)| so that you don't accidentally overwrite something. @@ +630,5 @@ > + let manifestURL = aMessage.manifestURL; > + debug("Unregistering message port for " + manifestURL); > + > + let messagePortID = aMessage.messagePortID; > + delete this._messagePortPairs[messagePortID]; You need to somehow ensure that the process that sent this message didn't just guess another app's UUID. @@ +654,5 @@ > + let isPublisher = aMessage.isPublisher; > + let message = aMessage.message; > + > + let pair = this._messagePortPairs[messagePortID]; > + if (!pair) { Check that the UUID is for the correct app. @@ +666,5 @@ > + debug("The receiver's port is not ready yet. Queuing the message."); > + let messageQueue = sender.messageQueue; > + messageQueue.push(message); > + if (messageQueue.length > kMaxPendingMessages) { > + messageQueue.shift(); Let's not do this. @@ +734,5 @@ > let target = aMessage.target; > > // To prevent the hacked child process from sending commands to parent > // to do illegal connections, we need to check its manifest URL. > + if (aMessage.name !== "child-process-shutdown" && Hm, "child-process-shutdown" is already in kMessages, why is this needed? ::: dom/apps/src/InterAppMessagePort.js @@ +134,4 @@ > }, > > start: function() { > + // Beginning dispatching messages received on the port. Nit: s/Beginning/Begin/ @@ +232,5 @@ > + } > + > + this._dispatchMessage(message.message); > + break; > + default: Nit: newline after the break.

Gene: I tried your suggestion for working around the missing MessagePort.onclose method by having the music app listen for the "unload" event and then sending a message to the system app. However, the system app never receives the message, presumably because the port is closed by the time it's sent. It looks like the only way to handle this is the way I'm doing now: by checking the appterminated event. This has the unfortunate side effect of meaning we can't use the IAC API when the music app is opened as an activity, since there's no event to listen to when an activity is closed. I think we're adding an event like that, but it's not there yet. I really think we need to be able to tell when the message port is closed. The fact that the W3C MessagePort spec doesn't address this is, in my opinion, a failing of the spec.

Yeap, I agree with you! I'll fire another bug and talk to some Message Port designers to add the onstart/onclose event handlers. For now, we need to find some work-arounds to bypass that before we're allowed to add them in the Message Port.

Hi Bent, Solved the comment #148. My solution is letting the parent save the manifest URLs of sender and receiver when the ID is generated. Later, the parent can use that to check the identity of the app for all the requests. Please see _identifyMessagePort(...) for how it checks port ID, which side and manifest URL. Regarding 'child-process-shutdown', the part-3 patch uses target.assertContainApp(manifestURL) to prevent the hacked child process and 'child-process-shutdown' doesn't carry manifestURL, so I didn't do the check for that. Thank you very much for the review!

Per discussion with Bent, we hope to strip out the |isPublisher| which could be unsafe. I'll come back with a new patch later.

Comment on attachment 803526 [details] [diff] [review] Part 6, post message, V7 Review of attachment 803526 [details] [diff] [review]: ----------------------------------------------------------------- This looks really great Gene! Thanks. Please don't forget to fix the DEBUG/debug() calls, and please address the following comments. r=me once that's all done! ::: dom/apps/src/InterAppCommService.js @@ +625,5 @@ > + > + debug("Registering message port for " + manifestURL); > + let pair = identity.pair; > + let isPublisher = identity.isPublisher; > + Nit: unnecessary whitespace here. @@ +768,5 @@ > > // To prevent the hacked child process from sending commands to parent > // to do illegal connections, we need to check its manifest URL. > + if (aMessage.name !== "child-process-shutdown" && > + kMessages.indexOf(aMessage.name) != -1) { Again, since "child-process-shutdown" is in kMessages I don't know why we need to check this separately.

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #156) > Comment on attachment 803526 [details] [diff] [review] > Part 6, post message, V7 > > Review of attachment 803526 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks really great Gene! Thanks. > > Please don't forget to fix the DEBUG/debug() calls, and please address the > following comments. No problem! Thank you for all the great reviews! > Again, since "child-process-shutdown" is in kMessages I don't know why we > need to check this separately. I already left a comment at comment #151: Regarding 'child-process-shutdown', the part-3 patch uses target.assertContainApp(manifestURL) to prevent the hacked child process and 'child-process-shutdown' doesn't carry manifestURL, so I cannot do the check for that.