Last Comment Bug 765684 - WebTelephony: invalid argument in Telephony::NotifyError
: WebTelephony: invalid argument in Telephony::NotifyError
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Hsin-Yi Tsai [:hsinyi]
:
:
Mentors:
Depends on: 712944
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-18 04:15 PDT by Hsin-Yi Tsai [:hsinyi]
Modified: 2012-06-22 09:17 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.99 KB, patch)
2012-06-19 21:26 PDT, Hsin-Yi Tsai [:hsinyi]
bent.mozilla: review+
Details | Diff | Splinter Review

Description Hsin-Yi Tsai [:hsinyi] 2012-06-18 04:15:13 PDT
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.
Comment 1 Hsin-Yi Tsai [:hsinyi] 2012-06-18 05:06:16 PDT
(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.
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-18 12:24:21 PDT
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);
  }
Comment 3 Philipp von Weitershausen [:philikon] 2012-06-18 17:24:34 PDT
(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.
Comment 4 Hsin-Yi Tsai [:hsinyi] 2012-06-19 21:26:13 PDT
Created attachment 634751 [details] [diff] [review]
patch

Thanks for the above comments.
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-20 17:00:14 PDT
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 -
Comment 6 Hsin-Yi Tsai [:hsinyi] 2012-06-21 18:31:36 PDT
Try server result: https://tbpl.mozilla.org/?tree=Try&rev=d498ea306a23
Comment 7 Hsin-Yi Tsai [:hsinyi] 2012-06-21 20:05:06 PDT
Pushed a revision with nit in Comment 5 addressed to inbound.

http://hg.mozilla.org/integration/mozilla-inbound/rev/7d2c164b80e6
Comment 8 Ed Morley [:emorley] 2012-06-22 09:17:09 PDT
https://hg.mozilla.org/mozilla-central/rev/7d2c164b80e6

Note You need to log in before you can comment on or make changes to this bug.