Last Comment Bug 744306 - B2G RIL: reponse id collision in Akami & RILv6
: B2G RIL: reponse id collision in Akami & RILv6
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Yoshi Huang[:allstars.chh]
:
Mentors:
Depends on:
Blocks: 726233 740238
  Show dependency treegraph
 
Reported: 2012-04-10 20:44 PDT by Vicamo Yang [:vicamo][:vyang]
Modified: 2012-04-13 03:57 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to prevent response id collision (1.57 KB, patch)
2012-04-11 01:35 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
Details | Diff | Review
Patch to prevent response id collision (1.58 KB, patch)
2012-04-11 01:58 PDT, Yoshi Huang[:allstars.chh]
no flags Details | Diff | Review
Part 2: Set RILQUIRKS_V5_LEGACY to false on akami (1.88 KB, patch)
2012-04-11 03:23 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
Details | Diff | Review
Part 1: Prevent response id collision v2 (1.58 KB, patch)
2012-04-12 03:09 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
Details | Diff | Review
Part 2: Set RILQUIRKS_V5_LEGACY to false on akami v2 (1.90 KB, patch)
2012-04-12 03:11 PDT, Yoshi Huang[:allstars.chh]
philipp: review+
Details | Diff | Review

Description Vicamo Yang [:vicamo][:vyang] 2012-04-10 20:44:18 PDT
Excerpted from bug 740238 comment 8:

> Full log is available in http://pastebin.com/bTziLNhr . The log is captured
> with gecko built WITHOUT attachment 611760 [details] [diff] [review].
> 
> Line 124: rild crash at boot.
> Line 663: Parcel (size 8): 1,0,0,0,10,4,0,0
> Line 666: got UNSOLICITED_RIL_CONNECTED(1034), but it is actually
>           UNSOLICITED_VOICE_RADIO_TECH_CHANGED(also 1034) for Akami. Since
>           all payload was already consumed, parcel guard throws an
>           exception in next read attempt.
> Line 1035: got 'sendSMS' dom message from RadioInterfaceLayer.
> Line 1036: sending REQUEST_GET_SMSC_ADDRESS(100)
> Line 1043: got response of REQUEST_GET_SMSC_ADDRESS(100) with error
>            ERROR_GENERIC_FAILURE(2)
Comment 1 Philipp von Weitershausen [:philikon] 2012-04-10 20:45:45 PDT
Hey Yoshi, since you have an Akami right now, could you take a look at this? Thanks!
Comment 2 Yoshi Huang[:allstars.chh] 2012-04-10 20:46:25 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> Hey Yoshi, since you have an Akami right now, could you take a look at this?
> Thanks!

Sure !
Comment 3 Yoshi Huang[:allstars.chh] 2012-04-11 01:35:17 PDT
Created attachment 613904 [details] [diff] [review]
Patch to prevent response id collision

I found the headers for ril can be found in [1]
Also I found the collision has been fixed in their ics branch. [2]
So instead to write code like  
if (model_id === "??" && version === "??") .... something like that, 

I just add a simple check for the length.

The long term solution may be we generate the ril_consts.js according to the target we did in 'make config-XXX' to get the corresponding ril.h in the build time, like what Android did in RILConstants.java 

thanks


[1] :https://www.codeaurora.org/gitweb/quic/la/?p=platform/hardware/ril.git;a=summary
[2]: http://bit.ly/Ixx0J7
Comment 4 Philipp von Weitershausen [:philikon] 2012-04-11 01:47:58 PDT
Comment on attachment 613904 [details] [diff] [review]
Patch to prevent response id collision

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

::: dom/system/gonk/ril_worker.js
@@ +2430,5 @@
>  RIL[UNSOLICITED_OEM_HOOK_RAW] = null;
>  RIL[UNSOLICITED_RINGBACK_TONE] = null;
>  RIL[UNSOLICITED_RESEND_INCALL_MUTE] = null;
>  RIL[UNSOLICITED_RIL_CONNECTED] = function UNSOLICITED_RIL_CONNECTED(length) {
> +  // To prevent responsed id collision between UNSOLICITED_RIL_CONNECTED and

s/To p/P/
s/responsed/response/

@@ +2432,5 @@
>  RIL[UNSOLICITED_RESEND_INCALL_MUTE] = null;
>  RIL[UNSOLICITED_RIL_CONNECTED] = function UNSOLICITED_RIL_CONNECTED(length) {
> +  // To prevent responsed id collision between UNSOLICITED_RIL_CONNECTED and
> +  // UNSOLICITED_VOICE_RADIO_TECH_CHANGED for Akami on gingerbread branch.
> +  if (length === 0) {

Could just be `if (!length)`
Comment 5 Yoshi Huang[:allstars.chh] 2012-04-11 01:58:04 PDT
Created attachment 613915 [details] [diff] [review]
Patch to prevent response id collision

Philikon's comment addressed.
Thanks, philikon :)
Comment 6 Yoshi Huang[:allstars.chh] 2012-04-11 03:23:59 PDT
Created attachment 613938 [details] [diff] [review]
Part 2: Set RILQUIRKS_V5_LEGACY to false on akami

I take the WIP from Bug 714973, thanks to jaoo.
and call this initRILQuirks in RIL_CONNECTED handler(which should be UNSOLICITED_VOICE_RADIO_TECH_CHANGED handler on akami).
I found this response (UNSOLICITED_VOICE_RADIO_TECH_CHANGED) comes just after SIGNAL_STRENGTH and RADIO_STATE_CHANGED, so call initRILQuirks here so RILQUIRKS_V5_LEGACY should already be set before any code needs it.

thanks
Comment 7 Philipp von Weitershausen [:philikon] 2012-04-12 01:37:45 PDT
Comment on attachment 613938 [details] [diff] [review]
Part 2: Set RILQUIRKS_V5_LEGACY to false on akami

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

::: dom/system/gonk/ril_worker.js
@@ +2439,5 @@
>    if (!length) {
> +    this.initRILQuirks();
> +    if (DEBUG) {
> +      debug("RILQUIRKS_V5_LEGACY is " + RILQUIRKS_V5_LEGACY);
> +    }

Please move this debug statement into initRILQuirks(), akin to the other ones ("Detected ..., enabling ..."). r=me with that!
Comment 8 Yoshi Huang[:allstars.chh] 2012-04-12 03:09:05 PDT
Created attachment 614320 [details] [diff] [review]
Part 1: Prevent response id collision v2

Address to philikon's comments. 
Thanks, philikon.
Comment 9 Yoshi Huang[:allstars.chh] 2012-04-12 03:11:02 PDT
Created attachment 614321 [details] [diff] [review]
Part 2: Set RILQUIRKS_V5_LEGACY to false on akami v2

Address to philikon's coments.
Since in the patch I set RILQUIRKS_V5_LEGACY to false, 
so I change to debug message to "disabling", instead of "enabling".

thanks
Comment 10 Yoshi Huang[:allstars.chh] 2012-04-12 03:40:13 PDT
(In reply to Yoshi Huang[:yoshi] from comment #9)
> Created attachment 614321 [details] [diff] [review]
> Part 2: Set RILQUIRKS_V5_LEGACY to false on akami v2
Part 2 uses code from jaoo's patch in Bug714973 to detect ril implementation.

Thanks jaoo :)

Note You need to log in before you can comment on or make changes to this bug.