STK Profile Download from bug 791934 makes the Nexus S modem rebooting, effectively breaking the main functionnality of a phone. Commenting out the STK profile download in ril_worker.js worksaround the issue. Attached is the dump from the modem when it crashes and resets.

Hi Lissy I just try my latest m-c on Nexus S and it's still crashing even I commented out the profile download code. my last m-c commit is commit 40c9aec14b80a697dd3f2f368a5513d7d1b54a8d Author: Alex Keybl <akeybl@mozilla.com> Date: Mon Nov 19 12:03:27 2012 -0800 Merging in version bump NO BUG Can you show me your m-c version so I can give it a try ? Thanks

$ git describe --always master b373a99 And the change to ril_worker.js that makes me happy: diff --git a/dom/system/gonk/ril_worker.js b/dom/system/gonk/ril_worker.js index f14099b..314034b 100644 --- a/dom/system/gonk/ril_worker.js +++ b/dom/system/gonk/ril_worker.js @@ -3030,11 +3030,13 @@ let RIL = { if (newCardState == GECKO_CARDSTATE_READY) { // For type SIM, we need to check EF_phase first. // Other types of ICC we can send Terminal_Profile immediately. + /* if (this.appType == CARD_APPTYPE_SIM) { this.getICCPhase(); } else { this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE); } + */ this.fetchICCRecords(); this.reportStkServiceIsRunning(); }

sorry for my late reply I tried to reproduce it but I can't. I just try my latest gecko commit f29aa5cb0067b628348d53098224f811e8aa7771 Author: Ehsan Akhgari <ehsan@mozilla.com> Date: Wed Nov 28 17:13:43 2012 -0500 and gaia commit 7c3bd603b482676da3214e2a54a697e5089932bc Merge: 47823b5 2cc7345 Author: Timothy Guan-tin Chien <timdream@gmail.com> Date: Wed Nov 28 18:51:37 2012 -0800 and the nexus won't reboot now. Lissy, can you check again ?

I'll try as soon as I get the device back. Do not hesitate to ping me if I forget.

I was experiencing the same issues on SGS2. Workaround in Comment #2 worked for me.

Got my Nexus S back, flashed it with brand new code (completely wiped ccache, out/ and objdir-gecko), still reproducing. Gecko is at : $ git describe --always acf076f The workaround "fix" I proposed earlier still works.

According to TS 101.127 section 5.2 [http://www.etsi.org/deliver/etsi_ts/101200_101299/101267/08.18.00_60/ts_101267v081800p.pdf], encoding of the profile downloaded to SIM is composed of 19 bytes plus one subsequent byte at zero. in ril_consts.js, this.STK_SUPPORTED_TERMINAL_PROFILE is defined with 20 bytes, and sendStkTerminalProfile() has the following code: sendStkTerminalProfile: function sendStkTerminalProfile(profile) { Buf.newParcel(REQUEST_STK_SET_PROFILE); Buf.writeUint32(profile.length * 2); for (let i = 0; i < profile.length; i++) { GsmPDUHelper.writeHexOctet(profile[i]); } Buf.writeUint32(0); Buf.sendParcel(); }, So we copy the 20 bytes then add another one. Which makes 21 bytes to me. Commenting the last "Buf.writeUint32(0)" fixes the issue.

Looks like "fixed the issue" means in this case "makes no profile being downloaded, therefore worksaround the bug".

I did some testing, re-enabling stk profile download and below is the result. ----------------------------------------------------------------------------- Working diff : ----------------------------------------------------------------------------- diff --git a/dom/system/gonk/ril_consts.js b/dom/system/gonk/ril_consts.js index 653c6c0..a2151bc 100644 --- a/dom/system/gonk/ril_consts.js +++ b/dom/system/gonk/ril_consts.js @@ -893,6 +893,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [ 0x00, // Proactive Commands 0x00, // Proactive Commands 0x00, // Softkey support + /* 0x00, // Softkey information 0x00, // BIP proactive commands 0x00, // BIP supported bearers @@ -903,6 +904,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [ 0x00, // 18, RFU 0x00, // 19, RFU 0x00, // 20, RFU + */ ]; /** ----------------------------------------------------------------------------- Does not work : ----------------------------------------------------------------------------- diff --git a/dom/system/gonk/ril_consts.js b/dom/system/gonk/ril_consts.js index 653c6c0..99e1ba8 100644 --- a/dom/system/gonk/ril_consts.js +++ b/dom/system/gonk/ril_consts.js @@ -894,6 +894,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [ 0x00, // Proactive Commands 0x00, // Softkey support 0x00, // Softkey information + /* 0x00, // BIP proactive commands 0x00, // BIP supported bearers 0x00, // Screen height @@ -903,6 +904,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [ 0x00, // 18, RFU 0x00, // 19, RFU 0x00, // 20, RFU + */ ]; /**

One will notice that it works well with 10, and that it does not work when going to 11.

Some other results of logcat on the radio. I can't decide if in the second case we send the correct profile. But it seems we get the same answer in both case. Half-length profile (cut in the middle), Buf.writeUint32(profile.length * 2); E/RIL(s) ( 81): TX: (M)IPC_SAT_CMD (S)IPC_SAT_PROFILE_DOWNLOAD (T)IPC_CMD_SET (len:0x1C mseq:0x55 aseq:0x00) [ 01 30 42 45 33 46 46 33 46 31 46 30 30 30 30 30 30 ... ] E/RIL(s) ( 81): RX: (M)IPC_GEN_CMD (S)IPC_GEN_PHONE_RES (T)IPC_CMD_RESP (len:0x0C mseq:0xFF aseq:0x55) [ 0E 01 03 01 80 ] Full-length profile, Buf.writeUint32(profile.length * 4); E/RIL(s) ( 81): TX: (M)IPC_SAT_CMD (S)IPC_SAT_PROFILE_DOWNLOAD (T)IPC_CMD_SET (len:0x1C mseq:0x52 aseq:0x00) [ 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... ] E/RIL(s) ( 81): RX: (M)IPC_GEN_CMD (S)IPC_GEN_PHONE_RES (T)IPC_CMD_RESP (len:0x0C mseq:0xFF aseq:0x52) [ 0E 01 03 01 80 ]

And now a small dump of sent parcel in both cases: Full-length profile, Buf.writeUint32(profile.length * 4); I/Gecko ( 77): RIL Worker: New outgoing parcel of type 68 I/Gecko ( 77): RIL Worker: Outgoing parcel: 0,0,0,96,68,0,0,0,44,0,0,0,80,0,0,0,48,0,66,0,69,0,51,0,70,0,70,0,51,0,70,0,49,0,70,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,0,0,0,0 Half-length profile (cut in the middle), Buf.writeUint32(profile.length * 2); I/Gecko ( 78): RIL Worker: New outgoing parcel of type 68 I/Gecko ( 78): RIL Worker: Outgoing parcel: 0,0,0,56,68,0,0,0,44,0,0,0,20,0,0,0,48,0,66,0,69,0,51,0,70,0,70,0,51,0,70,0,49,0,70,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,0,0,0,0

Please note that (profile.length*4) seems to be consistent with bug 819790 from what I can infer in both cases.

(In reply to Alexandre LISSY :gerard-majax from comment #7) > According to TS 101.127 section 5.2 > [http://www.etsi.org/deliver/etsi_ts/101200_101299/101267/08.18.00_60/ > ts_101267v081800p.pdf], encoding of the profile downloaded to SIM is > composed of 19 bytes plus one subsequent byte at zero. > > in ril_consts.js, this.STK_SUPPORTED_TERMINAL_PROFILE is defined with 20 > bytes, and sendStkTerminalProfile() has the following code: > > sendStkTerminalProfile: function sendStkTerminalProfile(profile) { > Buf.newParcel(REQUEST_STK_SET_PROFILE); > Buf.writeUint32(profile.length * 2); This line writes 'String length' > for (let i = 0; i < profile.length; i++) { > GsmPDUHelper.writeHexOctet(profile[i]); > } > Buf.writeUint32(0); This line writes String delimeter.

(In reply to Alexandre LISSY :gerard-majax from comment #13) > Please note that (profile.length*4) seems to be consistent with bug 819790 > from what I can infer in both cases. Hi Lissy I think this bug and bug 819790 are different From Bug 819790, the length is read from GsmPDUHelper.readHexOctet so /3 is correct, we don't have to do /2 again (become /6 now) to get the length of octets. But in STK Profile Download, the length is written for string length. Each octet will be encoded to two chars, said 0xff -> 'ff' 1 byte -> two chars so the length should be * 2.

Ok, I was following another path that consisted of sending the command without encoding it with PDU format, and using profile.length as Parecel length. Yoshi saw me that I was wrong cause PDU format is a requierement of ril (as inferred from ril.h). This approach overcomes the crash problem with ril, but I saw that I was receiving this response after sending the Terminal Profile Download command: I/Gecko ( 1914): RIL Worker: New incoming parcel of size 12 I/Gecko ( 1914): RIL Worker: Parcel (size 12): 0,0,0,0,15,0,0,0,2,0,0,0 I/Gecko ( 1914): RIL Worker: We have at least one complete parcel. I/Gecko ( 1914): RIL Worker: Solicited response for request type 68, token 15, error 2 I'm investigating what that response means (error 2?). But I'm receiving the same response when I implement Alexandre approach (using profile.length * 4 with PDU encoding). Well, at least rild is not crashing anymore, but it looks like the terminal profile message that we are sending is not what Samsung's RIL is expecting.

I've also found this: https://android.googlesource.com/device/samsung/crespo/+/0e70776ea20206519aebf48e33483cb53cd452ab%5E!/ The interesting part is: [ Problem ] - When we power on Crespo with SKT 3G USIM inserted. Crespo does not recognize USIM and causing infinite sim reset process. [ SW Modification ] - We enabled major sim toolkit command like Send SMS, display text, refresh etc. at Terminal Profile which notifing Mobile capability to SIM. [ Reason ] - Basically, STK USIM is not following 3GPP 31.101 standard. - When STK USIM receive Terminal Profile which basic sim toolkit command disabled from Mobile, It is not returning any status word 1,2 whcih have the result of current command processed. Because mobile didn't received any sw1, sw2 from SIM, It is trying to recover this invalid situation causing Cold reset to SIM. after that same Terminal Profile fall down to SIM and no sw1, sw2 again. this is repeated.

For the STK_SET_PROFILE request, it seems Samsung RIL doesn't implement this (so the TERMINAL PROFILE is implemented in modem/baseband) Currently I found some builds of my Nexus-S will crash and keeps rebooting, but even I remove the those profile download related code, it's still crashing. So I am thinking maybe it's caused by other stuff.

I've updated the code a couple of minutes ago, and once stk profile download commented, no crash.

I'm also getting a hang on boot. This is my logcat: E/Profiler( 193): BEGIN mozilla_sampler_init E/Profiler( 193): ----------------- MOZ_PROFILER_NEW not set ----------------- E/Profiler( 193): Registering start signal E/GeckoConsole( 193): Could not read chrome manifest 'file:///system/b2g/chrome.manifest'. F/<unknown>( 80): stack corruption detected: aborted I/Gonk ( 76): OnDisconnect Note that "stack corruption detected: aborted". I can confirm the workaround in comment 2 enables my Nexus S to boot and the phone works.

Hi Vincent, The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S (Actually it doesn't work on otoro/unagi either) Could you add a RIL quirk for this?

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21) > Hi Vincent, > The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S > (Actually it doesn't work on otoro/unagi either) > > Could you add a RIL quirk for this? Sure! I will try adding it.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21) > Hi Vincent, > The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S > (Actually it doesn't work on otoro/unagi either) > > Could you add a RIL quirk for this? Hi Yoshi, Can you give some feedbacks for the WIP? Thanks!

Comment on attachment 741664 [details] [diff] [review] WIP V1 Review of attachment 741664 [details] [diff] [review]: ----------------------------------------------------------------- Good! Thank you :) ::: dom/system/gonk/ril_worker.js @@ +76,5 @@ > let RILQUIRKS_SIM_APP_STATE_EXTRA_FIELDS = libcutils.property_get("ro.moz.ril.simstate_extra_field", "false") === "true"; > // Needed for call-waiting on Peak device > let RILQUIRKS_EXTRA_UINT32_2ND_CALL = libcutils.property_get("ro.moz.ril.extra_int_2nd_call", "false") == "true"; > +let RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD = libcutils.property_get("ro.moz.ril.send_stk_profile_dl", "false") == "true"; > +// Ril quirk to Send STK Profile Download Comment on wrong line?

(In reply to Alexandre LISSY :gerard-majax from comment #19) > I've updated the code a couple of minutes ago, and once stk profile download > commented, no crash. Can you please add the patch and try again? Thanks

(In reply to Vincent Liu[:vliu] from comment #25) > Created attachment 741670 [details] [diff] [review] > WIP V2 > > (In reply to Alexandre LISSY :gerard-majax from comment #19) > > I've updated the code a couple of minutes ago, and once stk profile download > > commented, no crash. > > Can you please add the patch and try again? Thanks I'm sorry, I don't have the time right now :(

(In reply to Vincent Liu[:vliu] from comment #25) > Created attachment 741670 [details] [diff] [review] > WIP V2 > > (In reply to Alexandre LISSY :gerard-majax from comment #19) > > I've updated the code a couple of minutes ago, and once stk profile download > > commented, no crash. > > Can you please add the patch and try again? Thanks I've retried working on this bug and I can still confirm that changing the length to "(profile.length + 1) * 2" OR removing the leading '0' written fixes the crash. I'm currently in the process of cross checking on Inari, but I don't see why this would not be a proper fix.

Please find attached a patch that fixes the issue by setting the correct amount of bytes that we will be sending to the modem. As far as I could test, it does not breaks Inari device.

Hi Alexandre Can you explain why +1 is needed? The line Buf.writeUint32(profile.length * 2); is to write the string length. Each octet 0x12 will be represeneted '12' in hex format string. And '12' is string of length 2 So that's why *2 is needed. The following Buf.writeUint32(0) is to write string delimiter. It serves the same purpose as Buf.writeStringDelmiter(profile.length * 2) which is to write 4 octets of zero. Thanks

Also if you insist +1 could work on Nexus S, can you also check GET_STK_PROFILE could return the same profile you set when calling SET_STK_PROFILE?

Comment on attachment 741670 [details] [diff] [review] WIP V2 Actually this is working.

Can vliu or alexandre update the title in the patch? It seems this patch is malformed I cannot review it entirely on Bugzilla.

Same patch, reformated.

Comment on attachment 785710 [details] [diff] [review] Enable STK Profile Download on a per device basis Review of attachment 785710 [details] [diff] [review]: ----------------------------------------------------------------- This patch still looks weird to me, when I do the review, I only see line 93~95 are adding, but not the changes in line 3261, 3269, and 10762. Also the patch here doesn't contain 8 lines of context.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #34) > Comment on attachment 785710 [details] [diff] [review] > Enable STK Profile Download on a per device basis > > Review of attachment 785710 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch still looks weird to me, > when I do the review, > I only see line 93~95 are adding, > but not the changes in line 3261, 3269, and 10762. > > Also the patch here doesn't contain 8 lines of context. I'm getting the same behavior :( Yet it's just a git format-patch -1

Comment on attachment 788050 [details] [diff] [review] HG patch Review of attachment 788050 [details] [diff] [review]: ----------------------------------------------------------------- Hi, Lissy can you help to update this HG patch? And add r=me. ::: dom/system/gonk/ril_worker.js @@ +89,5 @@ > let RILQUIRKS_EXTRA_UINT32_2ND_CALL = libcutils.property_get("ro.moz.ril.extra_int_2nd_call", "false") == "true"; > // On the emulator we support querying the number of lock retries > let RILQUIRKS_HAVE_QUERY_ICC_LOCK_RETRY_COUNT = libcutils.property_get("ro.moz.ril.query_icc_count", "false") == "true"; > > +// Ril quirk to Send STK Profile Download trailing space @@ +3295,5 @@ > ICCRecordHelper.fetchICCRecords(); > } else if (this.appType == CARD_APPTYPE_USIM) { > + if (RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD) { > + this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE); > + } a Tab here. @@ +3300,5 @@ > ICCRecordHelper.fetchICCRecords(); > } else if (this.appType == CARD_APPTYPE_RUIM) { > + if (RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD) { > + this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE); > + } here to.

Comment on attachment 785710 [details] [diff] [review] Enable STK Profile Download on a per device basis Please try my hg patch.

Discussed with Lissy on irc, will assign to Georgia to land this patch.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #38) > Please try my hg patch. Tried this patch on my nexus-s and call functionality was properly restored. Please check this in!

I've also try the patch in comment #38, there's no aborted in log and device works fine. Gaia: 9ebfaca574dc70893494ef445880ff897a7cb03b Gecko: 6a9509a8d7d1fde2f6c77c6b459191683cc23dbe

Try result: https://tbpl.mozilla.org/?tree=Try&rev=9c888c64cbc5

https://hg.mozilla.org/integration/b2g-inbound/rev/8f9fac2ade0f https://hg.mozilla.org/integration/b2g-inbound/rev/0024361780c4

https://hg.mozilla.org/mozilla-central/rev/8f9fac2ade0f https://hg.mozilla.org/mozilla-central/rev/0024361780c4