1087847 - mozIccManager reports 0 SIMs, but mozMobileConnection does

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

Attachment: ril.log

See more detailed here, https://groups.google.com/forum/?hl=zh-TW#!topic/mozilla.dev.gaia/RHSqyuzoTxE

We found the same symptom when debugging bug 1084023 (copy the log here).
The message for creating Icc object was delayed about 8 sec during boot up process because RadioInterfaceLayer.js queue the message sent to the target until receiving "system-message-listener-ready" event [1][2].

The perfect solution is bug 864489, but it is a huge work. Considering the impact of the API behavior, we'd like to have a quick solution first.

[1] dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#389-392
[2] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#389-392

Comment 1

[Edgar Chen [:edgar]]

On a second thought, use two different IPC message to sync two modules wasn't a good idea.
No matter we sent which message first, there is still a timing that two modules are unsynced.
It seems the perfect way is two modules rely on the same IPC message (`IccInfoChanged` is the proper one in this case).

In this approach, the DOM implementation of ICC and MobileConnection are both registering the listener to IccProvider, then updating and scheduling a dom event when the `IccInfoChanged` callback is triggered.
And the IPC message and listener for iccId changes in MobileConnection could be removed.

Comment 2

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 3

[Salvador de la Puente González [:salva]]

Do we have any news about this? Are you asking for review, Edgar?

Comment 4

[Edgar Chen [:edgar]]

(In reply to Salvador de la Puente González [:salva] from comment #3)
> Do we have any news about this? Are you asking for review, Edgar?

Hi salva, I am trying to find a way to verify this patch. Will ask review later.

I had try to apply this patch to see if it can help bug 1084076, but the widget still displays 'NO SIM' after switching airplane mode. So I suspect there is still another issue causes bug 1084076. Just share this information to you.

Comment 5

[Marina Rodríguez [:mai]]

Hi Edgar, 
  We have to adapt the code of the widget for working with your solution. Currently, the widget updates its state, after disabling the airplanemode, when the iccdetected event is received. This solution works fine with the QC modems because the ril is powered off when the airplanemode is enabled.

 If you want to test your patch with the widget, we can prepare a specific patch for the widget. 

Regards

Comment 6

[Edgar Chen [:edgar]]

(In reply to Marina Rodríguez [:mai] from comment #5)
> Hi Edgar, 
>   We have to adapt the code of the widget for working with your solution.
> Currently, the widget updates its state, after disabling the airplanemode,
> when the iccdetected event is received. This solution works fine with the QC
> modems because the ril is powered off when the airplanemode is enabled.
> 
>  If you want to test your patch with the widget, we can prepare a specific
> patch for the widget. 
> 
> Regards

That's great, please share the patch to me. Thank you very much.

Comment 7

[Marina Rodríguez [:mai]]

Hi Edgar,
I prepare the following patch for testing purpose. I've added listeners for the iccchange events. I put traces when the listeners are added, when the listeners are removed and when a event is received. I can check the patch because I don't receive any event. (Don't discard errors ;)  )

https://github.com/mozilla-b2g/gaia/pull/26417

Best regards

Comment 8

[Edgar Chen [:edgar]]

Attachment: Patch, v2

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 8529406 [details] [diff] [review]
Patch, v2

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

After doing many tests in marionette and real devices, relying on one IPC message isn't enough, it still has potential timing problem (due to the callback sequence).
So in this version, I schedule the event dispatching in next event loop, then we can make sure the MobileConnection and IccManager are both ready before dispatching the event.

Hi Hsinyi, may I have your feedback about this solution?

And per offline discuss, I agree that the webapi needs a refinement.
I will file a separated bug for it, and we can have more discussion there, how do you think?

Thank you.

Comment 10

[Edgar Chen [:edgar]]

(In reply to Marina Rodríguez [:mai] from comment #7)
> Hi Edgar,
> I prepare the following patch for testing purpose. I've added listeners for
> the iccchange events. I put traces when the listeners are added, when the
> listeners are removed and when a event is received. I can check the patch
> because I don't receive any event. (Don't discard errors ;)  )
> 
> https://github.com/mozilla-b2g/gaia/pull/26417
> 
> Best regards

Move the discussion to bug 1084076. Thank you.

Comment 11

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

Comment on attachment 8529406 [details] [diff] [review]
Patch, v2

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

Hi Edgar,
I think the overall direction makes much sense. Thank you very much for the proposal :)

::: dom/icc/IccManager.h
@@ +13,5 @@
>  namespace mozilla {
>  namespace dom {
>  
> +class Icc;
> +class IccChangeEvent;

We don't really need this, right?

::: dom/icc/tests/marionette/test_icc_detected_undetected_event.js
@@ +35,5 @@
>         "should not get a valid icc object here");
>  
> +    // The mozMobileConnection.iccId should be in sync.
> +    is(navigator.mozMobileConnections[0].iccId, null,
> +       "check mozMobileConnection.iccId");

(y)

::: dom/mobileconnection/interfaces/nsIMobileConnectionService.idl
@@ -88,5 @@
>  
>    /**
> -   * Notify when icc id is changed.
> -   */
> -  void notifyIccChanged();

I like the change which looks a sensible direction :)

::: dom/mobileconnection/tests/marionette/test_mobile_icc_change.js
@@ +31,5 @@
> +
> +      // ICC object should be ready in mozIccManager.
> +      let icc = getMozIccByIccId(mobileConnection.iccId);
> +      ok(icc instanceof MozIcc, "icc should be an instance of MozIcc");
> +      is(icc.iccInfo.iccid, mobileConnection.iccId, "icc.iccInfo.iccid");

(y)

Comment 12

[Edgar Chen [:edgar]]

Attachment: Patch, v3

Address comment #11.

Comment 13

[Edgar Chen [:edgar]]

Comment on attachment 8539148 [details] [diff] [review]
Patch, v3

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

Hi Hsinyi, Olli, may I have your review? Please see comment #1 and comment #9 for more details. Thank you.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8539148 [details] [diff] [review]
Patch, v3

>+class IccManager::DispatchEventAsyncTask : public nsRunnable
Do you actually need this class?

IccChangeEventInit init;
init.mBubbles = false;
init.mCancelable = false;
init.mIccId = {some value};
nsRefPtr<IccChangeEvent> event =
  IccChangeEvent::Constructor(IccManager, {eventype}, init);
event->SetTrusted(true);
(new AsyncEventDispatcher(IccManager, event))->PostDOMEvent();
should work.

>+class MobileConnection::DispatchEventAsyncTask : public nsRunnable
ditto.
For this case AsyncEventDispatcher(dom::EventTarget* aTarget, const nsAString& aEventType, bool aBubbles)
would be useful constructor.

Comment 15

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8539148 [details] [diff] [review]
> Patch, v3
> 
> >+class IccManager::DispatchEventAsyncTask : public nsRunnable
> Do you actually need this class?
> 
> IccChangeEventInit init;
> init.mBubbles = false;
> init.mCancelable = false;
> init.mIccId = {some value};
> nsRefPtr<IccChangeEvent> event =
>   IccChangeEvent::Constructor(IccManager, {eventype}, init);
> event->SetTrusted(true);
> (new AsyncEventDispatcher(IccManager, event))->PostDOMEvent();
> should work.
> 
> >+class MobileConnection::DispatchEventAsyncTask : public nsRunnable
> ditto.
> For this case AsyncEventDispatcher(dom::EventTarget* aTarget, const
> nsAString& aEventType, bool aBubbles)
> would be useful constructor.

Nice to know this, thank you. :)
Will address this in next version.

Comment 16

[Edgar Chen [:edgar]]

Comment on attachment 8539148 [details] [diff] [review]
Patch, v3

Should address review comment #14 first.

Comment 17

[Edgar Chen [:edgar]]

Attachment: Patch, v4

Address review comment #14.
- Use AsyncEventDispatcher instead.

Comment 18

[Edgar Chen [:edgar]]

Attachment: Patch, v4

Sorry, attach correct one.

Comment 19

[Edgar Chen [:edgar]]

Comment on attachment 8539960 [details] [diff] [review]
Patch, v4

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

Address review comment #14:
- Use AsyncEventDispatcher instead.

Hi Hsinyi, Olli, may I have your review again? Thank you.

Comment 20

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

Comment on attachment 8539960 [details] [diff] [review]
Patch, v4

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

Looks good to me, thank you Edgar.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8539960 [details] [diff] [review]
Patch, v4

>+MobileConnection::UpdateIccId()
>+{
>+  nsAutoString iccId;
>+  if (mIcc) {
>+    nsCOMPtr<nsIIccInfo> iccInfo;
>+    mIcc->GetIccInfo(mClientId, getter_AddRefs(iccInfo));
>+    if (iccInfo) {
>+      iccInfo->GetIccid(iccId);
>+    } else {
>+      iccId.SetIsVoid(true);
>+    }
>+  } else {
>+    iccId.SetIsVoid(true);
>+  }

Maybe this could be written
  nsAutoString iccId;
  nsCOMPtr<nsIIccInfo> iccInfo;
  if (mIcc && NS_SUCCEEDED(mIcc->GetIccInfo(mClientId, getter_AddRefs(iccInfo))) &&
      iccInfo) {
    iccInfo->GetIccid(iccId);
  } else {
    iccId.SetIsVoid(true);
  }

...to avoid to call iccId.SetIsVoid(true); twice

Comment 22

[Edgar Chen [:edgar]]

Build fail in non-B2G platform
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=80133d80f638

Comment 23

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #22)
> Build fail in non-B2G platform
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=80133d80f638

It is because dom/icc is guarded by MOZ_B2G_RIL now, but MobileConnection is available for all platform. It seem we have to add MOZ_B2G_RIL protection in MobileConnection code for accessing ICC module for now. And they could be removed after ICC moves to ipdl (bug 864489).

Comment 24

[Edgar Chen [:edgar]]

Attachment: Patch, v5, r=hsinyi,smaug

- Address review comment #21
- Fix build error in non-b2g platform.

Comment 25

[Edgar Chen [:edgar]]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7981f461d80b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5855dee4494f

Comment 26

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8f1c3e86aa20

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8f1c3e86aa20