1115603 - Introduce nsITelephonyCallInfo and use it to provide call info in nsITelephonyListener

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

Originally, in nsITelephonyListener, the function below requires 12 parameters.

void callStateChanged(in unsigned long clientId,
                        in unsigned long callIndex,
                        in unsigned short callState,
                        in AString number,
                        in unsigned short numberPresentation,
                        in AString name,
                        in unsigned short namePresentation,
                        in boolean isOutgoing,
                        in boolean isEmergency,
                        in boolean isConference,
                        in boolean isSwitchable,
                        in boolean isMergeable);

I am thinking about introducing an interface for those data. Then it could become...

interface nsITelephonyListener : nsISupports
{
  void enumerateCallState(in nsITelephonyCallInfo info);
  void callStateChanged(in nsITelephonyCallInfo info);
}


Benefits:

1. Whenever we need to add a new property for call info, we could just modify the interface nsITelephonyCallInfo and leave callStateChanged(), callStateChanged() and their implementation unchanged. 

2. After we have an interface for call info, we could keep improving callStateChanged() by passing an array of interface. That means we could merge several callStateChanged() into one and reduce the number of message passing.

Comment 1

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

+1 for the proposal!

Comment 2

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1: Add nsITelephonyCallInfo (interface)

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 1#2: Add nsITelephonyCallInfo (interface)

Comment 4

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2: Implement nsITelephonyCallInfo and use it (dom)

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 3: Implement nsITelephonyCallInfo and use it (ril)

Comment 6

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4: Use nsITelephonyCallInfo for call data (bluetooth)

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5: Add serializer for nsITelephonyCallInfo (ipc)

Comment 8

[Anshul]

Hsin-Yi, is this bug intended for 2.2?

Comment 9

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

(In reply to Anshul from comment #8)
> Hsin-Yi, is this bug intended for 2.2?

Hello Anshul,
We'd like to move Bug 1000485 - Streamline nsITelephonyService API forward as much as possible before we start to lock down ril interfaces. Considering the progress, it looks firm that we could land this patch on v2.2 (before Jan 12th). So, my answer is yes to your question. Let me know if any problems or further questions, thank you.

Comment 10

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

Comment on attachment 8541551 [details] [diff] [review]
Part 1#2: Add nsITelephonyCallInfo (interface)

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

Looks good to me.

Comment 11

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

Comment on attachment 8541563 [details] [diff] [review]
Part 2: Implement nsITelephonyCallInfo and use it (dom)

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

::: dom/telephony/Telephony.cpp
@@ +567,5 @@
>  
> +NS_IMETHODIMP
> +Telephony::EnumerateCallState(nsITelephonyCallInfo* aInfo)
> +{
> +  return HandleCallInfo(aInfo);

It's no harm to call HandleCallInfo here, but it looks too heavy in the case of "a call being in the calls list already."
I wonder if there's possibility that  any of call attributes changes.

::: dom/telephony/moz.build
@@ +22,5 @@
>  ]
>  
>  EXPORTS.mozilla.dom.telephony += [
>      'ipc/TelephonyChild.h',
> +    'ipc/TelephonyIPCSerializer.h',

Though we eventually need this, this doesn't seem belong to this part.

Comment 12

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

Comment on attachment 8541564 [details] [diff] [review]
Part 3: Implement nsITelephonyCallInfo and use it (ril)

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

r=me with comments addressed, thank you.

::: dom/telephony/gonk/TelephonyService.js
@@ +143,5 @@
> +  clientId: 0,
> +  callIndex: 0,
> +  callState: nsITelephonyService.CALL_STATE_UNKNOWN,
> +  number: "",
> +  numberPresentation: 0,

Let's use nsITelephonyService.CALL_PRESENTATION_ALLOWED

@@ +145,5 @@
> +  callState: nsITelephonyService.CALL_STATE_UNKNOWN,
> +  number: "",
> +  numberPresentation: 0,
> +  name: "",
> +  namePresentation: 0,

ditto.

Comment 13

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 2#2: Implement nsITelephonyCallInfo and use it (dom)

>  
> +NS_IMETHODIMP
> +Telephony::EnumerateCallState(nsITelephonyCallInfo* aInfo)
> +{
> +  return HandleCallInfo(aInfo);

> It's no harm to call HandleCallInfo here, but it looks too heavy in the case > of "a call being in the calls list already."
> I wonder if there's possibility that  any of call attributes changes.

I don't think we need to worry about it. Actually, for me, HandleCallInfo() is not a heavy function and the probability for the case you mentioned is really low. I'd prefer to have an easier code with an acceptable performance overhead.

If I want to address your comment, the fix in my find is as below. I'll introduce a boolean parameter. How do you think?

NS_IMETHODIMP
Telephony::CallStateChanged(nsITelephonyCallInfo* aInfo)
{
  return HandleCallInfo(aInfo, true);
}

NS_IMETHODIMP
Telephony::EnumerateCallState(nsITelephonyCallInfo* aInfo)
{
  return HandleCallInfo(aInfo, false);
}

Telephony::HandleCallInfo(nsITelephonyCallInfo* aInfo, bool isChange)
{
  ...

  if (!call) {
    nsRefPtr<TelephonyCallId> id = CreateCallId(aInfo);
    call = CreateCall(id, serviceId, callIndex, callState, isEmergency,
                      isConference, isSwitchable, isMergeable);
    ...

  } else if (isChange) {
    ...
  }
}

Comment 14

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 4#2: Use nsITelephonyCallInfo for call data (bluetooth)

Hi Ben,

May I have your review? We would like to provide a new interface for call related information and use it for nsITelephonyListener::CallStateChanged and nsITelephonyListener::EnumerateCallState.

Comment 15

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#2: Add serializer for nsITelephonyCallInfo (ipc)

Hi Hsinyi and Kyle,

It's my first time writing an IPC serializer and I am worry about if I have the correct handling of ref_cnt. So, I would like to have both your reviews.

Comment 16

[Ben Tian (inactive)]

Comment on attachment 8544354 [details] [diff] [review]
Part 4#2: Use nsITelephonyCallInfo for call data (bluetooth)

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

r=me with nit addressed.

::: dom/bluetooth/BluetoothRilListener.cpp
@@ +213,2 @@
>    return NS_OK;
> +

nit: remove extra newline.

::: dom/bluetooth2/BluetoothRilListener.cpp
@@ +212,2 @@
>    return NS_OK;
> +

nit: remove extra newline.

Comment 17

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

(In reply to Szu-Yu Chen [:aknow] from comment #13)
> Created attachment 8544353 [details] [diff] [review]
> Part 2#2: Implement nsITelephonyCallInfo and use it (dom)
> 
> >  
> > +NS_IMETHODIMP
> > +Telephony::EnumerateCallState(nsITelephonyCallInfo* aInfo)
> > +{
> > +  return HandleCallInfo(aInfo);
> 
> > It's no harm to call HandleCallInfo here, but it looks too heavy in the case > of "a call being in the calls list already."
> > I wonder if there's possibility that  any of call attributes changes.
> 
> I don't think we need to worry about it. Actually, for me, HandleCallInfo()
> is not a heavy function and the probability for the case you mentioned is
> really low. I'd prefer to have an easier code with an acceptable performance
> overhead.
> 
That's fair enough. I'll take your opinion :)

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8544355 [details] [diff] [review]
Part 5#2: Add serializer for nsITelephonyCallInfo (ipc)

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

::: dom/telephony/ipc/TelephonyIPCSerializer.h
@@ +119,5 @@
> +                              numberPresentation, name, namePresentation,
> +                              isOutgoing, isEmergency, isConference,
> +                              isSwitchable, isMergeable);
> +
> +    *aResult = info;

This is wrong.  The TelephonyCallInfo will be destroyed when | info | goes out of scope and ~nsCOMPtr runs.  But you've handed out a pointer to this memory, so we will use-after-free.  You need to do info.forget(aResult) or similar.  MobileConnectionIPCSerializer.h has other acceptable examples too.

Comment 19

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#3: Add serializer for nsITelephonyCallInfo (ipc)

Use info.forget(aResult).

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8544987 [details] [diff] [review]
Part 5#3: Add serializer for nsITelephonyCallInfo (ipc)

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

I probably should have been more explicit.  You've fixed the use-after-free problem, but now you're leaking the nsITelephonyCallInfo objects created from ParamTraits<>::Read.  The .forget() call creates a reference which is passed through the IPDL machinery to the RecvX functions.  Those functions must adopt ownership of the object, or we will leak.  Look at RecvNotify[Voice|Data]InfoChanged.

Comment 21

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 5#4: Add serializer for nsITelephonyCallInfo (ipc). r=khuey

Again.  Hope this time is okay...

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8545665 [details] [diff] [review]
Part 5#4: Add serializer for nsITelephonyCallInfo (ipc). r=khuey

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

r=me

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +62,2 @@
>  {
> +  // Use dont_AddRef here because this instances is already AddRef-ed in

this instance has already been AddRef-ed

I know you just copied this from the other location, but it is wrong there too ;)

@@ +162,3 @@
>  {
> +  // Use dont_AddRef here because this instances is already AddRef-ed in
> +  // TelephonyIPCSerializer.h

ibid

Comment 23

[Szu-Yu Chen [:aknow] (inactive)]

try looks good

Comment 24

[Szu-Yu Chen [:aknow] (inactive)]

... forgot to paste the url
try result => https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dcea4e638b8e



https://hg.mozilla.org/integration/b2g-inbound/rev/cacbc80a7926
https://hg.mozilla.org/integration/b2g-inbound/rev/16dabca480c5
https://hg.mozilla.org/integration/b2g-inbound/rev/1c5fc584e126
https://hg.mozilla.org/integration/b2g-inbound/rev/ce8dd8937f94
https://hg.mozilla.org/integration/b2g-inbound/rev/e3ecbb3f7bcd

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/cacbc80a7926
https://hg.mozilla.org/mozilla-central/rev/16dabca480c5
https://hg.mozilla.org/mozilla-central/rev/1c5fc584e126
https://hg.mozilla.org/mozilla-central/rev/ce8dd8937f94
https://hg.mozilla.org/mozilla-central/rev/e3ecbb3f7bcd