Last Comment Bug 777202 - RadioInterfaceLayer broadcasts request responses to all content processes
: RadioInterfaceLayer broadcasts request responses to all content processes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Hsin-Yi Tsai [:hsinyi]
:
Mentors:
Depends on:
Blocks: 777188
  Show dependency treegraph
 
Reported: 2012-07-24 18:33 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-08-17 05:26 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
WIP (4.70 KB, patch)
2012-08-01 01:57 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review
WIP part(a1): Address telephony-related event. (22.71 KB, patch)
2012-08-08 02:52 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review
WIP part(a1): Address telephony-related event. (22.09 KB, patch)
2012-08-08 03:04 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review
Part 1: handle request messages (15.84 KB, patch)
2012-08-12 20:56 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review
Part2: telephony-permission events (20.73 KB, patch)
2012-08-14 21:14 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review
Part2: telephony-permission events (21.02 KB, patch)
2012-08-14 23:28 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review-
Details | Diff | Review
Part 1: handle request messages (v2) (15.18 KB, patch)
2012-08-16 04:44 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review+
Details | Diff | Review
Patch: handle request messages (v3) (16.82 KB, patch)
2012-08-16 23:08 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-24 18:33:12 PDT
It should only send it to clients with telephony privileges.
Comment 1 Philipp von Weitershausen [:philikon] 2012-07-27 10:45:00 PDT
Hsinyi, can you tackle this? 

Quoting bug 777188:

> I can't really think of a case in which we should ever broadcast a message to
> all content processes at this level.  Instead, when specific subprocesses make
> requests to JS services, the services need to save the .target (bug 776825
> comment 4) and only reply to that.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-27 11:32:30 PDT
Yes, and note that this work is similar to bug 777200: we want to keep a list of the privileged listeners for this info, and scope broadcasts to them.

We should ensure that we find a good solution and use it as examples for the future! :)
Comment 3 Hsin-Yi Tsai [:hsinyi] 2012-07-29 18:50:42 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> Hsinyi, can you tackle this? 
> 
> Quoting bug 777188:
> 
Sure, let me study the reference bugs first. :)
Comment 4 Hsin-Yi Tsai [:hsinyi] 2012-08-01 01:57:52 PDT
Created attachment 647886 [details] [diff] [review]
WIP

WIP: add 'RIL:RegisterSendAsync' and 'RIL:UnregisterSendAsync' messages

Here's my rough idea about the possible solution for this issue.
Please kindly note that this patch is *NOT* completed yet. 
And sorry there are temporary debug messages which will be removed eventually.
However, I'd like to ask for some feedback and launch discussion for a better solution. :)

In constructor of RILContentHelper, the message 'RIL:RegisterSendAsync' is sent. Then, 'RadioInterfaceLayer.js' adds the target into '_registeredTargets'. Every time when a message needs to be asynchronously sent to content, we retrieve targets from '_registeredTargets' and use them to 'sendAsyncMessage', instead of 'ppmm'.

How do you think about the whole concept? Thanks.
Comment 5 Hsin-Yi Tsai [:hsinyi] 2012-08-01 02:00:35 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4)
> Created attachment 647886 [details] [diff] [review]
> WIP
> 
> WIP: add 'RIL:RegisterSendAsync' and 'RIL:UnregisterSendAsync' messages
> 
> Here's my rough idea about the possible solution for this issue.
> Please kindly note that this patch is *NOT* completed yet. 
> And sorry there are temporary debug messages which will be removed
> eventually.
> However, I'd like to ask for some feedback and launch discussion for a
> better solution. :)
> 
> In constructor of RILContentHelper, the message 'RIL:RegisterSendAsync' is
> sent. Then, 'RadioInterfaceLayer.js' adds the target into
> '_registeredTargets'. Every time when a message needs to be asynchronously
> sent to content, we retrieve targets from '_registeredTargets' and use them
> to 'sendAsyncMessage', instead of 'ppmm'.
> 
> How do you think about the whole concept? Thanks.

The other question is when to send 'UnregisterSendAsync' from RILContentHelper?
Comment 6 Philipp von Weitershausen [:philikon] 2012-08-03 15:54:40 PDT
Comment on attachment 647886 [details] [diff] [review]
WIP

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

::: dom/system/gonk/RILContentHelper.js
@@ +138,4 @@
>  
>    this.initRequests();
>    this.initMessageListener(RIL_IPC_MSG_NAMES);
> +  cpmm.sendAsyncMessage("RIL:RegisterSendAsync", {target:this});

Hmm, I'm not sure how this is supposed to work. You're sending `this`, an object, over the wire here. I imagine it will be JSON-ified and resurface as *something* on the other end, but probably not what you want.

You're on the right track though, but in reality I think it's simpler. I think you can just send an empty message and then in RadioInterfaceLayer::receiveMessage() you just look at `msg.target`.


But let's also back up for a moment and take a look at the kinds of messages that are sent between RadioInterfaceLayer and RILContentHelper:

(a) broadcasts from RadioInterfaceLayer to RILContentHelper, mostly for event firing. Here, RILContentHelper needs to register itself with RadioInterfaceLayer so we know which processes to send the information to. But not all content processes are the same! We have 3 different APIs:

  1. Telephony
  2. SMS
  3. MobileConnection

Each of those have their own permissions. For instance, we should make sure that telephony related events are only sent to sites with permissions to telephony. We can go even further and say that only content processes that have successfully instantiated navigator.mozTelephony should be able to receive these messages.

(b) requests from RILContentHelper to RadioInterfaceLayer that receive a response. Here we want to *only* reply to the content process that made the request. Not to any other. We need to store the original `msg.target` for that and use it in the reply.

So, as you can see, this is a fair bit of work and will require some structural changes. (a) and (b) are pretty independent, though, so they can be tackled at least in separate patches, if not even separate bugs.
Comment 7 Hsin-Yi Tsai [:hsinyi] 2012-08-05 19:48:56 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> 
> But let's also back up for a moment and take a look at the kinds of messages
> that are sent between RadioInterfaceLayer and RILContentHelper:
> 
> (a) broadcasts from RadioInterfaceLayer to RILContentHelper, mostly for
> event firing. Here, RILContentHelper needs to register itself with
> RadioInterfaceLayer so we know which processes to send the information to.
> But not all content processes are the same! We have 3 different APIs:
> 
>   1. Telephony
>   2. SMS
>   3. MobileConnection
> 
> Each of those have their own permissions. For instance, we should make sure
> that telephony related events are only sent to sites with permissions to
> telephony. We can go even further and say that only content processes that
> have successfully instantiated navigator.mozTelephony should be able to
> receive these messages.
> 

Philipp, thank you very much for the detailed explanation. 

According to your feedback, regarding (a), my current thought is:
1) Use 'nsIWindowWatcher' to get content windows. 
2) Then, use 'nsIPermissionManager' to check each content's permission. According to the permission, allowed contents will be kept in a corresponding target list according to the permission. So, there will be 3 target lists related to permission "telephony", "sms" and "mobileconnection". 
3) When a message is ready for be broadcast to RILContentHelper, RadioInterfaceLayer will check the target of each message, retrieve the right content from the lists, and then 
'*content*.QueryInterface(Ci.nsIFrameMessageManager).sendAsynMessage();'

How do you think about this? Is this in the right direction? 

> (b) requests from RILContentHelper to RadioInterfaceLayer that receive a
> response. Here we want to *only* reply to the content process that made the
> request. Not to any other. We need to store the original `msg.target` for
> that and use it in the reply.
> 

Very clear to me.  Thanks for the feedback again :)
Comment 8 Hsin-Yi Tsai [:hsinyi] 2012-08-05 20:02:08 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> (In reply to Philipp von Weitershausen [:philikon] from comment #6)
> > 
> > But let's also back up for a moment and take a look at the kinds of messages
> > that are sent between RadioInterfaceLayer and RILContentHelper:
> > 
> > (a) broadcasts from RadioInterfaceLayer to RILContentHelper, mostly for
> > event firing. Here, RILContentHelper needs to register itself with
> > RadioInterfaceLayer so we know which processes to send the information to.
> > But not all content processes are the same! We have 3 different APIs:
> > 
> >   1. Telephony
> >   2. SMS
> >   3. MobileConnection
> > 
> > Each of those have their own permissions. For instance, we should make sure
> > that telephony related events are only sent to sites with permissions to
> > telephony. We can go even further and say that only content processes that
> > have successfully instantiated navigator.mozTelephony should be able to
> > receive these messages.
> > 
> 
> Philipp, thank you very much for the detailed explanation. 
> 
> According to your feedback, regarding (a), my current thought is:
> 1) Use 'nsIWindowWatcher' to get content windows. 
> 2) Then, use 'nsIPermissionManager' to check each content's permission.
> According to the permission, allowed contents will be kept in a
> corresponding target list according to the permission. So, there will be 3
> target lists related to permission "telephony", "sms" and
> "mobileconnection". 
> 3) When a message is ready for be broadcast to RILContentHelper,
> RadioInterfaceLayer will check the target of each message, retrieve the
> right content from the lists, and then 
> '*content*.QueryInterface(Ci.nsIFrameMessageManager).sendAsynMessage();'
> 
Remarks: the 3 steps are meant to be taken in RadioInterfaceLayer.

> How do you think about this? Is this in the right direction? 
>
Comment 9 Philipp von Weitershausen [:philikon] 2012-08-06 16:51:58 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> According to your feedback, regarding (a), my current thought is:
> 1) Use 'nsIWindowWatcher' to get content windows. 

These don't show up in the chrome process.

> 2) Then, use 'nsIPermissionManager' to check each content's permission.

Unfortunately we don't have a good way to do that in the chrome process.

It turns out, we need a bunch of platform work to make this happen. In order: bug 776825, then bug 776832, then bug 776834.
Comment 10 Hsin-Yi Tsai [:hsinyi] 2012-08-08 02:52:37 PDT
Created attachment 650009 [details] [diff] [review]
WIP part(a1): Address telephony-related event.

'RILContentHelper' sends 'RIL:RegisterSendAsync' to RadioInterfaceLayer when a content 'addEventListener'. Similarly, send 'RIL:UnregisterSendAsync' when a content 'removeEventListener'.

'RadioInterfaceLayer' maintains the lists of registered targets in terms of permission types. Only content with 'telephony' permission receives telephony-related events.

Keep working on situations of 'mobileconnection', 'voicemail' and 'sms'
Comment 11 Hsin-Yi Tsai [:hsinyi] 2012-08-08 03:04:43 PDT
Created attachment 650010 [details] [diff] [review]
WIP part(a1): Address telephony-related event.

Explanation in Comment 10.
Comment 12 Hsin-Yi Tsai [:hsinyi] 2012-08-12 20:56:33 PDT
Created attachment 651261 [details] [diff] [review]
Part 1: handle request messages

Per part(b) in comment 6.
Comment 13 Mozilla RelEng Bot 2012-08-13 06:00:28 PDT
Try run for 26c2b3f61fe8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=26c2b3f61fe8
Results (out of 264 total builds):
    exception: 1
    success: 217
    warnings: 33
    failure: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/htsai@mozilla.com-26c2b3f61fe8
Comment 14 Philipp von Weitershausen [:philikon] 2012-08-14 10:46:19 PDT
Comment on attachment 651261 [details] [diff] [review]
Part 1: handle request messages

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

::: dom/system/gonk/RILContentHelper.js
@@ +571,5 @@
> +        if (msg.json.success) {
> +          this.fireRequestSuccess(msg.json.options.requestId, msg.json.options);
> +        } else {
> +          this.fireRequestError(msg.json.options.requestId, msg.json.options.errorMsg);
> +        }

Nice. Please see bug 774114, it does something similar here. It would be good if you could rebase on top of that patch.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +452,5 @@
> +    // Maintain targets for request messages.
> +    let targets = this[requestType];
> +    if (!targets) {
> +      targets = this[requestType] = [];
> +    }

I don't think this is right. We don't want to look up message managers by message type. We might broadcast information to a process that shouldn't get the information that way.

Instead we should look them up by requestId. If our code is done right, we should already cycle requestId to and from ril_worker, so all response messages from ril_worker should already include it. Otherwise the event notification RILContentHelper wouldn't work. The only place where this might not be the case yet is for enumerating voice calls, but we can easily add a `requestId` here and declare it to be part of the RILContentHelper <--> RadioInterfaceLayer protocol. In fact, it would be good to document it in a comment.

(Lastly, I don't think using `this` as the map is particularly clean.)

So I'm thinking something like:

  handleRequestTarget: function handleRequestTarget(msg) {
    let requestId = msg.json.requestId;
    if (!requestId) {
      // The content process isn't interested in a response.
      return;
    }
    let mm = msg.target.QueryInterface(Ci.nsIFrameMessageManager);
    this._messageManagersByRequest[requestId] = mm;
  }

And then you get look up the message manager later and send the return message, e.g.:

  let target = this._messageManagersByRequest[message.requestId];
  target.sendAsyncMessage("RIL:GetAvailableNetworks", message);

Note that this way you also won't have to rename a bunch of the messages.
Comment 15 Hsin-Yi Tsai [:hsinyi] 2012-08-14 21:14:12 PDT
Created attachment 651992 [details] [diff] [review]
Part2: telephony-permission events

Part 2: RadioInterfaceLayer.js send unsolicited telephony-related messages only to contents who listen to the events and are authorized telephony permission.
Comment 16 Hsin-Yi Tsai [:hsinyi] 2012-08-14 23:28:17 PDT
Created attachment 652011 [details] [diff] [review]
Part2: telephony-permission events

New: this._messageManagerByPermission
Comment 17 Hsin-Yi Tsai [:hsinyi] 2012-08-14 23:35:09 PDT
Comment on attachment 652011 [details] [diff] [review]
Part2: telephony-permission events

A content registers as a target of telephony-related messages when the content 'addEventListener' to the telephony-related events.
Comment 18 Hsin-Yi Tsai [:hsinyi] 2012-08-16 04:44:21 PDT
Created attachment 652396 [details] [diff] [review]
Part 1: handle request messages (v2)

Updated: 'this._messageManagerByRequest'
Comment 19 Hsin-Yi Tsai [:hsinyi] 2012-08-16 04:48:11 PDT
Comment on attachment 652396 [details] [diff] [review]
Part 1: handle request messages (v2)

This update addressed Comment 14 and rebased on Jose's patch of Bug 774114. 
Thanks for your comments, Philipp!
Comment 20 Philipp von Weitershausen [:philikon] 2012-08-16 14:35:45 PDT
Comment on attachment 652396 [details] [diff] [review]
Part 1: handle request messages (v2)

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

r=me with comments below addressed.

::: dom/system/gonk/RILContentHelper.js
@@ +692,5 @@
>      }
>    },
>  
> +  _getRandomId: function _getRandomId() {
> +    return Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator)

Please create a lazy service getter for this, e.g.:

  XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
                                     "@mozilla.org/uuid-generator;1",
                                     "nsIUUIDGenerator");

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +443,5 @@
>                          message.rilMessageType);
>      }
>    },
>  
> +  _messageManagerByRequest: {},

Initializing this on the prototype isn't problematic for a singleton object like RadioInterfaceLayer, but it's still a bit unclean. I suggest setting it to `null` here and initializing it in the constructor.

@@ +1087,3 @@
>      debug("Requesting enumeration of calls for callback");
> +    this.worker.postMessage({rilMessageType: "enumerateCalls",
> +                             requestId: requestId});

Alternatively, do what we do in the other methods: pass msg.json to enumerateCalls and just add `rilMessageType` to the object before sending it to the worker.
Comment 21 Philipp von Weitershausen [:philikon] 2012-08-16 14:38:54 PDT
Comment on attachment 652396 [details] [diff] [review]
Part 1: handle request messages (v2)

P.S.: One last naming nit: I think `saveRequestTarget` or similar is a better name than `handleRequestTargets`. Because (a) we're only dealing with one target a time, and (b) handleFoobar is already a convention within RadioInterfaceLayer and stands for something else.
Comment 22 Philipp von Weitershausen [:philikon] 2012-08-16 15:05:45 PDT
Comment on attachment 652011 [details] [diff] [review]
Part2: telephony-permission events

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

::: dom/system/gonk/nsIRadioInterfaceLayer.idl
@@ +144,5 @@
>    void registerVoicemailCallback(in nsIRILVoicemailCallback callback);
>    void unregisterVoicemailCallback(in nsIRILVoicemailCallback callback);
>  
>    /**
> +   * Called when a content registers/unregiters receiving asynchronous messages

Typo: unregisters

::: dom/telephony/Telephony.cpp
@@ +262,5 @@
>  
> +// nsIDOMEventTarget
> +
> +NS_IMETHODIMP
> +Telephony::AddEventListener(const nsAString& aType,

This is a great idea and also pretty elegant, but unfortunately not sufficient, because e.g. the `.calls` and `.active` attributes need to reflect reality without event listeners being registered. Furthermore, it is likely to leak because when a page gets unloaded, we don't clean up after ourselves.

If we have content processes register themselves with the chrome process, then we definitely need to ensure proper clean up. Alternatively, we could provide a helper in the chrome process to enumerate all content processes that have a certain permission.

Independently of which solution we decide to go with, I suggest we do this in a follow-up bug. That way we can go ahead and land part 1. Progress in small steps! :)
Comment 23 Philipp von Weitershausen [:philikon] 2012-08-16 15:13:01 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #22)
> Independently of which solution we decide to go with, I suggest we do this
> in a follow-up bug. That way we can go ahead and land part 1. Progress in
> small steps! :)

Fled bug 783392. Morphing this bug to just be about request-based responses.
Comment 24 Hsin-Yi Tsai [:hsinyi] 2012-08-16 19:17:07 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #22)
> Comment on attachment 652011 [details] [diff] [review]
> Part2: telephony-permission events
> 
> Review of attachment 652011 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIRadioInterfaceLayer.idl
> @@ +144,5 @@
> >    void registerVoicemailCallback(in nsIRILVoicemailCallback callback);
> >    void unregisterVoicemailCallback(in nsIRILVoicemailCallback callback);
> >  
> >    /**
> > +   * Called when a content registers/unregiters receiving asynchronous messages
> 
> Typo: unregisters
> 
> ::: dom/telephony/Telephony.cpp
> @@ +262,5 @@
> >  
> > +// nsIDOMEventTarget
> > +
> > +NS_IMETHODIMP
> > +Telephony::AddEventListener(const nsAString& aType,
> 
> This is a great idea and also pretty elegant, but unfortunately not
> sufficient, because e.g. the `.calls` and `.active` attributes need to
> reflect reality without event listeners being registered. 

Oh, that's true. Thanks for pointing this out. I should have had it on my mind :)

> 
> Independently of which solution we decide to go with, I suggest we do this
> in a follow-up bug. That way we can go ahead and land part 1. Progress in
> small steps! :)

Agree!
Comment 25 Hsin-Yi Tsai [:hsinyi] 2012-08-16 23:08:25 PDT
Created attachment 652684 [details] [diff] [review]
Patch: handle request messages (v3)

r=philikon with Comment 20 addressed.
Comment 27 Ed Morley [:emorley] 2012-08-17 05:26:37 PDT
https://hg.mozilla.org/mozilla-central/rev/e1cd9fb39dd7

Note You need to log in before you can comment on or make changes to this bug.