739780 - Switch Telephony.cpp to use nsTArrayHelpers.h implementation of nsTArrayToJSArray

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Bug 730318 added a more generic and memory-safe implementation of nsTArrayToJSArray.

The implementation used in Telephony.cpp doesn't properly root the valArray object.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Proposed patch

I've attached a tentative patch, what the patch does is:
- include nsTArrayHelpers.h in Telephony.cpp and remove the nsTArrayToJSArray implementation that was previously there
- add a CallQueryInterface() template function to nsAutoPtr.h to accomodate for the conversion in nsTArrayToJSArray
- remove Windows line endings (CR/LF) from nsTArrayHelpers.h

Comment 2

[Gabriele Svelto [:gsvelto]]

I am not entirely satisfied with the solution I've come up with. Using the new version of nsTArrayToJSArray wouldn't work straight away as there was no version of CallQueryInterface() that would accomodate 'const nsRefPtr<>' as the source and 'nsComPtr<>' as the destination. However this doesn't seem like a very elegant solution to me, probably a better way would be to alter nsTArrayToJSArray instead, making it more generic but I haven't figured out how to do it yet.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 665165 [details] [diff] [review]
Proposed patch

I'm probably not a good person to review this.

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: Proposed patch

I've revised my patch making it far less intrusive (no changes to nsAutoPtr.h are present in this one). To enable nsTArrayToJSArray() to work correctly in the Telephony.cpp case I've removed the CallQueryInterface() call and replaced it with an explicit invocation to QueryInterface(), this removes the typing ambiguities that prevented the compiler from accepting the proper CallQueryInterface() template. In addition to this the patch removes the redundant nsTArrayToJSArray() version and the CR/LF line endings from nsTArrayHelpers.h.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Jared Wein [:jaws] from comment #0)
> The implementation used in Telephony.cpp doesn't properly root the valArray
> object.

I'm not sure what you mean here. valArray is a new'd array of jsvals, which themselves are properly rooted. There's no need to root it. That being said, I'm all for this. Is there a separate bug filed to switch BluetoothDeviceArrayToJSArray over as well, or can we use this bug to do it?

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Blake Kaplan (:mrbkap) from comment #5)
> That being said,
> I'm all for this. Is there a separate bug filed to switch
> BluetoothDeviceArrayToJSArray over as well, or can we use this bug to do it?

I was discussing this on IRC with :qDot, he mentioned it would be a good idea to fold the ones into BluetoothUtils.cpp too (BluetoothDeviceArrayToJSArray and StringArrayToJSArray). I don't think there's already another bug for that though, shall I use this one instead? Or shall I create a new one after this patch is applied?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Blake Kaplan (:mrbkap) from comment #5)
> (In reply to Jared Wein [:jaws] from comment #0)
> > The implementation used in Telephony.cpp doesn't properly root the valArray
> > object.
> 
> I'm not sure what you mean here. valArray is a new'd array of jsvals, which
> themselves are properly rooted. There's no need to root it.

This bug was filed in March when the code looked like this, http://hg.mozilla.org/mozilla-central/diff/324ad5b90d7d/dom/telephony/Telephony.cpp

Since then, there have been two patches to fix this:
http://hg.mozilla.org/mozilla-central/rev/935501afee09
http://hg.mozilla.org/mozilla-central/rev/587ab949e65a

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Or shall I create a new one after this patch is applied?

Given that we already have a patch with review here, let's file a new one for the remaining folding (IMO bugzilla doesn't really deal well with multiple-patch bugs that don't land at the same time).

(In reply to Jared Wein [:jaws] from comment #7)
> This bug was filed in March when the code looked like this,

Yikes, I never even looked at the date! Indeed.

Comment 9

[Gabriele Svelto [:gsvelto]]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=eb474feaede5

Comment 10

[Mozilla RelEng Bot]

Try run for eb474feaede5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eb474feaede5
Results (out of 143 total builds):
    success: 136
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-eb474feaede5

Comment 11

[Mozilla RelEng Bot]

Try run for eb474feaede5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eb474feaede5
Results (out of 146 total builds):
    success: 138
    warnings: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-eb474feaede5

Comment 12

[Gabriele Svelto [:gsvelto]]

The two test failures in the the try build seem transient and unrelated to my patch

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ed Morley [:emorley]]

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