Closed Bug 777202 Opened 12 years ago Closed 12 years ago

RadioInterfaceLayer broadcasts request responses to all content processes

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
blocking-basecamp +

People

(Reporter: cjones, Assigned: hsinyi)

References

Details

Attachments

(1 file, 7 obsolete files)

It should only send it to clients with telephony privileges.
blocking-basecamp: --- → +
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.
Assignee: nobody → htsai
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! :)
(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. :)
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #647886 - Flags: feedback?(philipp)
(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 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.
Attachment #647886 - Flags: feedback?(philipp)
(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 :)
(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? 
>
(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.
'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'
Attachment #647886 - Attachment is obsolete: true
Explanation in Comment 10.
Attachment #650009 - Attachment is obsolete: true
Attached patch Part 1: handle request messages (obsolete) — Splinter Review
Per part(b) in comment 6.
Attachment #650010 - Attachment is obsolete: true
Attachment #651261 - Flags: review?(philipp)
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 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.
Attachment #651261 - Flags: review?(philipp)
Part 2: RadioInterfaceLayer.js send unsolicited telephony-related messages only to contents who listen to the events and are authorized telephony permission.
New: this._messageManagerByPermission
Attachment #651992 - Attachment is obsolete: true
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.
Attachment #652011 - Flags: review?(philipp)
Updated: 'this._messageManagerByRequest'
Attachment #651261 - Attachment is obsolete: true
Attachment #652396 - Flags: review?
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!
Attachment #652396 - Flags: review? → review?(philipp)
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.
Attachment #652396 - Flags: review?(philipp) → review+
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 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! :)
Attachment #652011 - Flags: review?(philipp) → review-
(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.
Summary: RadioInterfaceLayer broadcasts radio-state information to all content processes → RadioInterfaceLayer broadcasts request responses to all content processes
Attachment #652011 - Attachment is obsolete: true
(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!
r=philikon with Comment 20 addressed.
Attachment #652396 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e1cd9fb39dd7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: