793073 - B2G RIL: Quickly tapping Airplane Mode causes phone keeping in "Emergency Calls Only"

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vincent Liu[:vliu]]

Method to reproduce this issue.

1. Powered on the device and checked the device in in network coverage.
2. Quickly tapped Airplane Mode less than 1 second.
3. When Airplane Mode kept in off mode, the phone lose network coverage and showed "Emergency Calls Only".

Comment 1

[Philipp von Weitershausen [:philikon]]

Sounds like we should do some debouncing here, similarly to what I outlined in bug 785982 comment 1. Shian-Yow, we talked about this in Sao Paulo, perhaps you could take this issue? Thanks!

Comment 2

[Shian-Yow Wu [:swu]]

Attachment: WIP V1

When the issue happens, Vincent found multiple RIL_REQUEST_RADIO_POWER events queued in rild which caused problems in rild layer.  Vincent is checking with Otoro device partner for possible cause.

Attached debouncing patch can avoid this problem.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 663348 [details] [diff] [review]
WIP V1

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

I'm not sure I like the additional states. This (indirectly) amends the MobileConnection WebAPI. Furthermore, the debounce doesn't really seem to be doing what I as a user would expect from it: eventual consistency with my setting.

I would suggest that while we change the radio power, we keep a flag around until we get the next radio state changed notification. If the `ril.radio.enabled` setting changes at any time in between that, we ignore it. In the radio state changed notification, we again compare the radio state to the `ril.radio.enabled` setting and make adjustments if they're out of sync.

Comment 4

[Vincent Liu[:vliu]]

Hi Philikon, Shian-Yow,

May I have chance to take this issue for trying your suggestion? Thanks.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to vliu from comment #4)
> May I have chance to take this issue for trying your suggestion? Thanks.

Sure, I don't care. Just please don't spend too much time on this, this isn't a blocker. We need to finish blockers first.

Comment 6

[Shian-Yow Wu [:swu]]

(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> I'm not sure I like the additional states. This (indirectly) amends the
> MobileConnection WebAPI. Furthermore, the debounce doesn't really seem to be
> doing what I as a user would expect from it: eventual consistency with my
> setting.
> 
> I would suggest that while we change the radio power, we keep a flag around
> until we get the next radio state changed notification. If the
> `ril.radio.enabled` setting changes at any time in between that, we ignore
> it. In the radio state changed notification, we again compare the radio
> state to the `ril.radio.enabled` setting and make adjustments if they're out
> of sync.

As my understanding, out of sync handling should be already handled in current code logic.

_ensureRadioState() will called inside handleRadioStateChange() where there's a state change:
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?rev=3abe394b6110#735

And the syncing is done inside _ensureRadioState() by checking 'this._radioEnabled' in:
https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?rev=3abe394b6110#762

I think what we need to enhance is adding new flags, if we don't like to amend the MobileConnection WebAPI.

Comment 7

[Shian-Yow Wu [:swu]]

(In reply to vliu from comment #4)
> Hi Philikon, Shian-Yow,
> 
> May I have chance to take this issue for trying your suggestion? Thanks.

Yes, feel free to take it.

Comment 8

[Vincent Liu[:vliu]]

Attachment: WIP V2

Hi Philikon,

> I would suggest that while we change the radio power, we keep a flag around
> until we get the next radio state changed notification. If the
> `ril.radio.enabled` setting changes at any time in between that, we ignore
> it. In the radio state changed notification, we again compare the radio
> state to the `ril.radio.enabled` setting and make adjustments if they're out
> of sync.

Based on your suggestion, I implemented the code as WIP V2. The main idea is below.
1. The _radioEnabled was tracked by _currentradioEnabled only when debouncing was deactivated.
2. During debouncing, ignoring radio power adjustment if _currentradioEnabled and _radioEnabled were different.
3. _currentradioEnabled won't tracking until deactivating debouncing.

Can you please have a review and give me any suggestion? Thanks.

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to Shian-Yow Wu from comment #6)
> As my understanding, out of sync handling should be already handled in
> current code logic.
> 
> _ensureRadioState() will called inside handleRadioStateChange() where
> there's a state change:
> https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js?rev=3abe394b6110#735

Very good point!

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 663964 [details] [diff] [review]
WIP V2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +715,5 @@
>        return;
>      }
> +    if (this._currentradioEnabled != this._radioEnabled) {
> +      // During debouncing, we ignored radio power adjustments when the
> +      // radio state is out of sync.

Well the point is to ignore *all* radio power adjustments while we're bringing up or down the radio power. Why won't 1 flag be enough? Like this:

  if (this._changingRadioPower) {
    // We're changing the radio power currently, ignore any changes.
    return;
  }

and then just add

  this._changingRadioPower = true;

to the `setRadioEnabled()` method and

  this._changingRadioPower = false;

to the `handleRadioStateChange()` method before we call _ensureRadioState(). Seems way simpler to me.

Comment 11

[Vincent Liu[:vliu]]

Attachment: WIP V3

(In reply to Philipp von Weitershausen [:philikon] from comment #10)
> Comment on attachment 663964 [details] [diff] [review]
> WIP V2
> 
> Review of attachment 663964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +715,5 @@
> >        return;
> >      }
> > +    if (this._currentradioEnabled != this._radioEnabled) {
> > +      // During debouncing, we ignored radio power adjustments when the
> > +      // radio state is out of sync.
> 
> Well the point is to ignore *all* radio power adjustments while we're
> bringing up or down the radio power. 

Yes, your point is right and I should focus on it. Thanks.

Comment 12

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665228 [details] [diff] [review]
WIP V3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +728,5 @@
>      }
>      this.rilContext.radioState = newState;
>      //TODO Should we notify this change as a card state change?
>  
> +    this._changingRadioPower = false;

Please put this at the top of the function so we always execute this, even if we take the early exit in the `if`.

@@ +758,5 @@
>  
>      if (this.rilContext.radioState == RIL.GECKO_RADIOSTATE_OFF &&
>          this._radioEnabled) {
>        this.setRadioEnabled(true);
> +      this._changingRadioPower = true;

Please move this to `setRadioEnabled()`

@@ +1249,5 @@
>    _radioEnabled: null,
>  
> +  // Flag to ignore any radio power change requests during We're changing
> +  // the radio power.
> +  _changingRadioPower: null,

false, please.

Comment 13

[Vincent Liu[:vliu]]

Attachment: WIP V4

Hi Philikon,

Thanks for your code review on comment 12.

Comment 14

[Shian-Yow Wu [:swu]]

Probably we should set this to blocking-basecamp+ because when the bug occurs, a reboot is needed to recover.

Comment 15

[Andrew Overholt [:overholt]]

Blocking based on comment #14.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d3a2496dc9

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c2d3a2496dc9