Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Description • 10 years ago

I'd like to divide bug 864489 into several tasks.

This bug to refactor STK with IPDL.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 1 • 10 years ago

It shall be MozIcc.webidl instead of MozIccManager.webidl to prevent confusing.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 2 • 9 years ago

[Tracking Requested - why for this release]:

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 3 • 9 years ago

This patch is to 
1. Refactor StkProactiveCmdFactory.jsm as a XPCOM service.
2. Move nsIStkXXX interfaces from nsIIccMessenger.idl to nsIStkProactiveCmd.idl where nsIStkProactiveCmd.idl is the pool for all STK related XPCOM components.

Hi Edgar,

May I have your early feedback for this changes?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 4 • 9 years ago

This patch is to move the notification of STK proactive commands from IccProvider to IccService.

Hi Edgar,

May I have your early feedback for these changes?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 5 • 9 years ago

This patch is to move the STK related requests from IccProvider to IccService.

Hi Edgar,

May I have your early feedback for these changes?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 6 • 9 years ago

This patch is to deprecate the implementation in IccProvider.

Hi Edgar,

May I have your early feedback for these changes?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 7 • 9 years ago

This patch is to correct the data type of mcc/mnc to DOMString which is actually used by gaia, IccInfo, MobileConnection.

Hi Edgar,

May I have your early feedback of this change?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 8 • 9 years ago

Comment on attachment 8605777 [details] [diff] [review]
Part 1 v1: Refactor StkProactiveCmdFactory.jsm into a XPCOM Service.

Per offline discussion, we can have more easier way to implement IPDL with JSON.
Since it won't take too much time to finish the IPDL implementation,
I'll provide formal patch to review directly.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 9 • 9 years ago

Comment on attachment 8605779 [details] [diff] [review]
Part 2.1 v1: (Gonk) Refactor Stk Proactive Commands from IccProvider to IccService.

Per offline discussion, we can have more easier way to implement IPDL with JSON.
Since it won't take too much time to finish the IPDL implementation,
I'll provide formal patch to review directly.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 10 • 9 years ago

Comment on attachment 8605781 [details] [diff] [review]
Part 2.2 v1: (Gonk) Refactor Stk Request APIs from IccProvider to IccService.

Per offline discussion, we can have more easier way to implement IPDL with JSON.
Since it won't take too much time to finish the IPDL implementation,
I'll provide formal patch to review directly.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 11 • 9 years ago

Comment on attachment 8605782 [details] [diff] [review]
Part 2.3 v1: (Gonk) Remove Implementation from IccProvider.

Per offline discussion, we can have more easier way to implement IPDL with JSON.
Since it won't take too much time to finish the IPDL implementation,
I'll provide formal patch to review directly.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 12 • 9 years ago

Comment on attachment 8605784 [details] [diff] [review]
Part 2.4 v1: Type of MNC/MCC shall be the same to the one defined in MozMobileNetworkInfo.

Per offline discussion, we can have more easier way to implement IPDL with JSON.
Since it won't take too much time to finish the IPDL implementation,
I'll provide formal patch to review directly.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 13 • 9 years ago

1. Move STK related XPCOM interfaces from nsIIccMessenger.idl to newly created nsIStkProactiveCmd.idl
2. Refactor/Rename StkProactiveCmdFactory.jsm to StkCmdFactory.js as a XPCOM service.

Hi Edgar,

May I have your review for these changes?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 14 • 9 years ago

This patch is to migrate the notification of STK proactive command from IccProvider to IccService.

Hi Edgar,

May I have your review for these changes?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 15 • 9 years ago

This patch is to migrate STK related requests from IccProvider to IccService.

Hi Edgar,

May I have your review for these changes?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 16 • 9 years ago

This Patch is to remove the STK related implementation from IccProvider.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 17 • 9 years ago

Add IPDL declaration for STK-related transactions.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 18 • 9 years ago

Add Helper to deflate/inflate XPCOM object to/from JSON for IPC.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 19 • 9 years ago

Add IPDL implementation.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 20 • 9 years ago

Hi Edgar & Hsinyi,

Actually, the data type of mnc/mcc used by Icc [1], MobileConnection [2] and Gaia [3] is a DOMString instead of an integer type.

Hence, I'd like to modification the webidl definition accordingly to prevent any confusion.

Would you minding reviewing this change?

Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozIccInfo.webidl#21-29
[2] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileNetworkInfo.webidl#20-28
[3] https://github.com/mozilla-b2g/gaia/blob/d544c9b2a77a889d87566aff0ca957da5089a49e/apps/system/js/icc_events.js#L13-L47

	Edgar Chen [:edgar]
	Comment 21 • 9 years ago

Comment on attachment 8606862 [details] [diff] [review]
(v2) Part 1: Refactor StkProactiveCmdFactory.jsm into a XPCOM Service.

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

Looks good to me. Thank you.

::: dom/icc/interfaces/nsIStkCmdFactory.idl
@@ +27,5 @@
> +   */
> +  nsIStkProactiveCmd createCommand(in jsval aCommandDetails);
> +
> +  /**
> +   * @param  nsIStkProactiveCmd instance.

Nit:
@param aStkProactiveCmd
       a nsIStkProactiveCmd instance.

::: dom/system/gonk/RILSystemMessenger.jsm
@@ +33,5 @@
>    },
>  
>    /**
> +   * Hook of the function to create MozStkCommand message.
> +   * @param  nsIStkProactiveCmd instance.

Ditto.

	Edgar Chen [:edgar]
	Comment 22 • 9 years ago

Comment on attachment 8606863 [details] [diff] [review]
(v2) Part 2.1: (Gonk) Refactor Stk Proactive Commands from IccProvider to IccService.

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

Mostly looks good but I would like to review the revised version again. Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2025,5 @@
>      if (DEBUG) this.debug("handleStkProactiveCommand " + JSON.stringify(message));
>      let iccInfo = this.getIccInfo();
>      if (iccInfo && iccInfo.iccid) {
> +      let command = gStkCmdFactory.createCommand(message);
> +      gIccMessenger.notifyStkProactiveCommand(iccInfo.iccid, command);

Handle system message things in IccService.
And some utility functions probably can be removed as well, e.g. getIccInfo() ... etc.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 23 • 9 years ago

(In reply to Edgar Chen [:edgar][:echen] from comment #21)
> 
> ::: dom/icc/interfaces/nsIStkCmdFactory.idl
> @@ +27,5 @@
> > +   */
> > +  nsIStkProactiveCmd createCommand(in jsval aCommandDetails);
> > +
> > +  /**
> > +   * @param  nsIStkProactiveCmd instance.
> 
> Nit:
> @param aStkProactiveCmd
>        a nsIStkProactiveCmd instance.
> 
Oops, this change was done in patch part 2.2 accidentally.
I'll move it to patch part 2.1 instead.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 24 • 9 years ago

(In reply to Edgar Chen [:edgar][:echen] from comment #22)
> Comment on attachment 8606863 [details] [diff] [review]
> (v2) Part 2.1: (Gonk) Refactor Stk Proactive Commands from IccProvider to
> IccService.
> 
> Review of attachment 8606863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly looks good but I would like to review the revised version again.
> Thank you.
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +2025,5 @@
> >      if (DEBUG) this.debug("handleStkProactiveCommand " + JSON.stringify(message));
> >      let iccInfo = this.getIccInfo();
> >      if (iccInfo && iccInfo.iccid) {
> > +      let command = gStkCmdFactory.createCommand(message);
> > +      gIccMessenger.notifyStkProactiveCommand(iccInfo.iccid, command);
> 
> Handle system message things in IccService.
> And some utility functions probably can be removed as well, e.g.
> getIccInfo() ... etc.

Thanks for reminding this.
I'll address this in next update!

	Edgar Chen [:edgar]
	Comment 25 • 9 years ago

Comment on attachment 8606869 [details] [diff] [review]
(v2) Part 2.2: (Gonk) Refactor Stk Request APIs from IccProvider to IccService.

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

Please see my comments below, thank you.

::: dom/icc/gonk/StkCmdFactory.js
@@ +200,4 @@
>  };
>  
>  /**
> + * The implementation of nsIStkProactiveCommand set and paired JS object set.

Move this change to Part 1.

@@ +1038,5 @@
> +    this.additionalInformation = aStkTerminalResponse.additionalInformation;
> +  }
> +}
> +StkResponseMessage.prototype = {
> +  resultCode: RIL.STK_RESULT_OK

Most of Stk*Message doesn't give a default value to their non-optional attribute. Why give a default value for this one? Is there any special reason?

@@ +1077,5 @@
> +  if (aStkGetInputResponseMessage.input !== undefined) {
> +    // We expect input to be "" if user confirmed the input with nothing,
> +    // and we use null to present 'undefined' internally for the conversion
> +    // between nsIStkTerminalResponse and JS objects.
> +    this.input = (aStkGetInputResponseMessage.input === null)

this.input = aStkGetInputResponseMessage.input || "";

@@ +1505,5 @@
> +      throw new Error("Invalid event message: " + JSON.stringify(aEventMessage));
> +    }
> +
> +    switch (aEventMessage.eventType) {
> +      case RIL.STK_EVENT_TYPE_MT_CALL:

We have a mapping for proactive command. e.g. `MsgPrototypes[RIL.STK_CMD_FOO] = StkBar;`.
Should we use the same way for download event?

@@ +1528,5 @@
> +      throw new Error("Invalid download event: " + JSON.stringify(aStkDownloadEvent));
> +    }
> +
> +    let event;
> +    if (aStkDownloadEvent instanceof Ci.nsIStkLocationEvent) {

Ditto, we have a mapping for proactive command, e.g. `QueriedIFs[RIL.STK_CMD_FOO] = Ci.nsIStkBar;`.
Should we use the same way for download event?

::: dom/icc/interfaces/nsIIccProvider.idl
@@ -10,5 @@
>  
>  /**
>   * XPCOM component (in the content process) that provides the ICC information.
>   */
>  [scriptable, uuid(2fbacfc4-f52d-11e4-9667-33b72f279d14)]

Require a UUID bump.

@@ -19,5 @@
> -   * RadioInterfaceLayer in the chrome process. Only a content process that has
> -   * the 'mobileconnection' permission is allowed to register.
> -   */
> -  void registerIccMsg(in unsigned long clientId, in nsIIccListener listener);
> -  void unregisterIccMsg(in unsigned long clientId, in nsIIccListener listener);

Should the removing {register|unregister}IccMsg belong to Part 2.1?

::: dom/icc/interfaces/nsIIccService.idl
@@ +10,3 @@
>  interface nsIStkProactiveCmd;
> +interface nsIStkTerminalResponse;
> +interface nsIStkTimer;

Don't need this forward declaration for nsIStkTimer.

@@ +458,5 @@
> +  /**
> +   * Send envelope to notify the expiration of a requested timer.
> +   *
> +   * @param aTimer
> +   *        nsIStkTimer with timer id and real used seconds when expired.

Please update the comments.

@@ +467,5 @@
> +  /**
> +   * Send "Event Download" envelope to the ICC.
> +   *
> +   * @param nsIStkDownloadEvent
> +   *        The event that ICC listening to in STK_CMD_SET_UP_EVENT_LIST.

@param aEvent ....

::: dom/icc/interfaces/nsIStkCmdFactory.idl
@@ +22,5 @@
>  interface nsIStkCmdFactory : nsISupports
>  {
>    /**
>     * @param  aCommandDetails
> +   *         A JS object which complies with 'dictionary MozStkCommand'

Move this change to Part 1.

@@ +31,5 @@
>    nsIStkProactiveCmd createCommand(in jsval aCommandDetails);
>  
>    /**
> +   * @param  aStkProactiveCmd
> +   *         a nsIStkProactiveCmd instance.

Ditto.

@@ +36,2 @@
>     *
> +   * @return a JS object which complies with 'dictionary MozStkCommand'

Ditto.

@@ +63,5 @@
> +   *        in MozStkCommandEvent.webidl
> +   *
> +   * @return a nsIStkDownloadEvent instance.
> +   */
> +  nsIStkDownloadEvent createDownloadEvent(in jsval aEventMessage);

Since we have |createEventMessage| below, just follow the same naming rule, s/createDownloadEvent/createEvent/.

@@ +81,5 @@
> +   *        in MozStkCommandEvent.webidl
> +   *
> +   * Note: This API is specific to nsIIcc::sendStkTimerExpiration().
> +   */
> +   nsIStkTimer createStkTimer(in jsval aStkTimerMessage);

Since other function doesn't contain `Stk`, let's just follow the same rule, s/createStkTimer/createTimer/.

::: dom/icc/interfaces/nsIStkProactiveCmd.idl
@@ +825,5 @@
> +{
> +  /**
> +   * Indicate current service state of the MS with one of the values listed
> +   * below:
> +   *  - STK_SERVICE_STATE_NORMAL

Define consts for STK_SERVICE_STATE_*?

@@ +898,5 @@
> +  /**
> +   * The browser termination cause.
> +   * It shall be one of following:
> +   * - STK_BROWSER_TERMINATION_CAUSE_USER
> +   * - STK_BROWSER_TERMINATION_CAUSE_ERROR

Define consts for STK_BROWSER_TERMINATION_CAUSE_*?

	Edgar Chen [:edgar]
	Comment 26 • 9 years ago

Comment on attachment 8606873 [details] [diff] [review]
(v2) Part 3.1: (IPC) IPDL Declaration.

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

Looks good, thank you.

::: dom/icc/ipc/PIcc.ipdl
@@ +89,5 @@
>  
> +  /**
> +   * Notify STK proactive command issue by selected UICC.
> +   */
> +  NotifyStkCommand(nsString aStkProactiveCmd);

Add some comments about aStkProactiveCmd is json format.

@@ +110,5 @@
>  
>    /**
> +   * Send STK response to the selected UICC.
> +   */
> +  StkResponse(nsString aCommand, nsString aResponse);

Ditto.

@@ +125,5 @@
> +
> +  /**
> +   * Send STK Event Download to the selected UICC.
> +   */
> +  StkEventDownload(nsString aEvent);

Ditto.

	Edgar Chen [:edgar]
	Comment 27 • 9 years ago

Comment on attachment 8606874 [details] [diff] [review]
(v2) Part 3.2: (IPC) Add Helper to deflate/inflate XPCOM object to/from JSON for IPC.

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

::: dom/icc/gonk/StkCmdFactory.js
@@ +1153,5 @@
>      return;
>    }
>  
>    if (localInfo.date) {
> +    if (localInfo.date.getTime) {

What about `if (localInfo.date instanceof Date) {`?

	Edgar Chen [:edgar]
	Comment 28 • 9 years ago

Comment on attachment 8606875 [details] [diff] [review]
(v2) Part 3.3: (IPC) IPDL Implementation.

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

r=me with comments addressed, thank you.

::: dom/icc/ipc/IccChild.cpp
@@ +322,5 @@
> +  nsCOMPtr<nsIStkCmdFactory> cmdFactory =
> +    do_GetService(ICC_STK_CMD_FACTORY_CONTRACTID);
> +  NS_ENSURE_TRUE(cmdFactory, NS_ERROR_FAILURE);
> +
> +  nsString cmd, response;

Use nsAutoString which supports stack-based string allocation.

@@ +356,5 @@
> +  nsCOMPtr<nsIStkCmdFactory> cmdFactory =
> +    do_GetService(ICC_STK_CMD_FACTORY_CONTRACTID);
> +  NS_ENSURE_TRUE(cmdFactory, NS_ERROR_FAILURE);
> +
> +  nsString event;

nsAutoString

::: dom/icc/ipc/IccParent.cpp
@@ +228,5 @@
> +  nsCOMPtr<nsIStkCmdFactory> cmdFactory =
> +    do_GetService(ICC_STK_CMD_FACTORY_CONTRACTID);
> +  NS_ENSURE_TRUE(cmdFactory, NS_ERROR_UNEXPECTED);
> +
> +  nsString cmd;

nsAutoString

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 29 • 9 years ago

address nits in comment 21.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 30 • 9 years ago

Address comment 22 to handle both system message & stk event in IccService.

Hi Edgar,

May I have your review again?

Thanks!

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 31 • 9 years ago

1. Define STK related constants (STK_SERVICE_*, STK_BROWSER_TERMINATION_CAUSE_*, RESULT_*) in nsIStkProactiveCmd.idl.
2. Define maps to create Events accordingly.
3. Naming.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 32 • 9 years ago

rebased for v3.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 33 • 9 years ago

address comment 26.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 34 • 9 years ago

address suggestion in comment 27.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 35 • 9 years ago

address comment 28.

	Edgar Chen [:edgar]
	Comment 36 • 9 years ago

Comment on attachment 8609984 [details] [diff] [review]
(v3) Part 2.1: (Gonk) Refactor Stk Proactive Commands from IccProvider to IccService.

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

Nice. Thank you.

	Edgar Chen [:edgar]
	Comment 37 • 9 years ago

Comment on attachment 8609986 [details] [diff] [review]
(v3) Part 2.2: (Gonk) Refactor Stk Request APIs from IccProvider to IccService.

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

r=me with comments addressed, thank you.

::: dom/icc/interfaces/nsIIccService.idl
@@ +459,5 @@
> +   *
> +   * @param aTimerId
> +   *        The TimerId provided from previous proactive command.
> +   * @param aTimerValue
> +   *        real used seconds when expired.

Nit: s/real/Real/

::: dom/icc/interfaces/nsIStkCmdFactory.idl
@@ +39,5 @@
>     */
>    jsval createCommandMessage(in nsIStkProactiveCmd aStkProactiveCmd);
> +
> +  /**
> +   * @param aResponse

s/aResponse/aResponseMessage/

@@ +78,5 @@
> +  /**
> +   * @param aStkTimerMessage
> +   *        a JS object which complies with 'dictionary MozStkTimer'
> +   *        in MozStkCommandEvent.webidl
> +   *

Add documentation about return value. e.g. @return A nsIStkTimer instance.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 38 • 9 years ago

update final patch.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 39 • 9 years ago

update final patch.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 40 • 9 years ago

update final patch.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 41 • 9 years ago

update final patch.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 42 • 9 years ago

update final patch.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 43 • 9 years ago

update final patch.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 44 • 9 years ago

update final patch.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 45 • 9 years ago

update final patch.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 46 • 9 years ago

try server is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9e9792991cc

	Pulsebot
	Comment 47 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/23a81ddec869
https://hg.mozilla.org/integration/b2g-inbound/rev/ffead4c7b8c8
https://hg.mozilla.org/integration/b2g-inbound/rev/3a47c5e4aed4
https://hg.mozilla.org/integration/b2g-inbound/rev/849932b3d1ca
https://hg.mozilla.org/integration/b2g-inbound/rev/ea07a99e9bbf
https://hg.mozilla.org/integration/b2g-inbound/rev/7d60a39d95fc
https://hg.mozilla.org/integration/b2g-inbound/rev/6916cd8ab6ba
https://hg.mozilla.org/integration/b2g-inbound/rev/841b0ba3c666

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 48 • 9 years ago

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #46)
> try server is green:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9e9792991cc

You are our hero!

	Ryan VanderMeulen [:RyanVM]
	Comment 49 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/23a81ddec869
https://hg.mozilla.org/mozilla-central/rev/ffead4c7b8c8
https://hg.mozilla.org/mozilla-central/rev/3a47c5e4aed4
https://hg.mozilla.org/mozilla-central/rev/849932b3d1ca
https://hg.mozilla.org/mozilla-central/rev/ea07a99e9bbf
https://hg.mozilla.org/mozilla-central/rev/7d60a39d95fc
https://hg.mozilla.org/mozilla-central/rev/6916cd8ab6ba
https://hg.mozilla.org/mozilla-central/rev/841b0ba3c666

	Edgar Chen [:edgar]
	Comment 50 • 9 years ago

\o/

	:gerard-majax [PTO 01/08/2023-10/09/2023]
	Comment 51 • 9 years ago

We are getting reports of RIL breakage on Open C builds, as documented on https://bugzilla.frenchmozilla.org/show_bug.cgi?id=659#c4. Specifically, looking at RIL logcat output,

> 06-01 18:30:37.718   298   298 E GeckoConsole: [JavaScript Error: "TypeError: icc.iccInfo is null" {file: "jar:file:///system/b2g/omni.ja!/components/IccService.js" line: 142}]
> 06-01 18:30:37.718   298   298 E GeckoConsole: [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "icc.iccInfo is null" {file: "jar:file:///system/b2g/omni.ja!/components/IccService.js" line: 142}]'[JavaScript Error: "icc.iccInfo is null" {file: "jar:file:///system/b2g/omni.ja!/components/IccService.js" line: 142}]' when calling method: [nsIGonkIccService::notifyStkCommand]" {file: "jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js" line: 1068}]

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 52 • 9 years ago

(In reply to Alexandre LISSY :gerard-majax from comment #51)
> We are getting reports of RIL breakage on Open C builds, as documented on
> https://bugzilla.frenchmozilla.org/show_bug.cgi?id=659#c4. Specifically,
> looking at RIL logcat output,
> 
> > 06-01 18:30:37.718   298   298 E GeckoConsole: [JavaScript Error: "TypeError: icc.iccInfo is null" {file: "jar:file:///system/b2g/omni.ja!/components/IccService.js" line: 142}]
> > 06-01 18:30:37.718   298   298 E GeckoConsole: [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "icc.iccInfo is null" {file: "jar:file:///system/b2g/omni.ja!/components/IccService.js" line: 142}]'[JavaScript Error: "icc.iccInfo is null" {file: "jar:file:///system/b2g/omni.ja!/components/IccService.js" line: 142}]' when calling method: [nsIGonkIccService::notifyStkCommand]" {file: "jar:file:///system/b2g/omni.ja!/components/RadioInterfaceLayer.js" line: 1068}]

I'll look into this and feedback you soon.

Keep NI on me.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 53 • 9 years ago

I can reproduce this locally with the same symptom.
However, the problem that cause SIM card not detected is not related to the error message mentioned in comment 51. (It seems that rild will still report STK proactive command even the bootup sequence in ril_worker is pending.)

Actually, we never get any responses from rild after 1st RIL request 'setRadioEnabled' is sent, and there is no further SIM_STATUS_CHANGED reported from rild for ril_worker to fetch ICC related information in advance.

So far, I have no clue to prove that the patches of this bug will cause this problem.

I'll spend more time to narrow down the problem, so keep NI on me.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 54 • 9 years ago

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #53)
> I can reproduce this locally with the same symptom.
> However, the problem that cause SIM card not detected is not related to the
> error message mentioned in comment 51. (It seems that rild will still report
> STK proactive command even the bootup sequence in ril_worker is pending.)
> 
> Actually, we never get any responses from rild after 1st RIL request
> 'setRadioEnabled' is sent, and there is no further SIM_STATUS_CHANGED
> reported from rild for ril_worker to fetch ICC related information in
> advance.
> 
> So far, I have no clue to prove that the patches of this bug will cause this
> problem.
> 
> I'll spend more time to narrow down the problem, so keep NI on me.

Not a regression of this bug, because I can reproduce this problem with open_c with reversion 245775:14f0933c537c in m-c and there are 300+ change sets before this bug landed.

Note: This symptom seems only happened when device 1st boot-up and will be recovered if you stop/start b2g.

Clear NI and track this symptom in bug 1170467 instead.