986440 - [RIL] Ril worker leaves JS warnings in logcat

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

These warnings should be fixed.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 986440: Return 'null' from RIL-worker function

Fixed the warning

> E/GeckoConsole(  569): [JavaScript Warning: "TypeError: anonymous function does not always return a value" {file: "resource://gre/modules/ril_worker.js" line: 3440 column: 4 source: "    return info;

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler

Fixes the warning

> E/GeckoConsole(  178): [JavaScript Warning: "ReferenceError: reference to undefined property options.rilMessageType" {file: "resource://gre/modules/ril_worker.js" line: 6258}]

I think the code was simply copied from |getSmscAddress|.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 986440: Check for |radioTech| in |voiceRegistrationState|

I don't have the warning message any longer, but it happened because we're handling REQUEST_SIGNAL_STRENGTH before REQUEST_VOICE_REGISTRATION_STATE. The former needs |radioTech|, which is first set by the latter.

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS

This fixes a warning in ril_worker.js' function |queryCallForwardStatus| about |number| not being defined. It looks like the interface RilContentHelper.getCallForwardingOption would need an extra parameter to set the number.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8394735 [details] [diff] [review]
[01] Bug 986440: Return 'null' from RIL-worker function

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

Thanks for catching this.

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8394737 [details] [diff] [review]
[02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler

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

rilMessageType isn't coming from Rild but from Content Process instead. rilMessageType is used for correctly delivering callback.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8394740 [details] [diff] [review]
[03] Bug 986440: Check for |radioTech| in |voiceRegistrationState|

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

::: dom/system/gonk/ril_worker.js
@@ +3454,5 @@
>  
> +    // During startup, |radioTech| is not yet defined, so we need to
> +    // check it separately
> +    if (('radioTech' in this.voiceRegistrationState) &&
> +        !this._isGsmTechGroup(this.voiceRegistrationState.radioTech)) {

As [1] checks the validity of radioTech, it should be fine here.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#4193

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8394751 [details] [diff] [review]
[04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS

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

::: dom/system/gonk/RILContentHelper.js
@@ +1229,5 @@
>        clientId: clientId,
>        data: {
>          requestId: requestId,
> +        reason: reason,
> +        number: null

Thanks for catching this. Since the API defines "reason" is the only parameter, I think we shouldn't do this here.

Instead, let's do /s/options.number/options.number || ""/ in [1]

[1] http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/ril_worker.js#l2560

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v2)

Changes since v1:

  - unconditionally set |rilMessageType| in response handler

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8394751 [details] [diff] [review]
[04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS

It's not enough. The warning message in the logcat is

> E/GeckoConsole(  180): [JavaScript Warning: "ReferenceError: reference to undefined property this.voiceRegistrationState.radioTech" {file: "resource://gre/modules/ril_worker.js" line: 3455}]

While this is not strictly an error, it's still a warning that I think should be fixed; if only to not distract from the real errors. Please reconsider.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> Comment on attachment 8394751 [details] [diff] [review]
> [04] Bug 986440: Add missing field |number| for
> REQUEST_QUERY_CALL_FORWARD_STATUS
> 
> It's not enough. The warning message in the logcat is
> 
> > E/GeckoConsole(  180): [JavaScript Warning: "ReferenceError: reference to undefined property this.voiceRegistrationState.radioTech" {file: "resource://gre/modules/ril_worker.js" line: 3455}]
> 
> While this is not strictly an error, it's still a warning that I think
> should be fixed; if only to not distract from the real errors. Please
> reconsider.

Sorry, I clicked on the wrong attachment. This comment is meant for patch [03], not [04].

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8394740 [details] [diff] [review]
[03] Bug 986440: Check for |radioTech| in |voiceRegistrationState|

Please see my comment above and reconsider the patch.

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8396971 [details] [diff] [review]
[02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v2)

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

::: dom/system/gonk/ril_worker.js
@@ +6188,5 @@
>  };
>  RilObject.prototype[REQUEST_GET_SMSC_ADDRESS] = function REQUEST_GET_SMSC_ADDRESS(length, options) {
>    this.SMSC = options.rilRequestError ? null : this.context.Buf.readString();
>  
> +  options.rilMessageType = 'getSmscAddress';

Oh, sorry that I didn't say that clearly enough. :(

I don't think we need to set this here.
this.getSmscAddress() is called internally in ril_worker.js or called by Content process, i.e. RILContentHelper.

If it's the former case, then there is neither 'options' nor 'options.rilMessageType'.

So the original logic has nothing wrong. We should keep it as what it is.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS (v2)

Changes since v1:

  - don't set |number| RilContentHelper.js
  - test for existence of |number| in ril_worker.js

BTW, shouldn't the interface be extended? I'd guess that if I have multiple SIM cards, it would make sense to query the forwarding status for individual SIMs by supplying a phone number.

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8396975 [details] [diff] [review]
[04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS (v2)

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

r=me with comment addressed. Thank you!

::: dom/system/gonk/ril_worker.js
@@ +2556,5 @@
> +    if ('number' in options) {
> +      number = options.number;
> +    } else {
> +      number = '';
> +    }

Let us simply write this:

let number = options.number || "";

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #15)
> Created attachment 8396975 [details] [diff] [review]
> [04] Bug 986440: Add missing field |number| for
> REQUEST_QUERY_CALL_FORWARD_STATUS (v2)
> 
> Changes since v1:
> 
>   - don't set |number| RilContentHelper.js
>   - test for existence of |number| in ril_worker.js
> 
> BTW, shouldn't the interface be extended? I'd guess that if I have multiple
> SIM cards, it would make sense to query the forwarding status for individual
> SIMs by supplying a phone number.

The MobileConnections API has considered multi-SIM scenario. mozMobileConnections[n] is bound to SIM_n.

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8394740 [details] [diff] [review]
[03] Bug 986440: Check for |radioTech| in |voiceRegistrationState|

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

Thanks for this :)

::: dom/system/gonk/ril_worker.js
@@ +3452,5 @@
>        }
>      };
>  
> +    // During startup, |radioTech| is not yet defined, so we need to
> +    // check it separately

nit: place a period at the end of the line

@@ +3453,5 @@
>      };
>  
> +    // During startup, |radioTech| is not yet defined, so we need to
> +    // check it separately
> +    if (('radioTech' in this.voiceRegistrationState) &&

nit: we are using double-quotation marks in ril code. Please use that.

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Hsinyi,

Thanks for the reviews.

> @@ +6188,5 @@
> >  };
> >  RilObject.prototype[REQUEST_GET_SMSC_ADDRESS] = function REQUEST_GET_SMSC_ADDRESS(length, options) {
> >    this.SMSC = options.rilRequestError ? null : this.context.Buf.readString();
> >  
> > +  options.rilMessageType = 'getSmscAddress';
> 
> Oh, sorry that I didn't say that clearly enough. :(
> 
> I don't think we need to set this here.
> this.getSmscAddress() is called internally in ril_worker.js or called by
> Content process, i.e. RILContentHelper.
> 
> If it's the former case, then there is neither 'options' nor
> 'options.rilMessageType'.
> 
> So the original logic has nothing wrong. We should keep it as what it is.

I'm having trouble to get the idea here. As far as I understand the code, |getSmscAddress| sends a request to rild if |this.SMSC| has not yet been set. The argument |options| of the response handler contains the reply from rild. Since there is no |options.rilMessageType|, it simply returns without ever forwarding the reply to the upper layers.

To me this looks like a bug, but if the code is really intended to work this way, would you accept a fix for the warning message?

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 986440: Check for |radioTech| in |voiceRegistrationState| (v3)

Changes since v2:

  - use double quotes
  - added period at end of comment

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 986440: Add missing field |number| for REQUEST_QUERY_CALL_FORWARD_STATUS (v3)

Changes since v2:

  - replaced ugly if-else branching by nice JS magic

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #19)
> Hi Hsinyi,
> 
> Thanks for the reviews.
> 
> > @@ +6188,5 @@
> > >  };
> > >  RilObject.prototype[REQUEST_GET_SMSC_ADDRESS] = function REQUEST_GET_SMSC_ADDRESS(length, options) {
> > >    this.SMSC = options.rilRequestError ? null : this.context.Buf.readString();
> > >  
> > > +  options.rilMessageType = 'getSmscAddress';
> > 
> > Oh, sorry that I didn't say that clearly enough. :(
> > 
> > I don't think we need to set this here.
> > this.getSmscAddress() is called internally in ril_worker.js or called by
> > Content process, i.e. RILContentHelper.
> > 
> > If it's the former case, then there is neither 'options' nor
> > 'options.rilMessageType'.
> > 
> > So the original logic has nothing wrong. We should keep it as what it is.
> 
> I'm having trouble to get the idea here. As far as I understand the code,
> |getSmscAddress| sends a request to rild if |this.SMSC| has not yet been
> set. 

Till now you are correct!

> The argument |options| of the response handler contains the reply from
> rild. 

Not exactly.

[1] queries "getSmscAddress" then if there's no this.SMSC, [2] is called. Then [3] is called with "options" which is put into a mapping table, mTokenRequestMap. Once the response is sent back from rild, [4] takes the stored "options" out. ril_worker adds one new property "options.rilRequestError" according to the error response from rild. Finally we are in [5] to handle every specific request response.

So, options.rilMessageType is there. Hope it's clearer to you now.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#4076
[2] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#1965
[3] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#168
[4] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#127
[5] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#156

> Since there is no |options.rilMessageType|, it simply returns without
> ever forwarding the reply to the upper layers.
> 
> To me this looks like a bug, but if the code is really intended to work this
> way, would you accept a fix for the warning message?

Could you show me the warning message? If there is, I'd like to fix that but this patch seems not a right solution. :)

Comment 23

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v3)

The warning is

> E/GeckoConsole(  178): [JavaScript Warning: "ReferenceError: reference to undefined property options.rilMessageType" {file: "resource://gre/modules/ril_worker.js" line: 6194}]

I further debugged the issue and found that the faulty call to |RilObject.getSmscAddress| is in |_processVoiceRegistrationState|, both in ril_worker.js. This call does not supply a parameter to getSmscAddress, so no |rilMessageType| field exists. This new patch fixes this.

Comment 24

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8397017 [details] [diff] [review]
[02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v3)

Changes since v2:

  - handle |options.rilMessageType| in response handler
  - supply options object with cleared |rilMessageType| when calling |getSmscAddress| in |_processVoiceRegistrationState|

Comment 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8397017 [details] [diff] [review]
[02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v3)

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

We shall be fine next run. :)

::: dom/system/gonk/ril_worker.js
@@ -3571,5 @@
>    _processVoiceRegistrationState: function(state) {
>      let rs = this.voiceRegistrationState;
>      let stateChanged = this._processCREG(rs, state);
>      if (stateChanged && rs.connected) {
> -      this.getSmscAddress();

Indeed, this gives no parameter.

Let us complete the if-condition in [1] [2] instead:

if (!options || !options.rilMessageType ||
    options.rilMessageType !== "getSmscAddress")



[1] http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/ril_worker.js#l1969
[2] http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/ril_worker.js#l6192

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #25)
> Comment on attachment 8397017 [details] [diff] [review]
> [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler
> (v3)
> 
> Review of attachment 8397017 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We shall be fine next run. :)
> 
> ::: dom/system/gonk/ril_worker.js
> @@ -3571,5 @@
> >    _processVoiceRegistrationState: function(state) {
> >      let rs = this.voiceRegistrationState;
> >      let stateChanged = this._processCREG(rs, state);
> >      if (stateChanged && rs.connected) {
> > -      this.getSmscAddress();
> 
> Indeed, this gives no parameter.
> 
> Let us complete the if-condition in [1] [2] instead:
> 
> if (!options || !options.rilMessageType ||
>     options.rilMessageType !== "getSmscAddress")
> 
> 
> 
> [1]
> http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/
> ril_worker.js#l1969
> [2]
> http://hg.mozilla.org/mozilla-central/annotate/c69c55582faa/dom/system/gonk/
> ril_worker.js#l6192

There's always "options" in [2], so we could just have the if-clause
if (!options.rilMessageType || options.rilMessageType !== "getSmscAddress")

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v4)

Next round :)

Changes since v3:

  - revert v3
  - test for |options.rilMessageType| in request handler

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8397035 [details] [diff] [review]
[02] Bug 986440: Remove incorrect test from REQUEST_GET_SMSC_ADDRESS handler (v4)

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

Nice! Thanks again for catching this :P

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks for your reviews. :)

https://hg.mozilla.org/integration/b2g-inbound/rev/11eca35e9a33
https://hg.mozilla.org/integration/b2g-inbound/rev/aedb08f050f5
https://hg.mozilla.org/integration/b2g-inbound/rev/905fab37da9c
https://hg.mozilla.org/integration/b2g-inbound/rev/4613e505c800

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=4613e505c800

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/11eca35e9a33
https://hg.mozilla.org/mozilla-central/rev/aedb08f050f5
https://hg.mozilla.org/mozilla-central/rev/905fab37da9c
https://hg.mozilla.org/mozilla-central/rev/4613e505c800