Closed Bug 739780 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jaws, Assigned: gsvelto)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attached patch Proposed patch (obsolete) — Splinter Review
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
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.
Attachment #665165 - Flags: feedback?(khuey)
Attachment #665165 - Flags: feedback?(luke)
Comment on attachment 665165 [details] [diff] [review]
Proposed patch

I'm probably not a good person to review this.
Attachment #665165 - Flags: feedback?(luke)
Attachment #665165 - Flags: feedback?(khuey)
Attached patch Proposed patchSplinter Review
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.
Attachment #665165 - Attachment is obsolete: true
Attachment #665981 - Flags: review+
Attachment #665981 - Flags: review+ → review?(khuey)
Attachment #665981 - Flags: review?(mrbkap)
(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?
Attachment #665981 - Flags: review?(mrbkap) → review+
(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?
(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
(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.
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/gsvelto@mozilla.com-eb474feaede5
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/gsvelto@mozilla.com-eb474feaede5
The two test failures in the the try build seem transient and unrelated to my patch
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e6bc6b88d3ba
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.