Closed Bug 783392 Opened 7 years ago Closed 7 years ago

RadioInterfaceLayer broadcasts radio-state information to all content processes

Categories

(Core :: DOM: Device Interfaces, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: philikon, Assigned: hsinyi)

References

Details

(Whiteboard: [LOE:M])

Attachments

(1 file, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #777202 +++

In bug 777202 we've made sure that we don't broadcast responses to request-based IPC messages to all content processes, but instead only post to the process the request originated from.

Events that initiate in the RIL, however (e.g. incoming calls, SMS, network registration state changes, etc.) are still broadcast to all content processes. They should only be sent to the ones that have the appropriate privileges. This will at the very least require bug 776832 for the chrome-side permission check, but possibly we'll also need a a helper to enumerate processes by permission.
When is there a situation in which there are listeners that would need these notifications but haven't addEventListener()'d or set up to receive system messages?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> When is there a situation in which there are listeners that would need these
> notifications but haven't addEventListener()'d or set up to receive system
> messages?

The whole system messages thing is new, at least to me. Do we have that written up somewhere? I have no idea how it's supposed to work.

But yeah, we expose lots of information through attributes that should reflect the right data, even if content hasn't registered event listeners. `navigator.mozTelephony.active` being non-null for instance (as an indicator that you're on the phone) or `navigator.mozMobileConnection.voice.signalStrength` (and many other attributes), just to give two examples.
The way we handle that from C++ is to synchronously request current state when content first asks, and then leave behind a listener to receive updates.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> The way we handle that from C++ is to synchronously request current state
> when content first asks, and then leave behind a listener to receive updates.

Good to know. That's kind of what I had in mind, too. MobileConnection and Telephony already request state like that (although Telephony does it asynchronously at the moment, which can actually lead to interesting edge cases... but that's another story).
Are you the proper owner here, philikon?
Assignee: nobody → philipp
Whiteboard: [WebAPI:P0]
(In reply to Andrew Overholt [:overholt] from comment #5)
> Are you the proper owner here, philikon?

Asking Hsinyi to take care of this if she doesn't mind.
Assignee: philipp → htsai
Target Milestone: --- → mozilla17
Version: Trunk → Other Branch
sorry I accidentally modified version and branch
Target Milestone: mozilla17 → ---
Version: Other Branch → Trunk
Whiteboard: [WebAPI:P0] → [WebAPI:P0][LOE:M]
Whiteboard: [WebAPI:P0][LOE:M] → [WebAPI:P0][LOE:L]
Here are my thoughts. Send register messages to chrome to request radio states when content processes that have successfully instantiated navigator.mozTelephony or navigator.mozMobileConnection. 
And send unregister messages in destructor. The messages contain 'permission' info. So, RadioInterfaceLayer manages the message targets by permission.

Also, per Philipp's comments in bug 777202, we shall also carefully handle the situation when a child ends. So, this will depend on bug 777508. We will unregister contents when receiving a 'shutdown' message.

How do you think about this? Thanks!
Depends on: 777508
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> And send unregister messages in destructor.

Actually, as per bug 777508, we're automatically going to get an IPC message when a child process is terminated. I think that's enough.

> The messages contain
> 'permission' info. So, RadioInterfaceLayer manages the message targets by
> permission.

As per bug 776663 we now assert permissions on all incoming IPC messages. The registration messages should not be treated any differently.
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > And send unregister messages in destructor.
> 
> Actually, as per bug 777508, we're automatically going to get an IPC message
> when a child process is terminated. I think that's enough.
> 
> > The messages contain
> > 'permission' info. So, RadioInterfaceLayer manages the message targets by
> > permission.
> 
> As per bug 776663 we now assert permissions on all incoming IPC messages.
> The registration messages should not be treated any differently.

Thanks for the feedback. I'll take a look at those reference bugs and make registration right!
(In reply to Philipp von Weitershausen [:philikon] from comment #9)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > And send unregister messages in destructor.
> 
> Actually, as per bug 777508, we're automatically going to get an IPC message
> when a child process is terminated. I think that's enough.

To be clear, by "that's enough" I meant listening for that message ('child-process-shutdown') and then unregistering the corresponding message manager.
(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> (In reply to Philipp von Weitershausen [:philikon] from comment #9)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > > And send unregister messages in destructor.
> > 
> > Actually, as per bug 777508, we're automatically going to get an IPC message
> > when a child process is terminated. I think that's enough.
> 
> To be clear, by "that's enough" I meant listening for that message
> ('child-process-shutdown') and then unregistering the corresponding message
> manager.

Yes, that's what I think as well.
Whiteboard: [WebAPI:P0][LOE:L] → [WebAPI:P0][LOE:M]
Attached patch Patch (obsolete) — Splinter Review
Send register messages to chrome to request radio states when content processes that have successfully instantiated navigator.mozTelephony or navigator.mozMobileConnection. 

Chrome manages the registration lists. Only the registered and granted contents receive the unsolicited messages.

Chrome will handle 'unregister' when chrome receives 'child-process-shutdown' message, so content doesn't need to send unregister messages.
Attachment #668907 - Flags: review?(marshall)
Comment on attachment 668907 [details] [diff] [review]
Patch

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

Thanks Hsin-Yi, this looks great :) r- just because we'll need to support a separate "voicemail" target because of the "voicemail" permission. Also, we shouldn't land this patch without accompanying tests.

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +23,5 @@
> +   * mobileconnection permission is allowed to register. Note that a content
> +   * doesn't need to unregister because the chrome process will remove it from
> +   * the registration list once the chrome receives a 'child-process-shutdown'
> +   * message.
> +   */

nit: mentions of "a content" should probably be changed to just "content" or alternatively, "a content process" :)

@@ +24,5 @@
> +   * doesn't need to unregister because the chrome process will remove it from
> +   * the registration list once the chrome receives a 'child-process-shutdown'
> +   * message.
> +   */
> +  void registerMobileConnectionMsg();

We will need a matching registration call for voicemail, since it has it's own permission (though I'm open to merging voicemail into the telephony permission in a follow up).

The call should come from the Voicemail constructor:
https://mxr.mozilla.org/mozilla-central/source/dom/telephony/Voicemail.cpp#42

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +58,5 @@
>    "RIL:AnswerCall",
>    "RIL:RejectCall",
>    "RIL:HoldCall",
>    "RIL:ResumeCall",
> +  "RIL:RegisterTelephonyMsg",

nit: trailing comma here and below on line 78

@@ +521,5 @@
>                                         message.contacts);
>          }
>          break;
>        case "iccmbdn":
> +        this._sendMobileConnectionMessage("RIL:VoicemailNumberChanged", message);

Just a reminder to change this to _sendVoicemailMessage (or whatever it ends up being called) when voicemail targets are supported, and below on line 1202.

@@ +593,5 @@
> +      targets = this._messageManagerByPermission[permission] = [];
> +    }
> +
> +    if (targets.indexOf(target) != -1) {
> +      throw new Error("Already registered this target!");

Is it necessary to throw an exception instead of early return here?

@@ +620,5 @@
> +
> +  registerMobileConnectionTarget: function registerMobileConnectionTarget(target) {
> +    this.registerMessageTarget("mobileconnection", target);
> +  },
> +

Voicemail targets should also have their own register/unregister since there is a separate voicemail permission (see above)
Attachment #668907 - Flags: review?(marshall) → review-
(In reply to Marshall Culpepper [:marshall_law] from comment #15)
> Comment on attachment 668907 [details] [diff] [review]
> Patch
> 
> Review of attachment 668907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Hsin-Yi, this looks great :) r- just because we'll need to support a
> separate "voicemail" target because of the "voicemail" permission. Also, we
> shouldn't land this patch without accompanying tests.
> 
Marshall, thanks for the comments. I'll add 'voicemail' case and address your concerns.
 
B.t.w, I ran the marionette (telephony and mobileconnection) tests and pushed to tryserver to examine this patch. Isn't that enough?
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> B.t.w, I ran the marionette (telephony and mobileconnection) tests and
> pushed to tryserver to examine this patch. Isn't that enough?

The current tests are good, but AFAICT they would only test that content processes with the right permission receive the proper RIL events. It would be good to also verify the opposite, i.e. content processes without the telephony permission should _not_ receive telephony RIL events. Implementing this may be a bit tricky though..
Attached patch Patch - v2 (obsolete) — Splinter Review
WIP: Comment 15 addressed.

Need to keep working on the error below:
E/GeckoConsole(  105): [JavaScript Error: "NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]" {file: "     jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js" line: 670}]
Attachment #668907 - Attachment is obsolete: true
Attached patch Patch (v3)Splinter Review
Comment 15 addressed -- manage message targets per 'telephony', 'mobile connection' and 'voice mail' permission.
Attachment #670228 - Attachment is obsolete: true
Comment on attachment 670743 [details] [diff] [review]
Patch (v3)

Hi Marshall,

The revision addresses your comment 15. It passed marionette tests (telephony, mobileconnection and voicemail). Tryserver results (https://tbpl.mozilla.org/?tree=Try&rev=cc7c009ec3c9) looked good, too. Would you please help review again? Thanks!
Attachment #670743 - Flags: review?(marshall)
(In reply to Marshall Culpepper [:marshall_law] from comment #17)
> The current tests are good, but AFAICT they would only test that content
> processes with the right permission receive the proper RIL events. It would
> be good to also verify the opposite, i.e. content processes without the
> telephony permission should _not_ receive telephony RIL events. Implementing
> this may be a bit tricky though..

Content without telephony permission cannot obtain 'mozTelephony' in the first place, so it cannot receive any later telephony RIL events. I can provide a test showing that 
==============================
SpecialPowers.addPermission("telephony", false, document); 
let telephony = window.navigator.mozTelephony;
is(telephony, null); 
==============================

I am not sure whether this is what you expect, but this is the testcase I can come up with. More suggestions?
Priority: -- → P2
Whiteboard: [WebAPI:P0][LOE:M] → [LOE:M]
Comment on attachment 670743 [details] [diff] [review]
Patch (v3)

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

Awesome, thanks for updating this Hsin-Yi :). Don't worry about testing lack of permissions in Marionette, as you said, the mozTelephony API point won't even be accessible in that case.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +391,5 @@
>          break;
>        case "RIL:SelectNetworkAuto":
>          this.saveRequestTarget(msg);
>          this.selectNetworkAuto(msg.json.requestId);
> +        break;

eek, good catch!
Attachment #670743 - Flags: review?(marshall) → review+
https://hg.mozilla.org/mozilla-central/rev/9098fb489c6e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 670743 [details] [diff] [review]
Patch (v3)

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

Nice work, Hsin-Yi! I have some follow-up comments below that I would love to see addressed, mostly for code clarity and future proofing. Could you address these in a follow-up bug please? Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +303,5 @@
> +      // already forgotten its permissions so we need to unregister for both
> +      // telephony and mobile connection messages.
> +      this.unregisterTelephonyTarget(msg.target);
> +      this.unregisterMobileConnectionTarget(msg.target);
> +      this.unregisterVoicemailTarget(msg.target);

The defensive programmer in me would like to make this a bit more future-proof (memory leaks are scary!). Let's change the `unregisterMessageTarget()` method to treat a `null` or '*' permission as "go through all lists and remove `target`". Then we only have to call it once here for all eternity.

@@ +654,5 @@
> +  },
> +
> +  unregisterVoicemailTarget: function unregisterVoicemailTarget(target) {
> +    this.unregisterMessageTarget("voicemail", target);
> +  },

These 6 helpers are a lot of code noise that seems unnecessary. There seem to be no upside to not inlining the calls to [un]registerMessageTarget() directly, just downsides.

@@ +666,5 @@
> +    let targets = thisTargets.slice();
> +    for each (let target in targets) {
> +      if (thisTargets.indexOf(target) == -1) {
> +        continue;
> +      }

I don't understand this dance. Why the slice() and then checking for containment? Seems like wasted cycles to me.

@@ +681,5 @@
> +  },
> +
> +  _sendVoicemailMessage: function sendVoicemailMessage(message, options) {
> +    this._sendTargetMessage("voicemail", message, options);
> +  },

Same as above for these 3 helpers: let's inline them.
Hi Philipp, 

Thanks for the comment. I've  filed bug 803789 for addressing it.
You need to log in before you can comment on or make changes to this bug.