806307 - B2G RIL: shouldn't send icc-related messages unless content is ready

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vincent Liu[:vliu]]

Please see the below link.

https://bugzilla.mozilla.org/show_bug.cgi?id=805911#c3

Expected result: shows "Emergency Calls Only(No SIM)"
Actual result : shows "Emergency Calls Only"

Comment 1

[Vincent Liu[:vliu]]

Added logs to observe it. 

1. cardstatechange was checked in onmessage() in RadioInterfaceLayer.js and run _sendMobileConnectionMessage() with RIL:CardStateChanged to sent this message to RILContentHelper.
2. In RILContentHelper, it didn't receive this message to do further actions. I think that's the problem for this issue.

Comment 2

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

(In reply to vliu from comment #0)
> Please see the below link.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=805911#c3
> 
> Expected result: shows "Emergency Calls Only(No SIM)"
> Actual result : shows "Emergency Calls Only"

Hi vliu, 
"Emeregency calls only (no sim)" is displayed on my phone.

RILContentHelper receives the message as below:

I/Gecko   (  105): -*- RILContentHelper: Received message 'RIL:CardStateChanged': {"rilMessageType":"cardstatechange","cardState":"absent"}

Comment 3

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> (In reply to vliu from comment #0)
> > Please see the below link.
> > 
> > https://bugzilla.mozilla.org/show_bug.cgi?id=805911#c3
> > 
> > Expected result: shows "Emergency Calls Only(No SIM)"
> > Actual result : shows "Emergency Calls Only"
> 
> Hi vliu, 
> "Emeregency calls only (no sim)" is displayed on my phone.
> 
> RILContentHelper receives the message as below:
> 
> I/Gecko   (  105): -*- RILContentHelper: Received message
> 'RIL:CardStateChanged':
> {"rilMessageType":"cardstatechange","cardState":"absent"}

Sorry, I can reproduce this issue after I updated my code.

Gecko commit 491180673e924873afd41ea8fc9c12ce99a0f810

Comment 4

[Vincent Liu[:vliu]]

Hi Hsin-Yi,

Thanks for your confirmation. :) Take logs with time information, I observed the below situation.

1. RadioInterfaceLayer received incoming message cardstatechange was at "10-29 17:41:54.644 I/Gecko". The log point was showed below.
http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#501

2. receiveMessage() in RILContentHelper start receiving the first message was at "10-29 17:42:11" 

It seems the RILContentHelper not even ready to receive any messages when RadioInterfaceLayer wanted to send Cardstatechange to RILContentHelper. 

I am not sure this message will be queued for later dealt or just missing it. Maybe anyone can give me suggestion. Thanks.

Comment 5

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

(In reply to vliu from comment #4)
> Hi Hsin-Yi,
> 
> Thanks for your confirmation. :) Take logs with time information, I observed
> the below situation.
> 
> 1. RadioInterfaceLayer received incoming message cardstatechange was at
> "10-29 17:41:54.644 I/Gecko". The log point was showed below.
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js#501
> 
> 2. receiveMessage() in RILContentHelper start receiving the first message
> was at "10-29 17:42:11" 
> 
> It seems the RILContentHelper not even ready to receive any messages when
> RadioInterfaceLayer wanted to send Cardstatechange to RILContentHelper. 
> 
> I am not sure this message will be queued for later dealt or just missing
> it. Maybe anyone can give me suggestion. Thanks.

Oh... no, we don't queue the icc-related messages in RadioInterfaceLayer. What we only have is "system-message-listener-ready".  We don't turn on the 'radio' until we receive 'system-message-listener-ready', but the icc-related messages might arrive earlier than 'system-message-listener-ready'.

Comment 6

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Hi everyone,

Based on my test,
On devices without SIM card, several messages will be sent to RadioInterfaceLayer before 'system-message-listener-ready':
RIL:VoiceInfoChanged, RIL:DataInfoChanged, RIL:NetworkSelectionModeChanged, and RIL:CardStateChanged
The former three messages are meaningless on no-SIM-card situation.

On devices with SIM card, two more messages are sent before 'system-message-listener-ready': RIL:stkCommand and RIL:IccInfoChanged
RIL:VoiceInfoChanged and RIL:DataInfoChanged are triggered periodically, I think they can be lost.
I am not sure about RIL:NetworkSelectionModeChanged, RIL:stkCommand and RIL:IccInfoChanged.
They are not being resent, but device works fine even they are lost.


So the straightforward way is simply create a RIL:CardStateChanged message(maybe other required message) to listener after it is ready.

Another method is queuing all messages before listener is ready, and resend them after ready. But it requires a mutex, which is not supported in javascript, to prevent race condition on queue data.

I also think about resetting SIM card after listener is ready, then everything works on its own. I am not sure it is doable now.

Currently, I prefer the first method, create and send required message after listener is ready.

Comment 7

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

(In reply to Chuck Lee [:chucklee] from comment #6)
> Hi everyone,
> 
> Based on my test,
> On devices without SIM card, several messages will be sent to
> RadioInterfaceLayer before 'system-message-listener-ready':
> RIL:VoiceInfoChanged, RIL:DataInfoChanged, RIL:NetworkSelectionModeChanged,
> and RIL:CardStateChanged
> The former three messages are meaningless on no-SIM-card situation.
>

Not exactly, RIL:VoiceInfoChanged provides "signal strength" even no sim inserted.

> On devices with SIM card, two more messages are sent before
> 'system-message-listener-ready': RIL:stkCommand and RIL:IccInfoChanged
> RIL:VoiceInfoChanged and RIL:DataInfoChanged are triggered periodically, I
> think they can be lost.

Not exactly.  RIL:VoiceInfoChanged and RIL:DataInfoChanged are not sent periodically. They are sent when the info changes.

> I am not sure about RIL:NetworkSelectionModeChanged, RIL:stkCommand and
> RIL:IccInfoChanged.
> They are not being resent, but device works fine even they are lost.
> 
RIL:StkCommand isn't resent but it's cached by SystemMessage, so no problem with that.

RIL:IccInfoChanged provides the icc and operator info. 

I am not very sure about RIL:NetworkSelectionModeChanged, either.

Comment 8

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Thanks for the detailed information, Hsin-Yi.
So it's better to queue all messages except RIL:StkCommand, right?

To decrease the number of queued objects, is it acceptable for the queue to only accept one message per message type(RIL:XXXX)?

Comment 9

[Andrew Overholt [:overholt]]

Shian-Yow, can this cause other user-visible issues?

Comment 10

[Shian-Yow Wu [:swu]]

(In reply to Andrew Overholt [:overholt] from comment #9)
> Shian-Yow, can this cause other user-visible issues?

Currently it happens only on RIL:IccInfoChanged which caused this display issue.  The same problem could also happen in RIL:VoiceInfoChanged in racing condition, and that case will block making call.

Comment 11

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Queue messages before listener is ready, and resend them after listener is ready

It seems better to preserve all messages for listener, so I implement the message queue with limitation of one message per message type.

Messages in queue will be resend after listener ready event is arrived.

For the racing condition, I think run-to-completion should block new message event while resending queued messages.

Comment 12

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Queue messages before listener is ready, and resend them after listener is ready

Some modification to meet the coding style

Comment 13

[Andrew Overholt [:overholt]]

(In reply to Shian-Yow Wu from comment #10)
> (In reply to Andrew Overholt [:overholt] from comment #9)
> > Shian-Yow, can this cause other user-visible issues?
> 
> Currently it happens only on RIL:IccInfoChanged which caused this display
> issue.  The same problem could also happen in RIL:VoiceInfoChanged in racing
> condition, and that case will block making call.

Okay, let's get this fixed.

Comment 14

[Philipp von Weitershausen [:philikon]]

Comment on attachment 677325 [details] [diff] [review]
Queue messages before listener is ready, and resend them after listener is ready

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

I only looked at this briefly, so apologies for the naive question: the system message queuing mechanism implemented in shell.js [1] does not suffice here?

[1] https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#577

Comment 15

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

As my understanding, the queuing mechanism is for system events to observers, but these messages from RadioInterfaceLayer are IPC messages to listeners. They are handled in different way.

Another issue is these messages are dropped due to lack of IPC target, even before entering any IPC/event handling process:

In _sendTargetMessage() [1], it requires target to apply sendAsyncMessage(), or it just returns and drop the message.

The message target is registered through "RIL:RegisterXXXXMsg", "RIL:RegisterMobileConnectionMsg" for example [2], which is sent by RILContentHelper [3].

Because RILContentHelper is up later than RadioInterfaceLayer, "RIL:RegisterMobileConnectionMsg" always arrives later than some messages from ril_worker, like memtioned "RIL:CardStateChanged".

As the result, these messages are simply dropped.

So I think of queuing them in RadioInterfaceLayer.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#663
[2] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#430
[3] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#601

Comment 16

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

According to the info from jaoo, in addition to RIL:CardStateChanged, RIL:IccInfoChanged was also sent too early that causes mcc and mmc not available to content.

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

Any ETA for landing this?

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 677325 [details] [diff] [review]
Queue messages before listener is ready, and resend them after listener is ready

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +927,5 @@
>  
>      this._ensureRadioState();
>    },
>  
> +  _enqueueTargetMessage: function _enqueueTargetMessage(msgPermission, msgMessage, msgOptions) {

Please use the same naming with original ones.

@@ +931,5 @@
> +  _enqueueTargetMessage: function _enqueueTargetMessage(msgPermission, msgMessage, msgOptions) {
> +    // Queuing message is no longer required after listener is ready.
> +    if (this._sysMsgListenerReady) {
> +      return;
> +    }

We don't have to check same condition for twice.

@@ +933,5 @@
> +    if (this._sysMsgListenerReady) {
> +      return;
> +    }
> +
> +    // Message types that don't need to be queued.

I think I'll need Yoshi to comfirm this.

@@ +936,5 @@
> +
> +    // Message types that don't need to be queued.
> +    // "RIL:StkCommand" will be queued by system.
> +    let excludeMessageList = ["RIL:StkCommand"];
> +    if (excludeMessageList.indexOf(msgMessage) !== -1) {

If there is always only one element, please compare it directly.

@@ +951,5 @@
> +      if (msgQueue[i].message === msgMessage ) {
> +        msgQueue.splice(i, 1);
> +        break;
> +      }
> +    }

Since you're comparing by message type, why not:

  function RadioInterfaceLayer() {
    this._targetMessageQueue = {};
  }
  ...
  function _enqueueTargetMessage() {
    ...
    this._targetMessageQueue[msgMessage] = {
      ...
    };
  }

@@ +962,5 @@
> +    // enqueue message if listener is not ready.
> +    // So resend is only accepted while listener is ready
> +    if (!this._sysMsgListenerReady) {
> +      return;
> +    }

You really don't have to check everything twice.

@@ +969,5 @@
> +    let msgQueue = this._targetMessageQueue;
> +    while ( msgQueue.length !== 0 ) {
> +      let msg = msgQueue.shift();
> +      this._sendTargetMessage(msg.permission, msg.message, msg.options );
> +    }

And:

  for each (let msg in this._targetMessageQueue) {
    this._sendTargetMessage(msg.permission, msg.message, msg.options );
  }
  this._targetMessageQueue = null;

Comment 19

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

Comment on attachment 677325 [details] [diff] [review]
Queue messages before listener is ready, and resend them after listener is ready

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +935,5 @@
> +    }
> +
> +    // Message types that don't need to be queued.
> +    // "RIL:StkCommand" will be queued by system.
> +    let excludeMessageList = ["RIL:StkCommand"];

should be excluded with d
Also why sometimes you use msg, sometimes you use message?

Comment 21

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> Comment on attachment 677325 [details] [diff] [review]
> Queue messages before listener is ready, and resend them after listener is
> ready
> 
> Review of attachment 677325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +936,5 @@
> > +
> > +    // Message types that don't need to be queued.
> > +    // "RIL:StkCommand" will be queued by system.
> > +    let excludeMessageList = ["RIL:StkCommand"];
> > +    if (excludeMessageList.indexOf(msgMessage) !== -1) {
> 
> If there is always only one element, please compare it directly.

I'll change this after Yoshi confirms message type to be excluded.

> 
> @@ +951,5 @@
> > +      if (msgQueue[i].message === msgMessage ) {
> > +        msgQueue.splice(i, 1);
> > +        break;
> > +      }
> > +    }
> 
> Since you're comparing by message type, why not:
> 
>   function RadioInterfaceLayer() {
>     this._targetMessageQueue = {};
>   }
>   ...
>   function _enqueueTargetMessage() {
>     ...
>     this._targetMessageQueue[msgMessage] = {
>       ...
>     };
>   }

I use array to keep order of messages in case the order matters.

Comment 22

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #19)
> Comment on attachment 677325 [details] [diff] [review]
> Queue messages before listener is ready, and resend them after listener is
> ready
> 
> Review of attachment 677325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +935,5 @@
> > +    }
> > +
> > +    // Message types that don't need to be queued.
> > +    // "RIL:StkCommand" will be queued by system.
> > +    let excludeMessageList = ["RIL:StkCommand"];
> 
> should be excluded with d

Sorry, but I can't catch what "d" means.

> Also why sometimes you use msg, sometimes you use message?

I use msg while it's in the prefix, I'll fix that.

Comment 23

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

(In reply to Chuck Lee [:chucklee] from comment #22)
> (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #19)
> > > +    let excludeMessageList = ["RIL:StkCommand"];
> > 
> > should be excluded with d
> 
> Sorry, but I can't catch what "d" means.
> 
excludedMessageList.

Comment 24

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #23)
> (In reply to Chuck Lee [:chucklee] from comment #22)
> > (In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #19)
> > > > +    let excludeMessageList = ["RIL:StkCommand"];
> > > 
> > > should be excluded with d
> > 
> > Sorry, but I can't catch what "d" means.
> > 
> excludedMessageList.

Vicamo says |excludedMessageList| is not necessary if there's only one message to be excluded now.
Do you think of any other message?

Comment 25

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

My second thought is we don't have to exclude any message.

Comment 26

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Queue messages before listener is ready, and resend them after listener is ready, v2

1. Remove redundant codes as commented.
2. Queue all types of messages.
3. Fix variable naming.

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 680578 [details] [diff] [review]
Queue messages before listener is ready, and resend them after listener is ready, v2

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

Great! Thank you :)

Comment 28

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try Server : https://tbpl.mozilla.org/?tree=Try&rev=65e7d688bcc7

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/424d44523245

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/424d44523245

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a109ec8ca659