WebTelephony: invalid argument in Telephony::NotifyError

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: hsinyi, Assigned: hsinyi)

Tracking

unspecified
mozilla16
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

1.99 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
Seems not always correct in the mapping between 'aCallIndex' and 'the index of mCalls' in 'Telephony::NotifyError'. 

For example, 'aCallIndex' starts from '1', and is likely equal to mCalls.Length(). 

Or, there may be empty slot in 'mCalls' when a new call object is created. It is not always true that the latest call object will be appended to the calls array. 

So, need some improvement to get the right index for the failed call.
Assignee: nobody → htsai
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #0)
> Seems not always correct in the mapping between 'aCallIndex' and 'the index
> of mCalls' in 'Telephony::NotifyError'. 
> 
> For example, 'aCallIndex' starts from '1', and is likely equal to
> mCalls.Length(). 
> 
> Or, there may be empty slot in 'mCalls' when a new call object is created.
> It is not always true that the latest call object will be appended to the
> calls array. 

Sorry, my bad. Nothing wrong with the second example here. I got dazzled when doing some experiments for multi calls. But I am sure problems in the 1st example.  
 
> So, need some improvement to get the right index for the failed call.
aCallIndex has nothing to do with the index of the call in the mCalls array. This code is just wrong.

We should be doing something like this:

  NS_IMETHODIMP
  Telephony::NotifyError(PRInt32 aCallIndex,
                         const nsAString& aError)
  {
    nsRefPtr<TelephonyCall> callToNotify;

    if (!mCalls.IsEmpty()) {
      // The connection is not established yet, remove the latest call object
      if (aCallIndex == -1) {
        callToNotify = mCalls[mCalls.Length() - 1];
      else {
        for (PRUint32 index = 0; index < mCalls.Length(); index++) {
          nsRefPtr<TelephonyCall>& call = mCalls[index];
          if (call->CallIndex() == aCallIndex) {
            callToNotify = call;
            break;
          }
        }
      }
    }

    if (!callToNotify) {
      NS_ERROR("Don't call me with a bad call index!");
      return NS_ERROR_UNEXPECTED;
    }

    return callToNotify->NotifyError(aError);
  }
Depends on: 712944
(In reply to ben turner [:bent] from comment #2)
> aCallIndex has nothing to do with the index of the call in the mCalls array.
> This code is just wrong.

Gah, you're right. I should've caught that in review. Thanks for catching that.
Created attachment 634751 [details] [diff] [review]
patch

Thanks for the above comments.
Attachment #634751 - Flags: review?(bent.mozilla)
Comment on attachment 634751 [details] [diff] [review]
patch

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

::: dom/telephony/Telephony.cpp
@@ +483,5 @@
> +  nsRefPtr<TelephonyCall> callToNotify;
> +  if (!mCalls.IsEmpty()) {
> +    // The connection is not established yet. Get the latest call object.
> +    if (aCallIndex == -1) {
> +      callToNotify = mCalls[mCalls.Length()-1];

Nit: spaces around -
Attachment #634751 - Flags: review?(bent.mozilla) → review+
Try server result: https://tbpl.mozilla.org/?tree=Try&rev=d498ea306a23
Pushed a revision with nit in Comment 5 addressed to inbound.

http://hg.mozilla.org/integration/mozilla-inbound/rev/7d2c164b80e6

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7d2c164b80e6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.