Closed Bug 806307 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: vliu, Assigned: chucklee)

References

Details

Attachments

(1 file, 2 obsolete files)

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"
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.
(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"}
(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
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.
(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'.
blocking-basecamp: --- → ?
Summary: Emergency Calls Only is shown on lock screen without SIM is inserted. → B2G RIL: shouldn't send icc-related messages unless content is ready
Assignee: nobody → chulee
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.
(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.
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)?
Shian-Yow, can this cause other user-visible issues?
Flags: needinfo?(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.
Flags: needinfo?(swu)
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.
Attachment #677319 - Flags: review?(philipp)
Some modification to meet the coding style
Attachment #677319 - Attachment is obsolete: true
Attachment #677319 - Flags: review?(philipp)
Attachment #677325 - Flags: review?(philipp)
(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.
blocking-basecamp: ? → +
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
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
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.
Any ETA for landing this?
Attachment #677325 - Flags: review?(philipp) → review?(vyang)
Blocks: 809924
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;
Attachment #677325 - Flags: review?(vyang) → feedback?(allstars.chh)
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?
Attachment #677325 - Flags: feedback?(allstars.chh) → feedback+
(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.
(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.
(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.
(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?
My second thought is we don't have to exclude any message.
1. Remove redundant codes as commented.
2. Queue all types of messages.
3. Fix variable naming.
Attachment #677325 - Attachment is obsolete: true
Attachment #680578 - Flags: feedback?(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 :)
Attachment #680578 - Flags: feedback?(vyang) → review+
https://hg.mozilla.org/mozilla-central/rev/424d44523245
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.