Kai-Chih Hu [:khu] Reporter
	Description • 11 years ago

The operator variant logic in charge of customizing APNs, voicemail, cellbroadcast, and some messaging settings must be adapted to the DSDS feature.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 1 • 11 years ago

IMHO this bug must be part of v1.3. This bug is going to be a blocker for bug 928297 and bug 926372.

	Kai-Chih Hu [:khu] Reporter
	Comment 2 • 11 years ago

Sandip, what do you think?

	Ken Chang[:kenkai]
	Comment 3 • 11 years ago

Jose, can you please take this bug?

	Sandip Kamat
	Comment 4 • 11 years ago

(In reply to Kevin Hu [:khu] from comment #2)
> Sandip, what do you think?

I do not want to add risk for v1.3 scope defined in user story Google doc. I am okay for this to move to 1.4. Are there any downsides of doing that?

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 5 • 11 years ago

(In reply to Sandip Kamat from comment #4)
> (In reply to Kevin Hu [:khu] from comment #2)
> > Sandip, what do you think?
> 
> I do not want to add risk for v1.3 scope defined in user story Google doc. I
> am okay for this to move to 1.4. Are there any downsides of doing that?

Right now the APNs, voicemail, cellbroadcast, and some messaging settings are configured relying on the ICC card. If in a DSDS scenario the device has two different ICC card so how is it supposed to work in this new scenario? This bug is a must for v1.3. Data calls, voicemail service, both SMS and MMS services, cell broadcast services get affected since their settings won't be adapted if you guys decide to not take of care of this bug for v1.3. Actually the risk for v1.3 scope is to move it to 1.4.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 6 • 11 years ago

See coment #1, this is a blocker for bug 928297.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 7 • 11 years ago

Hsin-Yi, Arthur, Ken, I need you guys to confirm that this bug must be part of v1.3. There is a discussion about moving this bug to v1.4 and I would rise one more time this must to be done for v1.3. As you already know the `ril.data.apnSettings` setting is built by the system app on boot. Under this setting we store the array of APN settings for the ICC card on the device. Right now the operator variant logic builds an array of only one element as we only have one ICC card. This logic must change to build one more element with the APNs for this second ICC card (the APNs rely on the MCC and MNC codes in the ICC card). This one task that the operator variant logic perform, there are other ones related to voicemail, cellbroadcast and messaging settings.

Could you guys share your thoughts please? Thanks.

I hope this comment could help to understand how important this bug is and to set this bug as 1.3+.

	Ken Chang[:kenkai]
	Comment 8 • 11 years ago

This bugs is a customization. I agree that this bug is a critical bug for manufacturer and carrier. 
IMHO, this bug is a 1.3+ bug if it's also happened in single SIM project, Even this bug is a 1.3+ bug. But this bug isn't a blocker for any user story, I wonder if we have to fix this before FC date. 

Eventually, we still have to fix this bug . If you want to fix it early, it is fine for me to set this bug as 1.3+. However, the problem is when you expect to fix it. Do we have to fix this bug in current stage? Do we have resource for this bug now?

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

I agree with Ken on this. Thanks.

	Arthur Chen [:arthurcc][inactive after 6/30]
	Comment 10 • 11 years ago

I agree with Ken. Jose, thank you for taking the effort. Let me know if you need help.

	Kai-Chih Hu [:khu] Reporter
	Comment 11 • 11 years ago

Triaged today. We(Francis,Ken,Enpei,Wesley,me) think that it's reasonable to put this in 1.3. We agreed that this one won't block the 1.3 feature implementation. 

Joe, this looks like a bug in Comms team. Is it in your radar? Thanks.

	Noemí Freire (:noemi)
	Comment 12 • 11 years ago

Blocked by Bug 814637, clearing target milestone till WebIccManager api lands

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 13 • 11 years ago

WIP branch at https://github.com/jaoo/gaia/tree/933203

	Ken Chang[:kenkai]
	Comment 14 • 11 years ago

This bug is supposed to block the user story of data call.

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

I put two sim cards (chunghwa & twm) into fugu. chunghwa is in the 1st slot and twm in the 2nd. From lockscreen and 'settings-->cellular & data-->carrier' I saw 'chunghwa' as expected. However, I saw 'twm' in 'settings-->cellular & data-->data settings' so that I always got the apn settings for 'twm' sim cards. 

Per discussion with Arthur, we thought this weird behaviour seems caused by followings:
In icc_helper.js we don't check if iccIds[0] is coming from the 1st slot. iccIds[0] could be the card inserted into slot_2 but just getting ready first. 

Seems this issue is within the scope of this bug, so I commented here. :)
Just let me know if I should file a new bug for this.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 16 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #15)
> I put two sim cards (chunghwa & twm) into fugu. chunghwa is in the 1st slot
> and twm in the 2nd. From lockscreen and 'settings-->cellular &
> data-->carrier' I saw 'chunghwa' as expected. However, I saw 'twm' in
> 'settings-->cellular & data-->data settings' so that I always got the apn
> settings for 'twm' sim cards. 
> 
> Per discussion with Arthur, we thought this weird behaviour seems caused by
> followings:
> In icc_helper.js we don't check if iccIds[0] is coming from the 1st slot.
> iccIds[0] could be the card inserted into slot_2 but just getting ready
> first. 
> 
> Seems this issue is within the scope of this bug, so I commented here. :)
> Just let me know if I should file a new bug for this.

This bug (and bug 928297) will take of it. The IccHelper object was useful for a single ICC card scenario but not for DSDS scenario. I am stop using it in all this new DSDS bugs.

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #16)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #15)
> > I put two sim cards (chunghwa & twm) into fugu. chunghwa is in the 1st slot
> > and twm in the 2nd. From lockscreen and 'settings-->cellular &
> > data-->carrier' I saw 'chunghwa' as expected. However, I saw 'twm' in
> > 'settings-->cellular & data-->data settings' so that I always got the apn
> > settings for 'twm' sim cards. 
> > 
> > Per discussion with Arthur, we thought this weird behaviour seems caused by
> > followings:
> > In icc_helper.js we don't check if iccIds[0] is coming from the 1st slot.
> > iccIds[0] could be the card inserted into slot_2 but just getting ready
> > first. 
> > 
> > Seems this issue is within the scope of this bug, so I commented here. :)
> > Just let me know if I should file a new bug for this.
> 
> This bug (and bug 928297) will take of it. The IccHelper object was useful
> for a single ICC card scenario but not for DSDS scenario. I am stop using it
> in all this new DSDS bugs.

Got it. Thank you very much for handling this. :)

	Ken Chang[:kenkai]
	Comment 18 • 11 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #13)
> WIP branch at https://github.com/jaoo/gaia/tree/933203
Is it possible to have this before 12/6?

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 19 • 11 years ago

(In reply to Ken Chang from comment #18)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #13)
> > WIP branch at https://github.com/jaoo/gaia/tree/933203
> Is it possible to have this before 12/6?

Sure, I'm doing my best to request review for both bugs 928297 and this one today so the user could set the APNs for any of the ICC cards inserted in the multi ICC card device.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 20 • 11 years ago

I'm having good progress with the WIP patch while testing on the fugu device. I'll request review ASAP. Data calls for multi ICC card devices could be already tested by applying the patch from bug 928297 (see https://bugzilla.mozilla.org/show_bug.cgi?id=928297#c17).

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 22 • 11 years ago

With this patch the device sets up on boot the APN settings for the ICC cards in the devices, for all of them. The `ril.data.apnSettings` settings is now an array containing the APNs for the ICC cards.

We will handle only APNs settings in this bug so I'll change the title of the bug to make this clear.

I'd like to remark that I've been able to set up the APN settings for the two ICC cards only a few times. The fugu device is pretty unstable and lot of times the second ICC card never gets ready so we are not able to set up the APN settings relying on the MCC and MNC in the ICC card. The most important thing is that the changes introduced in this bug don't broke the functionality on sindle ICC devices.

Kaze, would you take a look at this patch please? Ping me on IRC if you have any doubt please.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 23 • 11 years ago

I'd like to make clear that we will take care of APN settings here. The other settings e.g. cellbroadcast, messaging settings and voicemail settings are not critical right now. I'll file bugs for taking care of them in a multi ICC card environment. For example, DSDS support has not been added to the mozCellbroadcast API so it makes no sense to work on cellbroadcast settings for a multi ICC card scenario.

	Fabien Cazenave [:kaze]
	Comment 24 • 11 years ago

Comment on attachment 8340139 [details] [diff] [review]
v2

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

LGTM

::: apps/system/js/operator_variant/operator_variant.js
@@ +101,5 @@
> +      var self = this;
> +      var xhr = new XMLHttpRequest();
> +      xhr.open('GET', OPERATOR_VARIANT_FILE, true);
> +      xhr.responseType = 'json';
> +      xhr.onreadystatechange = function() {

Nit: feel free to use xhr.onload instead.

@@ +259,3 @@
>        }
> +
> +      // Change property mane 'type' by 'types'.

Nit, typo: s/mane/name/

@@ +300,5 @@
> +
> +      var xhr = new XMLHttpRequest();
> +      xhr.open('GET', WAP_UA_PROFILE_FILE, true);
> +      xhr.responseType = 'json';
> +      xhr.onreadystatechange = function() {

Nit: feel free to use xhr.onload = function() instead.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 25 • 11 years ago

(In reply to Fabien Cazenave [:kaze] from comment #24)
> Comment on attachment 8340139 [details] [diff] [review]
> v2
> 
> Review of attachment 8340139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM

Thanks for the review Fabien!

HisinYi, the patch from this bug got r+ but I would like to raise some concerns. The fugu device is pretty unstable at ICC card detection on boot. Sometimes the ICC card 1 gets detected, sometimes the ICC card 2 gets detected, sometimes both. This is not because of the patch as I've tested. Having said that I'd like you to help me to take a decision on what to do (I mean land it).
The good thing is that this patch doesn't brake the functionality for single ICC card devices.

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #25)
> (In reply to Fabien Cazenave [:kaze] from comment #24)
> > Comment on attachment 8340139 [details] [diff] [review]
> > v2
> > 
> > Review of attachment 8340139 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > LGTM
> 
> Thanks for the review Fabien!
> 
> HisinYi, the patch from this bug got r+ but I would like to raise some
> concerns. The fugu device is pretty unstable at ICC card detection on boot.
> Sometimes the ICC card 1 gets detected, sometimes the ICC card 2 gets
> detected, sometimes both. This is not because of the patch as I've tested.
> Having said that I'd like you to help me to take a decision on what to do (I
> mean land it).

Hi José,

Thanks for raising the concern. I know you understand that the system team is working hard to stabilize fugu hardware so I don't share more progress here. :)

However, regarding the ICC detection problem, Edgar provided a patch on bug 944234 that should help. Maybe you'd like to apply to see if that helps you.

Back to your question, I think this bug covers the situation of only one card, two cards and no sim card. So if the patch reflects the situation that the system is detecting (rather than the exact number of cards inserted physically), then I think the patch is ready to go. For example, when the system tells gaia there's only one even there are two inserted physically, what we are expecting is the patch could react for the case of only one card inserted and one slot keeping empty. 

Does that make sense to you?

> The good thing is that this patch doesn't brake the functionality for single
> ICC card devices.

This is really good. :)

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 27 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #26)
> However, regarding the ICC detection problem, Edgar provided a patch on bug
> 944234 that should help. Maybe you'd like to apply to see if that helps you.

Oh, great. That is the issue I see. I'll give a try.

> Back to your question, I think this bug covers the situation of only one
> card, two cards and no sim card. So if the patch reflects the situation that
> the system is detecting (rather than the exact number of cards inserted
> physically), then I think the patch is ready to go. For example, when the
> system tells gaia there's only one even there are two inserted physically,
> what we are expecting is the patch could react for the case of only one card
> inserted and one slot keeping empty. 
> 
> Does that make sense to you?

The code is able to react to whatever the RIL plumbing is reporting and doesn't broke the functionality for single ICC card devices so once the test with Edgar's patch is done I'll land this patch. If we found any issue once the fugu device gets more stable we will file follow-up bugs to fix them.

Thanks for your feedback.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 28 • 11 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #27)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #26)
> > However, regarding the ICC detection problem, Edgar provided a patch on bug
> > 944234 that should help. Maybe you'd like to apply to see if that helps you.

The patch from bug 944234 saved my day. Now, if I apply that patch everything works as expected. On the down side I've noticed that unit test are failing and I need to fix them before landing this bug. The bad news is there are no mocks objects for the multi ICC card support for the mozIccManager/mozIcc objects and I need to create them :(. It will take more than expected.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 29 • 11 years ago

Fabien, I've added the test changes. Now they pass so this will be ready to land. You already reviewed this bug but I feel more comfortable if you take a look at the tests part. Sorry for bothering you. Thanks!

PS. I'll do the PR in parallel.

	Fabien Cazenave [:kaze]
	Comment 31 • 11 years ago

I see errors in Travis concerning mozSettings but they don’t seem to be related to this patch. I’d suggest to land this patch now if it helps working on bug 928297.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 32 • 11 years ago

gaia/master: https://github.com/mozilla-b2g/gaia/commit/71e2fcdaf3a6b86d11645ab1163e037179f591a3

	Jonathan Griffin (:jgriffin)
	Comment 33 • 11 years ago

This commit caused Marionette webapi tests to crash, so has been backed out with https://github.com/mozilla-b2g/gaia/commit/f6a5c9991765bc91511e552e175cb91e10efae89.  See bug 946178.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 34 • 11 years ago

(In reply to Jonathan Griffin (:jgriffin) from comment #33)
> This commit caused Marionette webapi tests to crash, so has been backed out
> with
> https://github.com/mozilla-b2g/gaia/commit/
> f6a5c9991765bc91511e552e175cb91e10efae89.  See bug 946178.

Sorry for the inconveniences, my apologies. How could I to shed some light here? I mean, in comment https://bugzilla.mozilla.org/show_bug.cgi?id=946178#c27 you mentioned you did local testing to figure out what the issue/the change causing the error is. How do you test this Marionette webapi tests to crash? Thanks!

	Jonathan Griffin (:jgriffin)
	Comment 35 • 11 years ago

If you have a local emulator build, you can run the webapi tests easily using ./mach marionette-webapi from the B2G directory.

I verified that the tests crash when the emulator is built with 71e2fcdaf3a6b86d11645ab1163e037179f591a3, but they don't crash when the emulator is built with the previous gaia commit, f9901db71b8e5a70da1700ae82b66231bc000660.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 36 • 11 years ago

Thanks Jonathan!

Hsin-Yi, It seems this patch causes marionette webapi test to crash and It was backed out (see bug 946178 please). I'm trying to figure out what is going on but at a first glance I don't see how this patch is causing the failures, could you shed some light here please? Thanks.

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #36)
> Thanks Jonathan!
> 
> Hsin-Yi, It seems this patch causes marionette webapi test to crash and It
> was backed out (see bug 946178 please). I'm trying to figure out what is
> going on but at a first glance I don't see how this patch is causing the
> failures, could you shed some light here please? Thanks.

José,

I looked at the log but didn't see something obvious. :( 
Much more investigation is required on my side as well ...

CC more people. Hopefully we get more clues.

	Edgar Chen [:edgar]
	Comment 38 • 11 years ago

I can reproduce this fail with the patch, below is the backtrace from gdb,

Program received signal SIGSEGV, Segmentation fault.
0x4126cc34 in mozilla::dom::network::MobileConnection::Listener::NotifyRadioStateChanged (this=<value optimized out>)
    at /data/mercurial/mozilla-central/dom/network/src/MobileConnection.cpp:33
33	  NS_FORWARD_SAFE_NSIMOBILECONNECTIONLISTENER(mMobileConnection)
(gdb) bt
#0  0x4126cc34 in mozilla::dom::network::MobileConnection::Listener::NotifyRadioStateChanged (this=<value optimized out>)
    at /data/mercurial/mozilla-central/dom/network/src/MobileConnection.cpp:33
#1  0x40c9c6d2 in NS_InvokeByIndex (that=<value optimized out>, 
    methodIndex=<value optimized out>, paramCount=<value optimized out>, 
    params=<value optimized out>)
    at /data/mercurial/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:164
#2  0x411ab690 in CallMethodHelper::Invoke (ccx=<value optimized out>, 
    mode=<value optimized out>)
    at /data/mercurial/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2567
#3  CallMethodHelper::Call (ccx=<value optimized out>, 
    mode=<value optimized out>)
    at /data/mercurial/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1907
#4  XPCWrappedNative::CallMethod (ccx=<value optimized out>, 
    mode=<value optimized out>)
    at /data/mercurial/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1873
#5  0x411aba2e in XPC_WN_CallMethod (cx=0x40220270, argc=0, 
    vp=<value optimized out>)
    at /data/mercurial/mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cp---Type <return> to continue, or q <return> ---Type <return> to continue, or q <return> to quit---

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

(In reply to Edgar Chen [:edgar][:echen] from comment #38)
> I can reproduce this fail with the patch, below is the backtrace from gdb,
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x4126cc34 in
> mozilla::dom::network::MobileConnection::Listener::NotifyRadioStateChanged
> (this=<value optimized out>)
>     at
> /data/mercurial/mozilla-central/dom/network/src/MobileConnection.cpp:33
> 33	  NS_FORWARD_SAFE_NSIMOBILECONNECTIONLISTENER(mMobileConnection)
> (gdb) bt
> #0  0x4126cc34 in
> mozilla::dom::network::MobileConnection::Listener::NotifyRadioStateChanged
> (this=<value optimized out>)
>     at
> /data/mercurial/mozilla-central/dom/network/src/MobileConnection.cpp:33
> #1  0x40c9c6d2 in NS_InvokeByIndex (that=<value optimized out>, 
>     methodIndex=<value optimized out>, paramCount=<value optimized out>, 
>     params=<value optimized out>)
>     at
> /data/mercurial/mozilla-central/xpcom/reflect/xptcall/src/md/unix/
> xptcinvoke_arm.cpp:164
> #2  0x411ab690 in CallMethodHelper::Invoke (ccx=<value optimized out>, 
>     mode=<value optimized out>)
>     at
> /data/mercurial/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2567
> #3  CallMethodHelper::Call (ccx=<value optimized out>, 
>     mode=<value optimized out>)
>     at
> /data/mercurial/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1907
> #4  XPCWrappedNative::CallMethod (ccx=<value optimized out>, 
>     mode=<value optimized out>)
>     at
> /data/mercurial/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1873
> #5  0x411aba2e in XPC_WN_CallMethod (cx=0x40220270, argc=0, 
>     vp=<value optimized out>)
>     at
> /data/mercurial/mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cp---
> Type <return> to continue, or q <return> ---Type <return> to continue, or q
> <return> to quit---

A guess:
[1] it's likely to have null mProvider (see constructor code). If that's the case, mListener->Disconnect() is never called that might cause some problems in the next run.

I am going to rewrite the code as follows and give it a try.

  if (mListener) {
    mListener->Disconnect();

    if (mProvider) {
      mProvider->UnregisterMobileConnectionMsg(mClientId, mListener);
      mProvider = nullptr;
    }
  
    mListener = nullptr;
  }


[1] http://dxr.mozilla.org/mozilla-central/source/dom/network/src/MobileConnection.cpp?from=MobileConnection.cpp#118

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

another gdb bt:

Program received signal SIGSEGV, Segmentation fault.
XPCWrappedNativeProto::JSProtoObjectFinalized (this=0x1, fop=0x442e4540, obj=0xbe86a7e0)
    at ../../../../gecko/js/xpconnect/src/XPCWrappedNativeProto.cpp:137
137	    ClassInfo2WrappedNativeProtoMap* map = GetScope()->GetWrappedNativeProtoMap();
(gdb) bt
#0  XPCWrappedNativeProto::JSProtoObjectFinalized (this=0x1, fop=0x442e4540, 
    obj=0xbe86a7e0) at ../../../../gecko/js/xpconnect/src/XPCWrappedNativeProto.cpp:137
#1  0x4128493a in mozilla::dom::network::MobileConnection::Listener::NotifyRadioStateChanged (this=<optimized out>) at ../../../../gecko/dom/network/src/MobileConnection.cpp:33
#2  0x40cf5c86 in NS_InvokeByIndex (that=<optimized out>, methodIndex=11, 
    paramCount=<optimized out>, params=0xbe86a6f8)
    at ../../../../../../../gecko/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:164
#3  0x411ceac2 in Invoke (this=0xbe86a6d0)
    at ../../../../gecko/js/xpconnect/src/XPCWrappedNative.cpp:2567
#4  Call (this=0xbe86a6d0)
    at ../../../../gecko/js/xpconnect/src/XPCWrappedNative.cpp:1907
#5  XPCWrappedNative::CallMethod (ccx=<optimized out>, mode=<optimized out>)
    at ../../../../gecko/js/xpconnect/src/XPCWrappedNative.cpp:1873
#6  0x411cef1e in XPC_WN_CallMethod (cx=0x40423270, argc=0, vp=0xbe86ab48)
    at ../../../../gecko/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1301
#7  0x41afaa50 in CallJSNative (args=..., 
    native=0x411cee29 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, 
    cx=0x40423270) at ../../../gecko/js/src/jscntxtinlines.h:220
#8  js::Invoke (cx=0x40423270, args=..., construct=js::NO_CONSTRUCT)
    at ../../../gecko/js/src/vm/Interpreter.cpp:463
#9  0x41a951d6 in js_fun_call (cx=0x40423270, argc=<optimized out>, vp=0x43b18130)
    at ../../../gecko/js/src/jsfun.cpp:892
#10 0x41a95b5e in js_fun_apply (cx=0x40423270, argc=<optimized out>, vp=0x43b18130)

(gdb) p map
$1 = (ClassInfo2WrappedNativeProtoMap *) 0xb
(gdb)

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

My try on comment 39 fails. :(

	Edgar Chen [:edgar]
	Comment 42 • 11 years ago

I think it is related to cycle collector setup of MobileConnectionArray [1].
The UNLINK process may trigger releasing MobileConnection without calling Shutdown().

There are two possible solution:
1). Calling Shutdown() in MobileConnectionArray UNLINK process.
2). Calling Shutdown() in MobileConnection destructor.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/network/src/MobileConnectionArray.cpp#13

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 43 • 11 years ago

Hsin-Yi, Edgar, thank you guys for being investigating this issue. I'm wondering if it would be better to file a new bug for the underlying issue and make this bug to depend on the new one. Thoughts?

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

Yup, agree with Edgar, seems problems with cycle collector. I set break point in [1] and MobileConnection::Shutdown [2]. [1] is reached right before crash without [2]. With the WIP patch, I don't see crash anymore.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/network/src/MobileConnection.cpp#54

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

Comment on attachment 8343079 [details] [diff] [review]
0001-Bug-933203.patch - WIP - fix crash

Oops, wrong file.

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

Shutdown() in 
1) MobileConnection unlink
2) MobileConnection destructor

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #43)
> Hsin-Yi, Edgar, thank you guys for being investigating this issue. I'm
> wondering if it would be better to file a new bug for the underlying issue
> and make this bug to depend on the new one. Thoughts?

Hi José,

Having another bug makes the bug context clear. I'll do that!  The WIP was attached here to help you verify the crash is done. :)

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #49)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #43)
> > Hsin-Yi, Edgar, thank you guys for being investigating this issue. I'm
> > wondering if it would be better to file a new bug for the underlying issue
> > and make this bug to depend on the new one. Thoughts?
> 
> Hi José,
> 
> Having another bug makes the bug context clear. I'll do that!  The WIP was
> attached here to help you verify the crash is done. :)

This crash issue has been addressed in bug 946437.

	Ken Chang[:kenkai]
	Comment 51 • 11 years ago

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #50)
> > Hi José,
> > 
> > Having another bug makes the bug context clear. I'll do that!  The WIP was
> > attached here to help you verify the crash is done. :)
> 
> This crash issue has been addressed in bug 946437.
Hi José, could you please check-in your patch?

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 52 • 11 years ago

(In reply to Ken Chang[:ken] from comment #51)
> Hi José, could you please check-in your patch?

We cannot land this patch again until bug 946437 lands first.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 53 • 11 years ago

Fabrice, we are waiting bug 946437 to land before re-landing this bug again. Would you mind to take a look at the patch attached here please?. If you are OK with it, I'll re-land this path as soon as bug 946437 gets landed and Travis goes green and tree is open. Thanks.

	[:fabrice] Fabrice Desré
	Comment 54 • 11 years ago

I can't take a decision base on the PR only, especially with so many changes. Please help me with the following information:
- is the code fairly isolated? ("I want to refactor the RIL worker"
would not pass).
- has this code caused churn or pain before?
- is it easy to turn the feature off?
- do we have good test coverage?
- do we have good QA coverage, ie in smoketests?
- do we have strong partner expectations for the feature?

	Kai-Chih Hu [:khu] Reporter
	Comment 55 • 11 years ago

We really need to know when the bug can be landed, because DSDS is a committed feature. Can you help to provide the ETA? Thank you.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 56 • 11 years ago

(In reply to Fabrice Desré [:fabrice] from comment #54)
> - is the code fairly isolated? ("I want to refactor the RIL worker"
> would not pass).

More or less, It touches both system and setting apps. The code is in charge of auto selecting the APNs on boot and allowing to modify them later by the user.

> - has this code caused churn or pain before?

Not specially, just a few changes motivated by the new use cases.

> - is it easy to turn the feature off?

We can just back it out to turn it off.

> - do we have good test coverage?

Yes, living them in the system app.

> - do we have good QA coverage, ie in smoketests?

No idea, ni? Enpei.

> - do we have strong partner expectations for the feature?

Kevin to provide an answer here. Requesting ni? then.

	Kai-Chih Hu [:khu] Reporter
	Comment 57 • 11 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #56)
> (In reply to Fabrice Desré [:fabrice] from comment #54)
> 
> > - do we have strong partner expectations for the feature?
> 
> Kevin to provide an answer here. Requesting ni? then.
Not from my side. I did not hear anything about this from partners. Maybe Sandip has some comments on this. I would suggest this to 1.4, if we don't have strongly desired voices from partners.

	Enpei
	Comment 58 • 11 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #56)
> > - do we have good QA coverage, ie in smoketests?
> 
> No idea, ni? Enpei.
DSDS APN test alreay has test cases in moztrap.

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

(In reply to Kevin Hu [:khu] from comment #57)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #56)
> > (In reply to Fabrice Desré [:fabrice] from comment #54)
> > 
> > > - do we have strong partner expectations for the feature?
> > 
> > Kevin to provide an answer here. Requesting ni? then.
> Not from my side. I did not hear anything about this from partners. Maybe
> Sandip has some comments on this. I would suggest this to 1.4, if we don't
> have strongly desired voices from partners.

This blocks several committed user stories. Without this, we are unable to setup a data call or send MMS with the second SIM.

	Kai-Chih Hu [:khu] Reporter
	Comment 60 • 11 years ago

I thought that this is used to adjust the APN setting automatically. Without this, we should be able to do it manually. But, after confirming with Hsinyi and realized that, even via Gaia, we can't configure the APN setting for 2nd SIM manually. So, this is a blocker for sure(for 2nd SIM).

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 61 • 11 years ago

Adding link for the new PR since the previous one is closed after being the patch backed out.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 62 • 11 years ago

Fabrice, I replied to your request at comment #56. Could you take a look at it please? Thanks!

	[:fabrice] Fabrice Desré
	Comment 63 • 11 years ago

Approval from me to land on 1.3, thanks.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 64 • 11 years ago

Landed on Gaia master branch. Last time it was backed out this time everything is fine, apparently. Finger crossed.

https://github.com/mozilla-b2g/gaia/commit/4dd40d64f137c48104d5f39e48c2ccb0ce9ab78c

	Anthony Ricaud (:rik)
	Comment 65 • 11 years ago

I had to back this out because of a unit test failure.

https://github.com/mozilla-b2g/gaia/commit/6415b8b44068596404c10365394544e94edd5ce5

[system] system/LockScreenConnInfoManager > Single sim devices "before each" hook:
TypeError: MockNavigatorMozIccManager.addIcc is not a function

	Zac C (:zac)
	Comment 66 • 11 years ago

Rik thanks, this was causing UI test failures too but we hadn't quite got to the source!

	Anthony Ricaud (:rik)
	Comment 67 • 11 years ago

Zac: Do you have some logs for those failures so that José can look at them?

	Zac C (:zac)
	Comment 68 • 11 years ago

Rik, yes Askeing filed:

https://bugzilla.mozilla.org/show_bug.cgi?id=948852

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 71 • 11 years ago

Fabien, the test in the last patch caused unit test failures in other tests that were using a mock object for the mozIccManager API. This was because the one I added was causing conflicts with the one already landed and it results in a mess. Now the tests in this patch are using the existing mock object for the mozIccManager API already landed in master. Everything seems to be right and I don't see any unit test failures.

I would need a quick review of the patch since we need to land this ASAP.

Thanks!

	Fabien Cazenave [:kaze]
	Comment 72 • 11 years ago

I’m afraid the tree is closed for now (NPM issues). I’m reviewing your patch, but we’ll have to wait for Travis results before giving an r+.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 73 • 11 years ago

All green now.

	Fabien Cazenave [:kaze]
	Comment 74 • 11 years ago

Comment on attachment 8346004 [details] [diff] [review]
v4

LGTM. Crossing my fingers…

As discussed on IRC, and for the record: this is a good example why I prefer to deal with multiple commits in a pull request. It makes it much easier to review successive versions of a patch.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 75 • 11 years ago

Travis goes green with the PR and everything seems to work fine for both peak and fugu devices. I hope the patch doesn't broke anything.

https://github.com/mozilla-b2g/gaia/commit/53b010647062b4e2637f98ac3538af1c7bfe723a

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 76 • 11 years ago

Travis went green also after landing the patch. See https://travis-ci.org/mozilla-b2g/gaia/builds/15387670.

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #76)
> Travis went green also after landing the patch. See
> https://travis-ci.org/mozilla-b2g/gaia/builds/15387670.

\o/

	Ryan VanderMeulen [:RyanVM]
	Comment 78 • 11 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #75)
> https://github.com/mozilla-b2g/gaia/commit/
> 53b010647062b4e2637f98ac3538af1c7bfe723a

John, can you please assist with the uplift to v1.3? :)

	John Ford [:jhford] CET/CEST Berlin Time
	Comment 79 • 11 years ago

[v1.3 a99ca07] Merge pull request #14579 from jaoo/933203

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 80 • 11 years ago

(In reply to John Ford [:jhford] -- please use 'needinfo?' instead of a CC from comment #79)
> [v1.3 a99ca07] Merge pull request #14579 from jaoo/933203

Hey John, you didn't uplift this patch correctly. You had to uplift https://github.com/mozilla-b2g/gaia/commit/53b010647062b4e2637f98ac3538af1c7bfe723a, see https://github.com/mozilla-b2g/gaia/pull/14579 please.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 81 • 11 years ago

I don't see commit a99ca07 anywhere.

	John Ford [:jhford] CET/CEST Berlin Time
	Comment 82 • 11 years ago

weird.  I've redone the cherry-pick.

[v1.3 1b432b0] Merge pull request #14579 from jaoo/933203

$ git push origin v1.3
Counting objects: 161, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (20/20), done.
Writing objects: 100% (20/20), 2.12 KiB | 0 bytes/s, done.
Total 20 (delta 15), reused 0 (delta 0)
To git@github.com:mozilla-b2g/gaia.git
   1594923..1b432b0  v1.3 -> v1.3