Last Comment Bug 748745 - readString through CDataFinalizer
: readString through CDataFinalizer
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: x86 All
: -- enhancement (vote)
: mozilla15
Assigned To: David Teller [:Yoric] (please use "needinfo")
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 748776
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-25 06:41 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2012-05-17 20:30 PDT (History)
2 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Supporting readString in a CDataFinalizer (6.15 KB, patch)
2012-04-25 08:05 PDT, David Teller [:Yoric] (please use "needinfo")
jorendorff: review+
Details | Diff | Splinter Review
Supporting readString in a CDataFinalizer (5.74 KB, patch)
2012-05-03 08:37 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Supporting readString in a CDataFinalizer (5.86 KB, patch)
2012-05-06 23:36 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Supporting readString in a CDataFinalizer (5.99 KB, patch)
2012-05-06 23:39 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Supporting readString in a CDataFinalizer (5.99 KB, patch)
2012-05-07 00:01 PDT, David Teller [:Yoric] (please use "needinfo")
jorendorff: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-04-25 06:41:47 PDT
At the moment, readString may only be called on a CData object. We should be able to call it also on a CDataFinalizer.
Comment 1 David Teller [:Yoric] (please use "needinfo") 2012-04-25 08:05:25 PDT
Created attachment 618266 [details] [diff] [review]
Supporting readString in a CDataFinalizer
Comment 2 Jason Orendorff [:jorendorff] 2012-05-03 08:30:25 PDT
Comment on attachment 618266 [details] [diff] [review]
Supporting readString in a CDataFinalizer

I don't see where test_finalizer_rel_string_t frees the memory allocated for the string in test_finalizer_acq_string_t. It's a leak, right?

>   acquire_void_ptr = library.declare("test_finalizer_acq_ptr_t",
>                                 ctypes.default_abi,
>-                                ctypes.int32_t.ptr,
>+                                ctypes.void_t.ptr,
>                                 ctypes.size_t);
>   dispose_void_ptr = library.declare("test_finalizer_rel_ptr_t",
>                                 ctypes.default_abi,
>                                 ctypes.void_t,
>                                 ctypes.int32_t.ptr);

While you're fixing acquire_void_ptr, I think the last line here (for dispose_void_ptr) should get the same change.

r=me with those changes.
Comment 3 David Teller [:Yoric] (please use "needinfo") 2012-05-03 08:37:17 PDT
Created attachment 620715 [details] [diff] [review]
Supporting readString in a CDataFinalizer

Fixed, thanks.
Comment 4 David Teller [:Yoric] (please use "needinfo") 2012-05-06 23:36:22 PDT
Created attachment 621510 [details] [diff] [review]
Supporting readString in a CDataFinalizer

Oops, turns out that this code could freeze – and that the testsuite somehow does not detect this. Now fixed.
Comment 5 David Teller [:Yoric] (please use "needinfo") 2012-05-06 23:39:27 PDT
Created attachment 621511 [details] [diff] [review]
Supporting readString in a CDataFinalizer
Comment 6 David Teller [:Yoric] (please use "needinfo") 2012-05-07 00:01:37 PDT
Created attachment 621515 [details] [diff] [review]
Supporting readString in a CDataFinalizer

Turns out that the error was caused by the interaction with the patch for bug 723530 – and that while it broke the xpcshell tests, it did so silently. Here is a patch that should solve the issue.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-05-17 06:42:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/14ab85ab1549
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-05-17 20:30:29 PDT
https://hg.mozilla.org/mozilla-central/rev/14ab85ab1549

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