783392 - RadioInterfaceLayer broadcasts radio-state information to all content processes

Status: RESOLVED FIXED

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

Description

[Philipp von Weitershausen [:philikon]]

+++ 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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 2

[Philipp von Weitershausen [:philikon]]

(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.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 4

[Philipp von Weitershausen [:philikon]]

(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).

Comment 5

[Andrew Overholt [:overholt]]

Are you the proper owner here, philikon?

Comment 6

[Philipp von Weitershausen [:philikon]]

(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.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

sorry I accidentally modified version and branch

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 9

[Philipp von Weitershausen [:philikon]]

(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.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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!

Comment 11

[Philipp von Weitershausen [:philikon]]

(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.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch

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.

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

try https://tbpl.mozilla.org/?tree=Try&rev=c3fc570df41d

Comment 15

[Marshall Culpepper [:marshall_law]]

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)

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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?

Comment 17

[Marshall Culpepper [:marshall_law]]

(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..

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch - v2

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}]

Comment 19

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: Patch (v3)

Comment 15 addressed -- manage message targets per 'telephony', 'mobile connection' and 'voice mail' permission.

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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?

Comment 22

[Marshall Culpepper [:marshall_law]]

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!

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9098fb489c6e

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9098fb489c6e

Comment 25

[Philipp von Weitershausen [:philikon]]

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.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/60554f99f313

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Philipp, 

Thanks for the comment. I've  filed bug 803789 for addressing it.