1067629 - Turn off radio control in System App when running test script for MozMobileConnection.SetRadioEnabled(false)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Anshul]

[Blocking Requested - why for this release]:

STR
1. Write a test script that calls MozMobileConnection.SetRadioEnabled(false)

Expected: Radio stays off as requested by the API
Observed: Radio turns back on.

This is a regression caused by bug 1038496.

Comment 1

[Anshul]

Fix for the Issue mentioned in bug 1038496 is already present in CAF builds and so is unnecessary and in fact, causing our test framework to fail as we rely on publicly exposed API MozMobileConnection.SetRadioEnabled heavily.

Comment 2

[bhavana bajaj [:bajaj]]

For 2.0 the solution is backing out 1038496.

For 2.1 arthur is investigating and will have an update here.

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Ansul, could you share more information about the test? If you are in the environment where system app is running, you should use "window.Radio.enabled = false" to turn off the radio instead of using the mobile connection API directly. Backing out the patch makes the device unable to recover the connection from the crash of rild.

Comment 4

[Anshul]

Arthur, I can experiment to see if our scripts have access to window.Radio.enabled as we are running in chrome context and have seen in the past that we don't have access to some APIs.

I still think that this decision to turn on radio once the phone recovers from RILD crash is to be done by RIL itself as the Gaia patch looks like a workaround rather than a clean solution.

Moreover mozMobileConnection API is a public API and so we are changing the behavior of that API with this path. Please correct me if my understanding of MozMobileConnection is incorrect.

Comment 5

[Anshul]

(In reply to Arthur Chen [:arthurcc] (PTO 9/17 ~ 9/23) from comment #3)
> Ansul, could you share more information about the test? If you are in the
> environment where system app is running, you should use
> "window.Radio.enabled = false" to turn off the radio instead of using the
Tested and found out that our scripts don't seem to have access to window.Radio object. I keep getting null radio object whether or not the script is running in chrome context. I think this may only be accessible to scripts running as Gaia UI tests.

> mobile connection API directly. Backing out the patch makes the device
> unable to recover the connection from the crash of rild.

Comment 6

[Michael Vines [:m1] [:evilmachines]]

(2.0+ to 2.2? since bug 1038496 has been backed out from v2.0/v2.1 however we'd still need a fix for v2.2)

Comment 7

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

(In reply to Anshul from comment #4)
> Arthur, I can experiment to see if our scripts have access to
> window.Radio.enabled as we are running in chrome context and have seen in
> the past that we don't have access to some APIs.
> 
> I still think that this decision to turn on radio once the phone recovers
> from RILD crash is to be done by RIL itself as the Gaia patch looks like a
> workaround rather than a clean solution.
> 

Hi Anshul,

I don't think this is a workaround. In our design, System/Settings app is the one memorizing and managing user preference. It is the one to react when the situation isn't the case user wants (the concept also applies for radio control at boot-up). 

> Moreover mozMobileConnection API is a public API and so we are changing the
> behavior of that API with this path. Please correct me if my understanding
> of MozMobileConnection is incorrect.

I am not getting your idea here. Why is this path changing the behaviour of mozMobileConneciton API? mozMobileConnection.setRadioEnabled is used for setting radio power. When System App detects the radio state doesn't meet user preference, it controls and manages the thing. So from the phone point of view, if anyone else somehow causes unexpected behaviour, System App is to call mozMobileConnection.setRadioEnabled to ensure radio state as users wish. Thus, mozMobileConnection.setRadioEnabled is still used for setting radio power.

I also want to echo Arthur on test environments. We should be very careful and sensitive to the goal of a test. If we are targeting on a unit-test of mozMobileConnection.setRadioEnabled() API, it's always important to have a clean environment, i.e. there shouldn't be another one calling .setRadioEnabled() to avoid interaction. If we are wanting to verify the whole behaviour on a phone, then based on the role of System App, it sounds reasonable that System app makes a decision and takes action. What's the CAF test aiming for, which level?

Comment 8

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> (In reply to Anshul from comment #4)
> > Arthur, I can experiment to see if our scripts have access to
> > window.Radio.enabled as we are running in chrome context and have seen in
> > the past that we don't have access to some APIs.
> > 
> > I still think that this decision to turn on radio once the phone recovers
> > from RILD crash is to be done by RIL itself as the Gaia patch looks like a
> > workaround rather than a clean solution.
> > 
> 
> Hi Anshul,
> 
> I don't think this is a workaround. In our design, System/Settings app is
> the one memorizing and managing user preference. It is the one to react when
> the situation isn't the case user wants (the concept also applies for radio
> control at boot-up). 

Let me offer more context and background of the design.

Gecko used to remember user preference and to be in charge of meeting the preference by using Settings api. But as time passed by, we (not just moz Telephony team, but also wifi team, system team...) learned that using a setting key to control hardware on gecko wasn't a good idea [1], because gaia had no idea about the process that restricted UI display due to the settings API restriction, i.e. settings key cannot represent a) user preference, b) request result, and c) current state at the same time.

To improve this, we shall design an API for gaia to control hardware enabling. In this way, gaia could know the request result (succeeds or not), keep the system state separate from user preference without ambiguity and confusion. With the kind of API, should gecko still be aware of user preference? It doesn't seem necessary given that we already provide an API for gaia to control.

But what happens if we do so? what happens if we want gecko to recover from unexpected situation and hide the current situation from gaia?
a) Gecko detects something unexpected. Gecko reads user preference and tries to recover it without dispatching real states to gaia.
==> This leads to state mismatched on gecko and gaia, and user could find it weird that everything looks right on UI but functionality broken.
b) Gecko detects something unexpected. Gecko reads user preference, tries to recover it but still likes to inform Gaia the current situation.
==> Okay, so now seems that we will need to provide additional signal to tell Gaia "hey, it's a special case. We want you to be aware but please don't handle it." That's because without the signal, there could be more than one controller that could always drive a device crazy easily. But if we do provide Gaia that signal, hmmm... what's the benefit? it kinda conflicts our original goal -- let gecko recover it and gaia doesn't need to know. 

Therefore, considering possible solutions and situations, we think having Sytem/Settings App take care of user preference benefits phone state monitoring and management, and make the management less error-prone, especially with the fact that System/Settings App isn't a normal app, they are designed as a phone control center.

[1] bug 928851, 861794, 795255, 859318, 917097, 904995 ...

Comment 9

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

After the offline discussion, the real problem here is how CAF could have a separate test environment for API unit-tests. (Anshul, please correct me if I am wrong) 

The requirement CAF wants is to test API behaviour, such as mobileConnection.setRadioEnabled, without System App interference. Disabling the App as below might help. Are the 2 options working for you, Anshul?

a) Disable "radio" screen: remove https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L309, or
b) Disable System app: have a blank gaia/apps/system/index.html

Comment 10

[Anshul]

Hsin-Yi, the setRadioEnabled public API was introduced by bug 856553. If this API wasn't mean to be public, why was this API added to nsIMozMobileConnection interface which is accessible to other apps in the system? If no one is suppose to use this API then this API shouldn't be accessible.

Did you get a chance to look at the fix for bug 1038496? As I said, the fix isn't consistent and honestly looks a little hacky. That is in part because Gaia shouldn't be the one making sure that the state of the system is the same as setting; that is the job of the consumers of these settings. 

Can you please cite examples of other settings where Gaia is enforcing such behavior? In fact for settings such as CLIR, call waiting, call forwarding, etc. Gaia sends the current settings value on boot up to telephony to be sent to the network but doesn't enforce that these settings stay the same. For example, these settings could also be changed using MMI, in which case the settings changed message is sent to Gaia and Gaia simply overrides the settings database with the new value instead of reverting the setting back to what it was before (unlike the current behavior of radio setting).

Comment 11

[Anshul]

Hsin-Yi, did you get a chance to look at this bug; this is still an issue for us?

Comment 12

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

Hi Anshul,
Sorry for being late. I believe your test needs revision for 2.2, otherwise you will still see this issue.

(In reply to Anshul from comment #10)
> Hsin-Yi, the setRadioEnabled public API was introduced by bug 856553. If
> this API wasn't mean to be public, why was this API added to
> nsIMozMobileConnection interface which is accessible to other apps in the
> system? If no one is suppose to use this API then this API shouldn't be
> accessible.
> 

Not sure if we have the same definition of "public" API... setRadioEnabled is an API for certified apps only, which isn't really public I'd say. However, I have to admit that we didn't consider the case carefully enough that there could be more than one certified app using this API at the same time. In fxos we lack of a mechanism to have a fine-grained permission control. Ideally, not all certified apps can access this API.

Comment 13

[Anshul]

Hsin-Yi, thanks for the explanation. I request you to please take a close look at the patch for bug 1038496 as it seems like a hack rather than a clean fix as per my comment #10.

Since our tests don't have access to window object, do you have a suggestion on how do we go about it. In the meanwhile we will keep bug 1038496 backed out in our internal tree.

Comment 14

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

Hello Anshul,
I discussed the design with Arthur, Jessica and Aknow again. Here's our proposal that I think meets your requirement. Please take a look.

1) Gecko should take care of the crash case, and does it's best to have radio state consistent with a user request via setRadioEnabled() during the life time.
2) Whenever Gaia calls setRadioEnabled() API, Gaia should carefully make sure that the user preference gets updated correspondingly. 
3) Gaia is the one to maintain user preference cross boot. Right on boot-up, Gaia should request setRadioEnabled() API based on the previous user preference.

Once the proposal is implemented, bug 1038496 will be reverted. Does that sound good?

Comment 15

[Anshul]

Hey Hsin-Yi, this sounds perfect. Thanks for considering the request.

Comment 16

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

Move component to RIL per comment 14.

Comment 17

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: partner's testing blocker.

Comment 18

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

There's an on-going discussion about the exact behaviour: https://groups.google.com/d/msg/mozilla.dev.b2g/KOaarZLKyZg/73N9WTghxNgJ

I'd like to follow the conclusion.

Comment 19

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> There's an on-going discussion about the exact behaviour:
> https://groups.google.com/d/msg/mozilla.dev.b2g/KOaarZLKyZg/73N9WTghxNgJ
> 
> I'd like to follow the conclusion.

Sorry, let me re-assign myself for now. I'll see if I am available when the conclusion on web-api is achieved.

Comment 20

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Deferring to next sprint per comment 19.

Comment 21

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

Okay... confirmed with Arthur that let's simply do comment 14 in this bug. We will have another follow-up bug to handle that webapi discussion as that requires more work on both gecko and gaia. Better do it step by step!

Comment 22

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

Hi Ben,
Could you help this bug? Thank you.

Comment 23

[Ben Hsu [:HoPang]]

Sure, I'll look it up.

Comment 24

[Ben Hsu [:HoPang]]

Attachment: Part 1: Construct a radio state FSM(mobileConnectionService.js)

Comment 25

[Ben Hsu [:HoPang]]

Attachment: Part 2: Remove some radio state checks(RadioInterfaceLayer.js)

In this patch, some of the radio state checks are moved to |mobileConnectionService.js|, so I remove them from here.

Comment 26

[Ben Hsu [:HoPang]]

Attachment: Part 3: Some minor modifications(ril_worker.js)

Comment 27

[Ben Hsu [:HoPang]]

Attachment: Part 4: Update the related testcase

Comment 28

[Ben Hsu [:HoPang]]

Attachment: [hardware/ril] pull request 43

Comment 29

[Ben Hsu [:HoPang]]

Attachment: [external/qemu] pull request 128

Comment 30

[Ben Hsu [:HoPang]]

Hi Edgar, 

I have to extend the functionality of the emulator for this bug, do you mind reviewing those pull requests?

Comment 31

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8560396 [details] [diff] [review]
Part 4: Update the related testcase

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

::: dom/mobileconnection/tests/marionette/head.js
@@ +716,5 @@
>   *        start*TestCommon() or 0 if not indicated.
>   *
>   * @return A deferred promise.
>   */
>  function setRadioEnabledAndWait(aEnabled, aServiceId) {

Where is this function?  Did you forget to upload a patch?

Comment 32

[Ben Hsu [:HoPang]]

(In reply to Szu-Yu Chen [:aknow] from comment #31)
> Where is this function?  Did you forget to upload a patch?

The function |setRadioEnabledAndWait()| is defined at |head.js| in the place you left this comment and invoked by other testcases, e.g., |test_mobile_set_radio.js|.

Comment 33

[Ben Hsu [:HoPang]]

Attachment: Part 4: Update the related testcase (V2)

Thanks for the advice came from Edgar, I revise this part by utilizing more components defined in |head.js|.

Comment 34

[Edgar Chen [:edgar]]

Comment on attachment 8560398 [details] [review]
[hardware/ril] pull request 43

As per sync, hardware/ril now moves to new branch model [1].

There are two thing needs your help,
1. Rebase the pull request #43 with latest master (For emulator-kk).
2. Provide a separated pull request for b2g-ril_v7 branch (For emulator-ics).

Please asking review again once the pull requests are ready, Thank you.

[1] Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1128837#c1

Comment 35

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8560393 [details] [diff] [review]
Part 1: Construct a radio state FSM(mobileConnectionService.js)

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

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +301,5 @@
>  
> +  /**
> +   * The two radio states below stand for the state expected by user and the
> +   * state of the hardware, respectively. |radioState| will be updated based on
> +   * the their values.

s/the //

@@ +607,5 @@
> +
> +    let newRadioState;
> +    let ri = this._radioInterface;
> +    switch (this._expectedRadioState) {
> +      case true:

It looks pretty weird to have
- case true
- case false
- default

Please change it.

@@ +1011,3 @@
>  
> +    // Before sending a equest to |ril_worker.js|, we should check radioState.
> +    switch (this.radioState) {

What is the purpose of this.radioState?
Does it differ from _expectedRadioState or _hardwareRadioState?

@@ +1029,5 @@
> +        aCallback.notifyError("InvalidStateError");
> +        return;
> +    }
> +
> +    this._expectedRadioState = aEnabled;

Set it to nsIMobileConnection.MOBILE_RADIO_STATE_*

@@ +1210,5 @@
>      if (DEBUG) {
>        debug("notifyRadioStateChanged for " + aClientId + ": " + aRadioState);
>      }
>  
> +    let mobileConnection = this.getItemByServiceId(aClientId);

s/mobileConnection/provider

Just follow the coding style of this file.

@@ +1211,5 @@
>        debug("notifyRadioStateChanged for " + aClientId + ": " + aRadioState);
>      }
>  
> +    let mobileConnection = this.getItemByServiceId(aClientId);
> +    switch (aRadioState) {

Try to move this 'switch' block into updateRadioState() because it's the work related to a single mobileConnection.

@@ +1213,5 @@
>  
> +    let mobileConnection = this.getItemByServiceId(aClientId);
> +    switch (aRadioState) {
> +      case RIL.GECKO_RADIOSTATE_UNKNOWN:
> +        mobileConnection._hardwareRadioState = null;

I don't like this kind of usage -- a variable which could be true/false and null. It's error-prone. Someday you may write a code like "if (_hardwareRadioState)" and lead to an error.

Let's just set it to RIL.GECKO_RADUISTATE_*

Comment 36

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Ben Hsu [:benhsu] from comment #32)
> (In reply to Szu-Yu Chen [:aknow] from comment #31)
> > Where is this function?  Did you forget to upload a patch?
> 
> The function |setRadioEnabledAndWait()| is defined at |head.js| in the place
> you left this comment and invoked by other testcases, e.g.,
> |test_mobile_set_radio.js|.

Oh. Sorry. It's mobileconnection. In the beginning, I tried to look up the function in telephony's head.js...

Comment 37

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8560394 [details] [diff] [review]
Part 2: Remove some radio state checks(RadioInterfaceLayer.js)

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +531,5 @@
>        radioInterface.workerMessenger.send("setRadioEnabled", message.data,
>                                            (function(response) {
>          if (response.errorMsg) {
> +          // If request fails, set current radio state to unknown, since we will
> +          // handle it in |mobileConnectionService|.

Why does it become UNKNOWN instead of keeping previous radioState?

If you send the UNKNOWN back, from the code, it looks like mobileConnection will trigger another request try to set the state to _expectedRadioState.  The request might fail again and result in infinite loop.

Comment 38

[Ben Hsu [:HoPang]]

Comment on attachment 8560393 [details] [diff] [review]
Part 1: Construct a radio state FSM(mobileConnectionService.js)

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

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +607,5 @@
> +
> +    let newRadioState;
> +    let ri = this._radioInterface;
> +    switch (this._expectedRadioState) {
> +      case true:

OK, I'll find new values for those states.

@@ +1011,3 @@
>  
> +    // Before sending a equest to |ril_worker.js|, we should check radioState.
> +    switch (this.radioState) {

|this.radioState| is the only value that can be obtained by DOM, while |_expectedRadioState| and |_hardwareRadioState| are used as internal states inside mobileConnectionService. |_expectedRadioState| is set by users, while |_hardwareRadioState| shows the status of underlying hardware, and their states can be only one of three values: "on", "off" and "unknown". Based on the values of the two internal states, we can finally derive |this.radioState| to represent the current status of the radio.

@@ +1029,5 @@
> +        aCallback.notifyError("InvalidStateError");
> +        return;
> +    }
> +
> +    this._expectedRadioState = aEnabled;

Since the values of |_expectedRadioState| and |_hardwareRadioState| can be only "on", "off" or "unknown", I think it's a little misleading to set them to nsIMobileConnection.MOBILE_RADIO_STATE_* which includes "enabling" and "disabling".

@@ +1213,5 @@
>  
> +    let mobileConnection = this.getItemByServiceId(aClientId);
> +    switch (aRadioState) {
> +      case RIL.GECKO_RADIOSTATE_UNKNOWN:
> +        mobileConnection._hardwareRadioState = null;

I think the new state values mentioned earlier will solve this issue.

Comment 39

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8561906 [details] [diff] [review]
Part 4: Update the related testcase (V2)

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

::: dom/mobileconnection/tests/marionette/head.js
@@ +727,3 @@
>  
> +  let promise_1 = () => {
> +    let actualRadioState = [];

s/actualRadioState/actualRadioStates

@@ +727,4 @@
>  
> +  let promise_1 = () => {
> +    let actualRadioState = [];
> +    let wantedRadioState = aEnabled ? ["enabling", "enabled"] :

s/wantedRadioState/wantedRadioStates

@@ +744,5 @@
> +  }
> +
> +  let promise_2 = setRadioEnabled.bind(this, aEnabled, aServiceId);
> +
> +  return Promise.all([promise_1(), promise_2()]);

How about just do something like

let promise_1 = waitForManagerEvent("radiostatechange", aServiceId, () => mobileConn.radioState === "enabling")
.then(() => waitForManagerEvent("radiostatechange", aServiceId, () => mobileConn.radioState === "enabled"));

let promise_2 = setRadioEnabled(aEnabled, aServiceId);

return Promise.all([promise_1, promise_2])

::: dom/mobileconnection/tests/marionette/test_mobile_set_radio.js
@@ +22,5 @@
> +        let wantedStr = JSON.stringify(wantedRadioState);
> +        is(actualStr, wantedStr,
> +          "Unexpected Radio State: Actual" + actualStr + " Wanted" + wantedStr);
> +      });
> +  }

Same here.  Have a similar modification as we done in head.js

@@ +49,5 @@
>      .then(() => setRadioEnabledAndWait(true))
>  
> +    // Test auto-restore radio state
> +    .then(() => log("Test auto-restore radio state"))
> +    .then(() => testAutoRestoreRadioState(false))

Missing serviceId

@@ +50,5 @@
>  
> +    // Test auto-restore radio state
> +    .then(() => log("Test auto-restore radio state"))
> +    .then(() => testAutoRestoreRadioState(false))
> +    .then(() => testAutoRestoreRadioState(true))

Missing serviceId

Comment 40

[Edgar Chen [:edgar]]

Comment on attachment 8561906 [details] [diff] [review]
Part 4: Update the related testcase (V2)

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

Sorry for jumping in, just sharing my thoughts.

::: dom/mobileconnection/tests/marionette/head.js
@@ +727,2 @@
>  
> +  let promise_1 = () => {

We usually use camel-case for variable name.

@@ +734,5 @@
> +      return mobileConn.radioState ===  (aEnabled ? "enabled" : "disabled");
> +    }
> +
> +    return waitForManagerEvent("radiostatechange", aServiceId, matchFunc)
> +      .then(() => {

Another idea :)

let expectedSequence = aEnabled ? ["enabling", "enabled"] :
                                  ["disabling", "disabled"];
waitForManagerEvent("radiostatechange", aServiceId, function() {
  let mobileConn = getMozMobileConnectionByServiceId(aServiceId);
  let expectedState = expectedSequence.shift();
  is(mobileConn.radioState, expectedState, "check radio state");
  return expectedSequence.length === 0;
}

::: dom/mobileconnection/tests/marionette/test_mobile_set_radio.js
@@ +30,5 @@
> +    return runEmulatorCmdSafe(command);
> +  };
> +
> +  return setRadioEnabledAndWait(aEnabled, aServiceId)
> +    .then(() => Promise.all([promise_1(), promise_2()]));

Nit: I prefer moving the declaration of promise1 and promise2 into `then`, this probably makes code clearer.

e.g.

return setRadioEnabledAndWait(aEnabled, aServiceId)
  .then(() => {
    let promise1 = ...;
    let promise2 = ...;

    return Promise.all([promise1, promise2]);
  });

Comment 41

[Ben Hsu [:HoPang]]

Comment on attachment 8561906 [details] [diff] [review]
Part 4: Update the related testcase (V2)

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

::: dom/mobileconnection/tests/marionette/head.js
@@ +734,5 @@
> +      return mobileConn.radioState ===  (aEnabled ? "enabled" : "disabled");
> +    }
> +
> +    return waitForManagerEvent("radiostatechange", aServiceId, matchFunc)
> +      .then(() => {

Wow, it's a clever way to compare expected state sequence and actual state sequence, but I prefer my implementation here, since it can generate a better looking log on marionette :P

@@ +744,5 @@
> +  }
> +
> +  let promise_2 = setRadioEnabled.bind(this, aEnabled, aServiceId);
> +
> +  return Promise.all([promise_1(), promise_2()]);

Here, I can definitely create the two promises in this way, but I will try to make the structure of this part the same as the one in |test_mobile_set_radio.js|.

::: dom/mobileconnection/tests/marionette/test_mobile_set_radio.js
@@ +22,5 @@
> +        let wantedStr = JSON.stringify(wantedRadioState);
> +        is(actualStr, wantedStr,
> +          "Unexpected Radio State: Actual" + actualStr + " Wanted" + wantedStr);
> +      });
> +  }

Here, I cannot create the two promises as your suggestion for |head.js|, since the two promises should be created after we manually set the radio state in the end of the function |testAutoRestoreRadioState(...)|. Otherwise, the listeners added in |promise_1| will catch the events triggered by the previous setting radio state.

Now, I am trying to address Edgar's advice. On success, maybe we can avoid creating the two promises with functions.

@@ +30,5 @@
> +    return runEmulatorCmdSafe(command);
> +  };
> +
> +  return setRadioEnabledAndWait(aEnabled, aServiceId)
> +    .then(() => Promise.all([promise_1(), promise_2()]));

I'll try it out, but I am afraid of writing this way will cause too much indent.

@@ +49,5 @@
>      .then(() => setRadioEnabledAndWait(true))
>  
> +    // Test auto-restore radio state
> +    .then(() => log("Test auto-restore radio state"))
> +    .then(() => testAutoRestoreRadioState(false))

I skip the |serviceId| here on purpose, since all the other |serviceId|s in this testcase are skipped. Do you prefer to add every |serviceId| back?

Comment 42

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Ben Hsu [:benhsu] from comment #41)
> Comment on attachment 8561906 [details] [diff] [review]
> Part 4: Update the related testcase (V2)
> 
> Review of attachment 8561906 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobileconnection/tests/marionette/head.js
> @@ +734,5 @@
> > +      return mobileConn.radioState ===  (aEnabled ? "enabled" : "disabled");
> > +    }
> > +
> > +    return waitForManagerEvent("radiostatechange", aServiceId, matchFunc)
> > +      .then(() => {
> 
> Wow, it's a clever way to compare expected state sequence and actual state
> sequence, but I prefer my implementation here, since it can generate a
> better looking log on marionette :P
> 
> @@ +744,5 @@
> > +  }
> > +
> > +  let promise_2 = setRadioEnabled.bind(this, aEnabled, aServiceId);
> > +
> > +  return Promise.all([promise_1(), promise_2()]);
> 
> Here, I can definitely create the two promises in this way, but I will try
> to make the structure of this part the same as the one in
> |test_mobile_set_radio.js|.
> 
> ::: dom/mobileconnection/tests/marionette/test_mobile_set_radio.js
> @@ +22,5 @@
> > +        let wantedStr = JSON.stringify(wantedRadioState);
> > +        is(actualStr, wantedStr,
> > +          "Unexpected Radio State: Actual" + actualStr + " Wanted" + wantedStr);
> > +      });
> > +  }
> 
> Here, I cannot create the two promises as your suggestion for |head.js|,
> since the two promises should be created after we manually set the radio
> state in the end of the function |testAutoRestoreRadioState(...)|.
> Otherwise, the listeners added in |promise_1| will catch the events
> triggered by the previous setting radio state.
> 
> Now, I am trying to address Edgar's advice. On success, maybe we can avoid
> creating the two promises with functions.

Just create two promises in the callback of first promise.

> 
> @@ +30,5 @@
> > +    return runEmulatorCmdSafe(command);
> > +  };
> > +
> > +  return setRadioEnabledAndWait(aEnabled, aServiceId)
> > +    .then(() => Promise.all([promise_1(), promise_2()]));
> 
> I'll try it out, but I am afraid of writing this way will cause too much
> indent.
> 
> @@ +49,5 @@
> >      .then(() => setRadioEnabledAndWait(true))
> >  
> > +    // Test auto-restore radio state
> > +    .then(() => log("Test auto-restore radio state"))
> > +    .then(() => testAutoRestoreRadioState(false))
> 
> I skip the |serviceId| here on purpose, since all the other |serviceId|s in
> this testcase are skipped. Do you prefer to add every |serviceId| back?

If it's the purpose, you can provide a default value.

function testAutoRestoreRadioState(aEnabled, aServiceId = 0)

Comment 43

[Ben Hsu [:HoPang]]

Comment on attachment 8560394 [details] [diff] [review]
Part 2: Remove some radio state checks(RadioInterfaceLayer.js)

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +531,5 @@
>        radioInterface.workerMessenger.send("setRadioEnabled", message.data,
>                                            (function(response) {
>          if (response.errorMsg) {
> +          // If request fails, set current radio state to unknown, since we will
> +          // handle it in |mobileConnectionService|.

You're right. In fact, no matter we set the radio to "its previous state" or "unknown state", there will be an infinite loop, since |mobileConnectionService| will keep trying to set the radio state to  |_expectedRadioState| till it successes.

Besides, I think "unknown" is better than "its previous state", because failing to set the radio state somehow indicates that we lose the control of the underlying hardware. If we set the radio to its previous state, we cannot notify upper layers about this condition, but we should block some operations if we cannot guarantee the radio is truly "enabled".

I think we should set the radio state to "unknown" here, and I'll try to fix the infinite loop problem at |mobileConnectionService|.

Comment 44

[Ben Hsu [:HoPang]]

Attachment: Part 1: Construct a radio state FSM(mobileConnectionService.js) (V2)

To prevent infinite loop, when failing to enable/disable the radio state no matter on demand or automatically, we just set the radio state to "UNKNOWN" and reject the request if the enable/disable is triggered by upper layers.

Comment 45

[Ben Hsu [:HoPang]]

Comment on attachment 8560394 [details] [diff] [review]
Part 2: Remove some radio state checks(RadioInterfaceLayer.js)

Hi Aknow,

Do you mind reviewing this patch again? The prevention of inifinite loops is implemented in |mobileConnectionService.js| now, so we just notify the radio state as |UNKNOWN| here.

Comment 46

[Ben Hsu [:HoPang]]

Attachment: Part 3: Some minor modifications(ril_worker.js)

After trying out all the advice above, I adopt the one from Edgar, since we only have to add the listener only one time and the debug message is also quite readable.

Comment 47

[Ben Hsu [:HoPang]]

Comment on attachment 8563962 [details] [diff] [review]
Part 3: Some minor modifications(ril_worker.js)

Sorry Aknow, 

I obsoleted the wrong patch, can you review this patch again?

Comment 48

[Ben Hsu [:HoPang]]

Attachment: Part 4: Update the related testcase (V3)

After trying out all the advice above, I adopt the one from Edgar, since we only have to add the listener only one time and the debug message is also quite readable.

Comment 49

[Ben Hsu [:HoPang]]

Attachment: Part 5: Deprecate GECKO_RADIOSTATE_ENABLING and GECKO_RADIOSTATE_DISABLING (ril_consts.js)

Per offline discussion with Aknow, we found out those two state are no longer being used after this issue, so we can remove them.

Comment 50

[Ben Hsu [:HoPang]]

Attachment: [hardware/ril] pull request 46 (ril V7)

Hi Edgar,

I've addressed the advice you gave, do you mind reviewing those pull requests?

Comment 51

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8563963 [details] [diff] [review]
Part 4: Update the related testcase (V3)

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

r=me with comments addressed

::: dom/mobileconnection/tests/marionette/head.js
@@ +722,5 @@
> +  let mobileConn = getMozMobileConnectionByServiceId(aServiceId);
> +
> +  if (mobileConn.radioState === (aEnabled ? "enabled" : "disabled")) {
> +    return Promise.resolve();
> +  }

I think we can move this part to |setRadioEnabled|.

@@ +732,5 @@
>      let mobileConn = getMozMobileConnectionByServiceId(aServiceId);
> +    let expectedRadioState = expectedSequence.shift();
> +    is(mobileConn.radioState, expectedRadioState, "Check radio state");
> +    return expectedSequence.length === 0;
> +  });

Have another utility function |waitRadioStateSequence(aExpectedSequence, aServiceId)|.  In the function do the exactly same thing as p1.

@@ +741,1 @@
>  }

function setRadioEnabledAndWait(aEnabled, aServiceId) {
  let expectedSequence = aEnabled ? ["enabling", "enabled"] : ["disabling", "disabled"];
  let p1 = waitRadioStateSequence(expectedSequence, aService);
  let p2 = setRadioEnabled(aEnabled, aServiceId);
  return Promise.all([p1, p2]);
}

::: dom/mobileconnection/tests/marionette/test_mobile_set_radio.js
@@ +14,5 @@
> +        let mobileConn = getMozMobileConnectionByServiceId(aServiceId);
> +        let expectedRadioState = expectedSequence.shift();
> +        is(mobileConn.radioState, expectedRadioState, "Check radio state");
> +        return expectedSequence.length === 0;
> +      });

let p1 = waitRadioStateSequence(expectedSequence, aServiceId);

Comment 52

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8563954 [details] [diff] [review]
Part 1: Construct a radio state FSM(mobileConnectionService.js) (V2)

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

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +587,5 @@
>        }
>      }
>    },
>  
> +  updateRadioState: function(aExpectedRS, aHardwareRS, aCallback) {

We don't have to provide other two variable.  Just modify this._expected*, this._hardware* at the proper place.

I'd like to rename it to something like ensureRadio().
The function's purpose is to ensure that hardwareRadioState matches expectedRadioState.

ensureRadioState: function(aCallback = null) {

@@ +592,5 @@
> +
> +    if ((aExpectedRS !== null && aHardwareRS !== null) ||
> +        (aExpectedRS === null && aHardwareRS === null)) {
> +      return;
> +    }

Then, we can remove this 'if' block.

@@ +597,5 @@
> +
> +    this._expectedRadioState = aExpectedRS !== null ? aExpectedRS :
> +                                                      this._expectedRadioState;
> +    this._hardwareRadioState = aHardwareRS !== null ? aHardwareRS :
> +                                                      this._hardwareRadioState;

Also remove this part.

@@ +605,5 @@
> +        return false;
> +      }
> +
> +      if (aResponse.errorMsg) {
> +        aCallback.notifyError(aResponse.errorMsg);

What happened to this.radioState if setRadio fail?  Will it be trapped in ENABLING or DISABLING?

@@ +619,5 @@
> +    switch (this._expectedRadioState) {
> +      case RIL.GECKO_RADIOSTATE_ENABLED:
> +        if (this._hardwareRadioState === RIL.GECKO_RADIOSTATE_ENABLED) {
> +          newRadioState = Ci.nsIMobileConnection.MOBILE_RADIO_STATE_ENABLED;
> +        } else if (aHardwareRS === RIL.GECKO_RADIOSTATE_UNKNOWN) {

When users call the API to set radio before we get the first update of hardwareRadioState from ril?
At this moment, this condition is also hit but the logic seems not correct.

@@ +622,5 @@
> +          newRadioState = Ci.nsIMobileConnection.MOBILE_RADIO_STATE_ENABLED;
> +        } else if (aHardwareRS === RIL.GECKO_RADIOSTATE_UNKNOWN) {
> +          // TODO: If this update is triggered by underlying layers and the
> +          // state is UNKNOWN, we should find a better way of handling this
> +          // update than just setting the redio state to UNKNOWN.

I don't like the duplicated comments.  Any way to avoid that?

@@ +658,5 @@
> +          newRadioState = Ci.nsIMobileConnection.MOBILE_RADIO_STATE_UNKNOWN;
> +        }
> +    }
> +
> +    if (DEBUG) debug("Current Radio State is '" + newRadioState + "'");

Use this._debug

@@ +1022,5 @@
>    },
>  
>    setRadioEnabled: function(aEnabled, aCallback) {
> +    if (DEBUG) {
> +      debug("setRadioEnabled for " + this._clientId + ": " + aEnabled);

You should use |this._debug()|.  It's the debug function for mobileConnectionProvider.  By using it, clientId will be print out automatically.

@@ +1046,5 @@
> +        aCallback.notifyError("InvalidStateError");
> +        return;
> +    }
> +
> +    let expectedRS = aEnabled ? RIL.GECKO_RADIOSTATE_ENABLED :

this._expectedRadioState = aEnabled ? ...

@@ +1048,5 @@
> +    }
> +
> +    let expectedRS = aEnabled ? RIL.GECKO_RADIOSTATE_ENABLED :
> +                                RIL.GECKO_RADIOSTATE_DISABLED;
> +    this.updateRadioState(expectedRS, null, aCallback);

this.updateRadioState(aCallback)

@@ +1229,5 @@
>        debug("notifyRadioStateChanged for " + aClientId + ": " + aRadioState);
>      }
>  
> +    this.getItemByServiceId(aClientId)
> +        .updateRadioState(null, aRadioState, null);

this.getItemByServiceId(aClientId).updateRadioState(aRadioState);

in provider, have a function

updateRadioState: function(aRadioState) {
  this._hardwareRadioState = aRadioState;
  this._ensureRadio();
}

Comment 53

[Ben Hsu [:HoPang]]

Comment on attachment 8563954 [details] [diff] [review]
Part 1: Construct a radio state FSM(mobileConnectionService.js) (V2)

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

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +587,5 @@
>        }
>      }
>    },
>  
> +  updateRadioState: function(aExpectedRS, aHardwareRS, aCallback) {

In my design, I want to make |updateRadioState(...)| is the only place where we can update both _expectedRadioState and _hardwareRadioState. By doing this, we can keep the entire implementation of the FSM in a function instead of making it scatter around the file, and  make |updateRadioState(...)| the only one can fire |radioStateChanged| event.

@@ +605,5 @@
> +        return false;
> +      }
> +
> +      if (aResponse.errorMsg) {
> +        aCallback.notifyError(aResponse.errorMsg);

It's won't be trapped in ENABLING or DISABLING, since on request failure, |RadioInterfaceLayer.js| will set the radio state to unknown.

@@ +619,5 @@
> +    switch (this._expectedRadioState) {
> +      case RIL.GECKO_RADIOSTATE_ENABLED:
> +        if (this._hardwareRadioState === RIL.GECKO_RADIOSTATE_ENABLED) {
> +          newRadioState = Ci.nsIMobileConnection.MOBILE_RADIO_STATE_ENABLED;
> +        } else if (aHardwareRS === RIL.GECKO_RADIOSTATE_UNKNOWN) {

In fact, the condition check is |aHardwareRS === RIL.GECKO_RADIOSTATE_UNKNOWN|, which indicate that the update is triggered by underlying layers, since the value of aHardwareRS is always |null| for updates triggered by upper layers(e.g., users). As a result, the case described in your comment will hit the |else block|, which will pass the request to underlying layers.

Comment 54

[Edgar Chen [:edgar]]

Comment on attachment 8560400 [details] [review]
[external/qemu] pull request 128

Thank you.

Comment 55

[Ben Hsu [:HoPang]]

Attachment: Part 1: Construct a radio state FSM(mobileConnectionService.js) (V3)

Comment 56

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8564988 [details] [diff] [review]
Part 1: Construct a radio state FSM(mobileConnectionService.js) (V3)

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

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +627,5 @@
> +            newRadioState = Ci.nsIMobileConnection.MOBILE_RADIO_STATE_UNKNOWN;
> +        }
> +    }
> +
> +    if (aMessage.msgType === "ExpectedRadioState" &&

Should this be "HardwareRadioState"?

Comment 57

[Ben Hsu [:HoPang]]

Attachment: Part 1: Construct a radio state FSM(mobileConnectionService.js) (V4)

Comment 58

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8565240 [details] [diff] [review]
Part 1: Construct a radio state FSM(mobileConnectionService.js) (V4)

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

r=me with comments address or question responded

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +600,5 @@
> +        if (DEBUG) this._debug("updateRadioState: Invalid message type");
> +        return;
> +    }
> +
> +    let newRS;

Please don't use abbreviation.  Use |newRadioState| or |newState|.
|newRS| is not readable and pronounceable.

@@ +604,5 @@
> +    let newRS;
> +    switch (this._expectedRadioState) {
> +      case RIL.GECKO_RADIOSTATE_ENABLED:
> +        newRS = this._hardwareRadioState === this._expectedRadioState ?
> +          Ci.nsIMobileConnection.MOBILE_RADIO_STATE_ENABLED:

Add a space before ':'

@@ +610,5 @@
> +        break;
> +
> +      case RIL.GECKO_RADIOSTATE_DISABLED:
> +        newRS = this._hardwareRadioState === this._expectedRadioState ?
> +          Ci.nsIMobileConnection.MOBILE_RADIO_STATE_DISABLED:

Add a space before ':'

@@ +627,5 @@
> +            newRS = Ci.nsIMobileConnection.MOBILE_RADIO_STATE_UNKNOWN;
> +        }
> +    }
> +
> +    if (aMessage.msgType === "HardwareRadioState" &&

Separate the comment.  Write following before the 'if'
"""
This update is triggered by underlying layers and the state is UNKNOWN.
"""

@@ +631,5 @@
> +    if (aMessage.msgType === "HardwareRadioState" &&
> +        aMessage.msgData === RIL.GECKO_RADIOSTATE_UNKNOWN) {
> +      // TODO: If this update is triggered by underlying layers and the state is
> +      // UNKNOWN, we should find a better way of handling this update than just
> +      // setting the radio state to UNKNOWN.

// TODO: find a better way of ...

@@ +639,5 @@
> +    if (aMessage.msgType === "ExpectedRadioState" && aCallback) {
> +      if (aMessage.msgData === RIL.GECKO_RADIOSTATE_ENABLED &&
> +          newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_ENABLED ||
> +          aMessage.msgData === RIL.GECKO_RADIOSTATE_DISABLED &&
> +          newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_DISABLED) {

Does it equal to |this._expectedRadioState === this._hardwareRadioState| ?

@@ +643,5 @@
> +          newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_DISABLED) {
> +        // Early resolved
> +        aCallback.notifySuccess();
> +      }
> +    }

Could we move the entire 'if' block to the place just after first 'switch' ?

@@ +647,5 @@
> +    }
> +
> +    if (newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_ENABLING ||
> +        newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_DISABLING) {
> +      let radioInterface = this._radioInterface;

Let's remove it.  The symbol is only referred once.  It's not worth creating an alias.

@@ +649,5 @@
> +    if (newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_ENABLING ||
> +        newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_DISABLING) {
> +      let radioInterface = this._radioInterface;
> +      let action = this._expectedRadioState === RIL.GECKO_RADIOSTATE_ENABLED ?
> +                   true : false;

let action = this._expectedRadioState === RIL.GECKO_RADIOSTATE_ENABLED;

@@ +650,5 @@
> +        newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_DISABLING) {
> +      let radioInterface = this._radioInterface;
> +      let action = this._expectedRadioState === RIL.GECKO_RADIOSTATE_ENABLED ?
> +                   true : false;
> +      radioInterface.sendWorkerMessage("setRadioEnabled", {enabled: action},

this._radioInterface

@@ +660,5 @@
> +          aCallback.notifyError(aResponse.errorMsg);
> +          return false;
> +        }
> +        aCallback.notifySuccess();
> +        return false;

Well... I think maybe we could file a bug to remove all this kind of |return false| in mobileConnectionService.js.

Comment 59

[Ben Hsu [:HoPang]]

Comment on attachment 8565240 [details] [diff] [review]
Part 1: Construct a radio state FSM(mobileConnectionService.js) (V4)

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

::: dom/mobileconnection/gonk/MobileConnectionService.js
@@ +639,5 @@
> +    if (aMessage.msgType === "ExpectedRadioState" && aCallback) {
> +      if (aMessage.msgData === RIL.GECKO_RADIOSTATE_ENABLED &&
> +          newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_ENABLED ||
> +          aMessage.msgData === RIL.GECKO_RADIOSTATE_DISABLED &&
> +          newRS === Ci.nsIMobileConnection.MOBILE_RADIO_STATE_DISABLED) {

Yes, they are equivalent, and thanks for this advice. However, I think I should explain a liitle more here. Since the value of msgData of |type ExpectedRadioState| can only be RIL.GECKO_RADIOSTATE_ENABLED or RIL.GECKO_RADIOSTATE_DISABLED, we don't have to worry about RIL.GECKO_RADIOSTATE_UNKNOWN. Therefore, when |this._expectedRadioState === this._hardwareRadioState| returns true, radio state must be |enabled| or |disabled| correspondingly. Therefore, the two expresssions are equivelent here.

Comment 60

[Ben Hsu [:HoPang]]

Attachment: Part 1: Construct a radio state FSM(mobileConnectionService.js) (V5)

Comment 61

[Ben Hsu [:HoPang]]

Attachment: Part 5: Deprecate GECKO_RADIOSTATE_ENABLING and GECKO_RADIOSTATE_DISABLING (ril_consts.js)

Just update the commit message.

Comment 62

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22d4f572a673

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8560398 [details] [review]
[hardware/ril] pull request 43

In bugs like these, it's very confusing when obsolete attachments aren't marked as such. Please be more conscientious about that in the future.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/038a9dea3783
https://hg.mozilla.org/integration/b2g-inbound/rev/15831b5e5f46
https://hg.mozilla.org/integration/b2g-inbound/rev/88984657509c
https://hg.mozilla.org/integration/b2g-inbound/rev/2acb4a408619
https://hg.mozilla.org/integration/b2g-inbound/rev/dad2219de6a3

Master: https://github.com/mozilla-b2g/platform_external_qemu/commit/97c3d9b8b87774ca7a08c89145e95b55652459ef

b2g-ril_v7: https://github.com/mozilla-b2g/platform_hardware_ril/commit/93f9ba577f68d772093987c2f1c0a4ae293e1802

Comment 65

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/038a9dea3783
https://hg.mozilla.org/mozilla-central/rev/15831b5e5f46
https://hg.mozilla.org/mozilla-central/rev/88984657509c
https://hg.mozilla.org/mozilla-central/rev/2acb4a408619
https://hg.mozilla.org/mozilla-central/rev/dad2219de6a3

Comment 66

[Ben Hsu [:HoPang]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #63)
> Comment on attachment 8560398 [details] [review]
> [hardware/ril] pull request 43
> 
> In bugs like these, it's very confusing when obsolete attachments aren't
> marked as such. Please be more conscientious about that in the future.

Hi Ryan

Sorry for my late response and your inconvenience. |[hardware/ril] pull request 43| shouldn't be obsoleted, it's for emulator-kk, while |[hardware/ril] pull request 46 (ril V7)| is for emulator, aka emulator-ics. Since the try server uses emulator-ics for automated testing, I tested |[hardware/ril] pull request 43| locally. If you need more information, please refer [1] for more information. Thanks for your help and all the kind reminders you gave.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1067629#c34

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8560398 [details] [review]
[hardware/ril] pull request 43

Sorry for the misunderstanding :(

Master: https://github.com/mozilla-b2g/platform_hardware_ril/commit/f810abaa47932079396ea3815df65fbdd83b1f94

Generally, people note which branch a PR is intended for when there are multiple ones for the same repo. Helps avoid this kind of confusion :)

Comment 68

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: ril_log.txt

I found the device is not able to camp on the network after the recovery of rid. Log is attached.

Comment 69

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

(In reply to Arthur Chen [:arthurcc] from comment #68)
> Created attachment 8569031 [details]
> ril_log.txt
> 
> I found the device is not able to camp on the network after the recovery of
> rid. Log is attached.

As the searching issue exists before this patch landing, and as the radio is recovered as expected, I file a new bug 1136585 for investigating the searching problem.

Comment 70

[Ryan VanderMeulen [:RyanVM]]

Does something need to land on v2.2 here still? If so, please be sure to get the appropriate approval requests up :)

Comment 71

[Ben Hsu [:HoPang]]

Thanks for your reminder. There are lots of things should be uplifted, and they are being tested in Try servers now :)

Comment 72

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 1: Construct a radio state FSM(mobileConnectionService.js)

Comment 73

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 2: Remove some radio state checks(RadioInterfaceLayer.js)

Comment 74

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 3: Some minor modifications(ril_worker.js)

Comment 75

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 4: Update the related testcase

Comment 76

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 5: Deprecate GECKO_RADIOSTATE_ENABLING and GECKO_RADIOSTATE_DISABLING (ril_consts.js)

Comment 77

[Ben Hsu [:HoPang]]

Attachment: (2.2) [hardware/ril] pull request 47

Comment 78

[Ben Hsu [:HoPang]]

Attachment: (2.2) [external/qemu] pull request 135

Comment 79

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 6: Enable the radio when making an emergency call

This patch makes |ril_worker.js| able to ask |MobileConnectionService| to enable the radio instead of enabling the radio by itself. Since if |ril_worker.js| enables the radio on its own, the radio will be later disabled by |MobileConnectionService|.

Comment 80

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8576619 [details] [diff] [review]
(2.2) Part 6: Enable the radio when making an emergency call

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1896,5 @@
>        case "otastatuschange":
>          gMobileConnectionService.notifyOtaStatusChanged(this.clientId, message.status);
>          break;
> +      case "radioNeeded":
> +        gMobileConnectionService.notifyRadioNeeded(this.clientId);

Let's use |nsIMobileConnection.setRadioEnabled| without creating a new interface.  Then you don't have to modify nsIGonkMobileConenctionSevice.idl and MobileConnectionService.js

::: dom/system/gonk/ril_worker.js
@@ +1644,5 @@
>            onerror: onerror
>          };
>  
> +        this.sendChromeMessage({
> +          rilMessageClientId: this.context.clientId,

|rilMessageClientId| is not needed. It will be added in |sendChromeMessage()|

Comment 81

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 6: Enable the radio when making an emergency call (V2)

Comment 82

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8577066 [details] [diff] [review]
(2.2) Part 6: Enable the radio when making an emergency call (V2)

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1896,5 @@
>        case "otastatuschange":
>          gMobileConnectionService.notifyOtaStatusChanged(this.clientId, message.status);
>          break;
> +      case "radioNeeded":
> +        gMobileConnectionService.getItemByServiceId(this.clientId).setRadioEnabled(true);

I think that |setRadioEnabled| needs a callback.

Comment 83

[Ben Hsu [:HoPang]]

Attachment: (2.2) Part 6: Enable the radio when making an emergency call (V3)

Comment 84

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8577112 [details] [diff] [review]
(2.2) Part 6: Enable the radio when making an emergency call (V3)

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

Looks great.  Thank you.

Comment 85

[Ben Hsu [:HoPang]]

Comment on attachment 8576389 [details] [diff] [review]
(2.2) Part 1: Construct a radio state FSM(mobileConnectionService.js)

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 86

[Ben Hsu [:HoPang]]

Comment on attachment 8576390 [details] [diff] [review]
(2.2) Part 2: Remove some radio state checks(RadioInterfaceLayer.js)

Comment on attachment 8576389 [details] [diff] [review] [diff] [review]
(2.2) Part 1: Construct a radio state FSM(mobileConnectionService.js)

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 87

[Ben Hsu [:HoPang]]

Comment on attachment 8576390 [details] [diff] [review]
(2.2) Part 2: Remove some radio state checks(RadioInterfaceLayer.js)

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 88

[Ben Hsu [:HoPang]]

Comment on attachment 8576391 [details] [diff] [review]
(2.2) Part 3: Some minor modifications(ril_worker.js)

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 89

[Ben Hsu [:HoPang]]

Comment on attachment 8576393 [details] [diff] [review]
(2.2) Part 4: Update the related testcase

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 90

[Ben Hsu [:HoPang]]

Comment on attachment 8576394 [details] [diff] [review]
(2.2) Part 5: Deprecate GECKO_RADIOSTATE_ENABLING and GECKO_RADIOSTATE_DISABLING (ril_consts.js)

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 91

[Ben Hsu [:HoPang]]

Comment on attachment 8576398 [details] [review]
(2.2) [hardware/ril] pull request 47

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 92

[Ben Hsu [:HoPang]]

Comment on attachment 8576399 [details] [review]
(2.2) [external/qemu] pull request 135

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 93

[Ben Hsu [:HoPang]]

Comment on attachment 8577112 [details] [diff] [review]
(2.2) Part 6: Enable the radio when making an emergency call (V3)

Bug caused by (feature/regressing bug #): NA
User impact if declined: No, but CAF testcases will fail without this patch.
Testing completed: device, and try server.
Risk to taking this patch (and alternatives if risky): 

1. Cannot camp on Network after rild is restarted, currently fixing in Bug 1136585 
2. Fail marionette WebAPI testcases, currently fixing in Bug 1139841

String or UUID changes made by this patch: NA

Comment 94

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ad1e45eefaf

Failed testcases are currently fixing in Bug 1139841

Comment 95

[bhavana bajaj [:bajaj]]

A roll-up patch with one approval request, would have been much helpful here, something to keep in mind for future.

Comment 96

[Ben Hsu [:HoPang]]

(In reply to bhavana bajaj [:bajaj] from comment #95)
> A roll-up patch with one approval request, would have been much helpful
> here, something to keep in mind for future.

Hi Bhavana, 

Sorry for your inconvenience, I'll remember that. Can you also uplift the patch of Bug 1129277? The patch is tested with the patches of this bug on both device and try server, and here is the try server result.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c653bab7ec9&exclusion_profile=false

Comment 97

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f27d6bedb1c6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f54b402b831c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/494d08cf6c4b
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ddcf583cee9c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/eadf45cab460
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9671cee40826

v2.2: https://github.com/mozilla-b2g/platform_hardware_ril/commit/72067ca0cb52bda5426913ad1eb89842929c0296

v2.2: https://github.com/mozilla-b2g/platform_external_qemu/commit/6bfddcebdf18b159340c6d65d00fb767b6b7121f