766866 - B2G RIL: DataCallInfo.apn is null in RILv6

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Save data call id to APN mapping.

Also save other parameters in the options object. There is a followup to simplify the data call info object.

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 635202 [details] [diff] [review]
Save data call id to APN mapping.

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

::: dom/system/gonk/ril_worker.js
@@ +656,5 @@
>    /**
> +   * Map data call id to APN. This information is lost in
> +   * REQUEST_DATA_CALL_LIST response.
> +   */
> +  apnMap: {},

I'm not sure you mean "lost" here.

@@ +3273,5 @@
>  RIL[REQUEST_LAST_DATA_CALL_FAIL_CAUSE] = null;
>  
> +RIL.readDataCall_v5 = function readDataCall_v5(options) {
> +  if (!options) {
> +      options = {};

Nit: indent by 2 spaces

@@ +3311,5 @@
> +  if (!options.apn) {
> +    options.apn = this.apnMap[options.cid];
> +  } else {
> +    this.apnMap[options.cid] = options.apn;
> +  }

So why is `this.apnMap` needed? Either `options` or `this.currentDataCalls[options.cid]` will already contain the right APN, no? If not, let's fix `RIL._processDataCallList()` to keep the right information in `this.currentDataCalls` instead of creating yet another mapping.

(Also, `this.apnMap` is never cleaned up, so it'll easily contain stale information.)

The rest of the patch looks very good, though! Thanks!

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> Comment on attachment 635202 [details] [diff] [review]
> Save data call id to APN mapping.
> 
> Review of attachment 635202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +656,5 @@
> >    /**
> > +   * Map data call id to APN. This information is lost in
> > +   * REQUEST_DATA_CALL_LIST response.
> > +   */
> > +  apnMap: {},
> 
> I'm not sure you mean "lost" here.

Ah.. I mean lost because in v5 there was a APN field in the response. Now this information is only available from the SetupDataCall parameter.

> 
> @@ +3273,5 @@
> >  RIL[REQUEST_LAST_DATA_CALL_FAIL_CAUSE] = null;
> >  
> > +RIL.readDataCall_v5 = function readDataCall_v5(options) {
> > +  if (!options) {
> > +      options = {};
> 
> Nit: indent by 2 spaces
> 
> @@ +3311,5 @@
> > +  if (!options.apn) {
> > +    options.apn = this.apnMap[options.cid];
> > +  } else {
> > +    this.apnMap[options.cid] = options.apn;
> > +  }
> 
> So why is `this.apnMap` needed? Either `options` or
> `this.currentDataCalls[options.cid]` will already contain the right APN, no?
> If not, let's fix `RIL._processDataCallList()` to keep the right information
> in `this.currentDataCalls` instead of creating yet another mapping.

No. If the options is passed in by setupDataCall() then it will contain the APN field. But if it is passed in by REQUEST_DATA_CALL_LIST (someone called getDataCallList()) then it wont.

> 
> (Also, `this.apnMap` is never cleaned up, so it'll easily contain stale
> information.)

Right!

Comment 4

[Philipp von Weitershausen [:philikon]]

(In reply to Kan-Ru Chen [:kanru] from comment #3)
> > >    /**
> > > +   * Map data call id to APN. This information is lost in
> > > +   * REQUEST_DATA_CALL_LIST response.
> > > +   */
> > > +  apnMap: {},
> > 
> > I'm not sure you mean "lost" here.
> 
> Ah.. I mean lost because in v5 there was a APN field in the response. Now
> this information is only available from the SetupDataCall parameter.

I see. Historical notes are not very useful as source code comments because newcomers don't necessarily understand the context.

> > So why is `this.apnMap` needed? Either `options` or
> > `this.currentDataCalls[options.cid]` will already contain the right APN, no?
> > If not, let's fix `RIL._processDataCallList()` to keep the right information
> > in `this.currentDataCalls` instead of creating yet another mapping.
> 
> No. If the options is passed in by setupDataCall() then it will contain the
> APN field. But if it is passed in by REQUEST_DATA_CALL_LIST (someone called
> getDataCallList()) then it wont.

Ok, but at some point `setupDataCall()` was called to create the data call in the first place. So `this.currentDataCalls` should already have an entry for it that `_processDataCallList()` can simply update.

The only scenario that I see where that's not true is if Gecko crashed and has to re-read the state via `getDataCallList()`. But then I don't see how your `apnMap` would help us, either.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> > > So why is `this.apnMap` needed? Either `options` or
> > > `this.currentDataCalls[options.cid]` will already contain the right APN, no?
> > > If not, let's fix `RIL._processDataCallList()` to keep the right information
> > > in `this.currentDataCalls` instead of creating yet another mapping.
> > 
> > No. If the options is passed in by setupDataCall() then it will contain the
> > APN field. But if it is passed in by REQUEST_DATA_CALL_LIST (someone called
> > getDataCallList()) then it wont.
> 
> Ok, but at some point `setupDataCall()` was called to create the data call
> in the first place. So `this.currentDataCalls` should already have an entry
> for it that `_processDataCallList()` can simply update.
> 
> The only scenario that I see where that's not true is if Gecko crashed and
> has to re-read the state via `getDataCallList()`. But then I don't see how
> your `apnMap` would help us, either.

Every time you call `getDataCallList()`, a set of new datacalls without the original parameters is passed to `_processDataCallList()`. But I see, we can copy the information to them in `_processDataCallList()`

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to Kan-Ru Chen [:kanru] from comment #5)
> Every time you call `getDataCallList()`, a set of new datacalls without the
> original parameters is passed to `_processDataCallList()`.

Yes, but `_processDataCallList()` copies information over rather than replacing the data call objection: https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2296

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> (In reply to Kan-Ru Chen [:kanru] from comment #5)
> > Every time you call `getDataCallList()`, a set of new datacalls without the
> > original parameters is passed to `_processDataCallList()`.
> 
> Yes, but `_processDataCallList()` copies information over rather than
> replacing the data call objection:
> https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js#2296

Great! I didn't notice that `datacalls` array is updated in place.

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Save data call information in data call object

Removed the apnMap.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Save data call information in data call object

patch bitrotted

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a757d8b9cda1

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a757d8b9cda1