952371 - [B2G] [DSDS] mozMobileconnection.lastKnownNetwork and .lastKnownHomeNetwork aren't working

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

This bug was originally reported on https://bugzilla.mozilla.org/show_bug.cgi?id=948222#c21.

The root cause is we still use one preference to record two SIMs. We should have separate preferences for each SIM. Re-design the preference format for "ril.lastKnownNetwork" and "ril.lastKnownHomeNetwork" is required.

Comment 2

[Jessica Jong [:jessica] (inactive)]

I am wondering why mozMobileconnection.lastKnownNetwork and .lastKnownHomeNetwork were designed using preferences, and not like the other APIs, go through the RILContentHelper -> RadioInterfaceLayer flow.
I mean, can we still have two different permissions, mobilenetwork and mobileconnection, and mobilenetwork will still have access only to lastKnownNetwork and lastKnownHomeNetwork, but the values are retrieved directly from RadioInterfaceLayer? Will this break the current security model?

Sorry if this is something trivial...

Comment 3

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

(In reply to Jessica Jong [:jjong] [:jessica] from comment #2)
> I am wondering why mozMobileconnection.lastKnownNetwork and
> .lastKnownHomeNetwork were designed using preferences, and not like the
> other APIs, go through the RILContentHelper -> RadioInterfaceLayer flow.
> I mean, can we still have two different permissions, mobilenetwork and
> mobileconnection, and mobilenetwork will still have access only to
> lastKnownNetwork and lastKnownHomeNetwork, but the values are retrieved
> directly from RadioInterfaceLayer? Will this break the current security
> model?
> 
> Sorry if this is something trivial...

I tried to get more clues from Bug 866272 but in vain. 

Hi, Fabrice, may I have your explanation here? Thank you.

Comment 4

[[:fabrice] Fabrice Desré]

That was done this way because when RadioInterfaceLayer gets the IPC message to retrieve lastKnownNetwork, there is no way to check for mobilenetwork || mobileconnection. If any of these fail, we end up killing the child process.

We could probably do it without preferences by sending different IPC messages based on whether the child has mobilenetwork or mobileconnection permission. I didn't do it for lack of time when adding this...

Comment 5

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes

Comment 6

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ril implementation

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: tests

Comment 9

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8351322 [details] [diff] [review]
Part 4: tests

I am just testing the basic functions, cause to test other cases, e.g. changing the operator, would require listening events after sending the corresponding commands to the emulator, which would require 'mobileconnection' permission.

Comment 10

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

Comment on attachment 8351318 [details] [diff] [review]
Part 1: idl changes

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

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +48,5 @@
>    void unregisterMobileConnectionMsg(in unsigned long clientId,
>                                       in nsIMobileConnectionListener listener);
>  
> +  DOMString getLastKnownNetwork(in unsigned long clientId);
> +  DOMString getLastKnownHomeNetwork(in unsigned long clientId);

Please add a comment saying 'mobilenetwork' permission is required for accessing these two functions.

Comment 11

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

Comment on attachment 8351321 [details] [diff] [review]
Part 3: ril implementation

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +241,5 @@
>          ppmm.addMessageListener(msgname, this);
>        }
> +      for (let msgname of RIL_IPC_MOBILENETWORK_MSG_NAMES) {
> +        ppmm.addMessageListener(msgname, this);
> +      }

r- because we need 'ppmm.removeMessageListener' for RIL_IPC_MOBILENETWORK_MSG_NAMES as well.

Comment 12

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

Comment on attachment 8351322 [details] [diff] [review]
Part 4: tests

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

Thank you!!!

::: dom/network/tests/marionette/test_mobile_last_known_network.js
@@ +44,5 @@
> +  finish();
> +}
> +
> +runNextTest();
> +

nit: no new line at the end of a file

Comment 13

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

1.3? because this is a miss in DSDS implementation.

Comment 14

[Ken Chang[:kenkai]]

Yes, it is a 1.3+ issue.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: idl changes (final)

Address review comment 10:
- add a comment saying 'mobilenetwork' permission is required for accessing the two functions.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: ril implementation (final)

Address review comment 11:
- add 'ppmm.removeMessageListener' for RIL_IPC_MOBILENETWORK_MSG_NAMES.

Comment 17

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: tests (final)

Address review comment 12:
- no new line at the end of a file.

Comment 18

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8351319 [details] [diff] [review]
Part 2: dom changes

smaug,

We are using IPC messages instead of preferences to query last known networks. May I have your review on this?

Thanks.

Comment 19

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

Comment on attachment 8355989 [details] [diff] [review]
Part 3: ril implementation (final)

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

Great, thanks!

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: dom changes (final)

Add f=hsinyi r=smaug.

Comment 21

[Jessica Jong [:jessica] (inactive)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=75fd2b952561

Comment 22

[Jessica Jong [:jessica] (inactive)]

Full try: https://tbpl.mozilla.org/?tree=Try&rev=bf4bb913b2d5
(Rerun with latest healthy codebase and all green!)

Comment 23

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

I modified the comments of patches a little bit to provide more information of what the patches have been trying to do. Otherwise, the comment "Part 1 - idl changes" for example is too rough.

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/3826c3a2d5ee
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/3fd3861f9778
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/50101ad17ae7
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/3b867c474f45

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3826c3a2d5ee
https://hg.mozilla.org/mozilla-central/rev/3fd3861f9778
https://hg.mozilla.org/mozilla-central/rev/50101ad17ae7
https://hg.mozilla.org/mozilla-central/rev/3b867c474f45

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/031315f1fe0b
https://hg.mozilla.org/releases/mozilla-aurora/rev/f58b2c2c9f27
https://hg.mozilla.org/releases/mozilla-aurora/rev/1aab8c2a2114
https://hg.mozilla.org/releases/mozilla-aurora/rev/ef25b3b02dcc