744306 - B2G RIL: reponse id collision in Akami & RILv6

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

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]]

Hey Yoshi, since you have an Akami right now, could you take a look at this? Thanks!

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: 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]]

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch to prevent response id collision

Philikon's comment addressed.
Thanks, philikon :)

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: 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]]

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: Prevent response id collision v2

Address to philikon's comments. 
Thanks, philikon.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: 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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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 :)

Comment 11

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa9350d9ed9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19f6f1a4615

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/bfa9350d9ed9
https://hg.mozilla.org/mozilla-central/rev/a19f6f1a4615