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

RESOLVED FIXED in Firefox 28

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: hsinyi, Assigned: jessica)

Tracking

unspecified
1.3 C2/1.4 S2(17jan)
x86_64
Linux
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

No description provided.
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.
Summary: [B2G] [DSDS] we dpmpreference ril.lastKnownNetwork and ril.lastKnownHomeNetwork → [B2G] [DSDS] mozMobileconnection.lastKnownNetwork and .lastKnownHomeNetwork aren't working
Blocks: 948222
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...
(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.
Flags: needinfo?(fabrice)
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...
Flags: needinfo?(fabrice)
See Also: → 947784
Posted patch Part 1: idl changes (obsolete) — Splinter Review
Posted patch Part 2: dom changes (obsolete) — Splinter Review
Posted patch Part 4: tests (obsolete) — Splinter Review
Attachment #8351318 - Flags: review?(htsai)
Attachment #8351319 - Flags: feedback?(htsai)
Attachment #8351321 - Flags: review?(htsai)
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.
Attachment #8351322 - Flags: review?(htsai)
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.
Attachment #8351318 - Flags: review?(htsai) → review+
Attachment #8351319 - Flags: feedback?(htsai) → feedback+
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.
Attachment #8351321 - Flags: review?(htsai) → review-
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
Attachment #8351322 - Flags: review?(htsai) → review+
1.3? because this is a miss in DSDS implementation.
blocking-b2g: --- → 1.3?
Yes, it is a 1.3+ issue.
blocking-b2g: 1.3? → 1.3+
Address review comment 10:
- add a comment saying 'mobilenetwork' permission is required for accessing the two functions.
Attachment #8351318 - Attachment is obsolete: true
Attachment #8355988 - Flags: review+
Address review comment 11:
- add 'ppmm.removeMessageListener' for RIL_IPC_MOBILENETWORK_MSG_NAMES.
Attachment #8351321 - Attachment is obsolete: true
Attachment #8355989 - Flags: review?(htsai)
Address review comment 12:
- no new line at the end of a file.
Attachment #8351322 - Attachment is obsolete: true
Attachment #8355990 - Flags: review+
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.
Attachment #8351319 - Flags: review?(bugs)
Comment on attachment 8355989 [details] [diff] [review]
Part 3: ril implementation (final)

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

Great, thanks!
Attachment #8355989 - Flags: review?(htsai) → review+
Attachment #8351319 - Flags: review?(bugs) → review+
Add f=hsinyi r=smaug.
Attachment #8351319 - Attachment is obsolete: true
Attachment #8356478 - Flags: review+
Attachment #8355988 - Attachment description: Part 1: idl changes. v2. → Part 1: idl changes (final)
Attachment #8355989 - Attachment description: Part 3: ril implementation, v2. → Part 3: ril implementation (final)
Attachment #8355990 - Attachment description: Part 4: tests, v2. → Part 4: tests (final)
Full try: https://tbpl.mozilla.org/?tree=Try&rev=bf4bb913b2d5
(Rerun with latest healthy codebase and all green!)
Assignee: nobody → jjong
Keywords: checkin-needed
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
Keywords: checkin-needed
Duplicate of this bug: 947784
You need to log in before you can comment on or make changes to this bug.