Hsin-Yi Tsai (she/her) [:hsinyi] Assignee
	Description • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Assignee
	Comment 1 • 12 years ago

(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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 2 • 12 years ago

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);
  }

	Philipp von Weitershausen [:philikon]
	Comment 3 • 12 years ago

(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.

	Hsin-Yi Tsai (she/her) [:hsinyi] Assignee
	Comment 4 • 12 years ago

Thanks for the above comments.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 5 • 12 years ago

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 -

	Hsin-Yi Tsai (she/her) [:hsinyi] Assignee
	Comment 6 • 12 years ago

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=d498ea306a23

	Hsin-Yi Tsai (she/her) [:hsinyi] Assignee
	Comment 7 • 12 years ago

Pushed a revision with nit in Comment 5 addressed to inbound.

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

	Ed Morley [:emorley]
	Comment 8 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/7d2c164b80e6