Closed Bug 1087847 Opened 5 years ago Closed 5 years ago

mozIccManager reports 0 SIMs, but mozMobileConnection does

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(2 files, 5 obsolete files)

Attached file 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
See Also: → 864489
Blocks: 843452
Blocks: 1084076
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.
Attached patch Patch, v1 (obsolete) — Splinter Review
Do we have any news about this? Are you asking for review, Edgar?
Flags: needinfo?(echen)
(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.
Flags: needinfo?(echen)
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
Flags: needinfo?(echen)
(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.
Flags: needinfo?(echen) → needinfo?(marina.rodriguez.iglesias)
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
Flags: needinfo?(marina.rodriguez.iglesias)
Attached patch Patch, v2 (obsolete) — Splinter Review
Attachment #8522697 - Attachment is obsolete: true
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.
Attachment #8529406 - Flags: feedback?(htsai)
(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 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)
Attachment #8529406 - Flags: feedback?(htsai) → feedback+
Attached patch Patch, v3 (obsolete) — Splinter Review
Address comment #11.
Attachment #8529406 - Attachment is obsolete: true
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.
Attachment #8539148 - Flags: review?(htsai)
Attachment #8539148 - Flags: review?(bugs)
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.
Attachment #8539148 - Flags: review?(bugs) → review-
(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 on attachment 8539148 [details] [diff] [review]
Patch, v3

Should address review comment #14 first.
Attachment #8539148 - Flags: review?(htsai)
Attached patch Patch, v4 (obsolete) — Splinter Review
Address review comment #14.
- Use AsyncEventDispatcher instead.
Attachment #8539148 - Attachment is obsolete: true
Attached patch Patch, v4 (obsolete) — Splinter Review
Sorry, attach correct one.
Attachment #8539956 - Attachment is obsolete: true
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.
Attachment #8539960 - Flags: review?(htsai)
Attachment #8539960 - Flags: review?(bugs)
Comment on attachment 8539960 [details] [diff] [review]
Patch, v4

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

Looks good to me, thank you Edgar.
Attachment #8539960 - Flags: review?(htsai) → review+
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
Attachment #8539960 - Flags: review?(bugs) → review+
(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).
- Address review comment #21
- Fix build error in non-b2g platform.
Attachment #8539960 - Attachment is obsolete: true
Attachment #8541437 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8f1c3e86aa20
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1116431
You need to log in before you can comment on or make changes to this bug.