Last Comment Bug 748776 - CDataFinalizer should be able to maintain the exact type of a CData
: CDataFinalizer should be able to maintain the exact type of a CData
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on: 745184
Blocks: 748745
  Show dependency treegraph
 
Reported: 2012-04-25 07:45 PDT by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-05-10 08:01 PDT (History)
2 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Recording a more precise type in CDataFinalizer::Construct (150 bytes, patch)
2012-04-25 07:56 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Returning the result of dispose (4.52 KB, patch)
2012-04-25 07:57 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Recording a more precise type in CDataFinalizer::Construct (4.59 KB, patch)
2012-04-25 08:09 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Recording a more precise type in CDataFinalizer::Construct (6.72 KB, patch)
2012-05-07 00:29 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
jorendorff: review+
Details | Diff | Review
Recording a more precise type in CDataFinalizer::Construct (9.39 KB, patch)
2012-05-07 08:59 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Recording a more precise type in CDataFinalizer::Construct (9.40 KB, patch)
2012-05-09 01:34 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-25 07:45:20 PDT
At the moment, if we have a CData holding a |char*| and we pass it to a CDataFinalizer with finalizer |free|, the CDataFinalizer will itself be listed as |void*|, because we are only looking at the type of the function.

In this case, we need to be able to return |char*|.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-25 07:56:24 PDT
Created attachment 618262 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct
Comment 2 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-25 07:57:40 PDT
Created attachment 618263 [details] [diff] [review]
Returning the result of dispose

Same one, but a non-empty patch.
Comment 3 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-25 08:09:50 PDT
Created attachment 618269 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

Fixing the tests. Sorry about the snafu. Should now be ok.
Comment 4 Jason Orendorff [:jorendorff] 2012-05-03 14:01:19 PDT
Comment on attachment 618269 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

Review of attachment 618269 [details] [diff] [review]:
-----------------------------------------------------------------

Wow. The fact that it's like this is actually a kind of interesting observation on its own. It kind of breaks how I was thinking of CDataFinalizer (you learn something every day). What kind of real-world use cases are we addressing here? Are there also use cases where ImplicitConvert is too strict?

We probably want a few more tests that this isn't going to go horribly wrong. For example, what if the type of the CData object is a smaller size than the finalizer argument type? I think you could get valgrind errors or correctness errors, maybe crashes. Note that CDataFinalizer::Construct allocates a buffer of type objArgType, but CDataFinalizer::Methods::Forget assumes that the buffer holds a value of exactly GetCType(obj).

I think if we need this, CDataFinalizer::Construct should be pretty strict about it. Perhaps it should allow this only in cases where the argument type and the CData type are both pointer types of the same size. Other cases should throw--which should be easy enough to test.

Clearing r?.
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-03 14:46:36 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Comment on attachment 618269 [details] [diff] [review]
> Recording a more precise type in CDataFinalizer::Construct
> 
> Review of attachment 618269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Wow. The fact that it's like this is actually a kind of interesting
> observation on its own. It kind of breaks how I was thinking of
> CDataFinalizer (you learn something every day). What kind of real-world use
> cases are we addressing here? Are there also use cases where ImplicitConvert
> is too strict?

The specific scenario is the following:
- I have obtained a |char*| from some C function (in that case, |getwd(NULL)|);
- I need to deallocate that |char*| with |free|, with a CDataFinalizer;
- I also need to pass that |char*| to |readString| and other functions that take a |char*|.
Comment 6 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-04 00:00:10 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> We probably want a few more tests that this isn't going to go horribly
> wrong. For example, what if the type of the CData object is a smaller size
> than the finalizer argument type? I think you could get valgrind errors or
> correctness errors, maybe crashes. Note that CDataFinalizer::Construct
> allocates a buffer of type objArgType, but CDataFinalizer::Methods::Forget
> assumes that the buffer holds a value of exactly GetCType(obj).
> 
> I think if we need this, CDataFinalizer::Construct should be pretty strict
> about it. Perhaps it should allow this only in cases where the argument type
> and the CData type are both pointer types of the same size. Other cases
> should throw--which should be easy enough to test.

I assumed (wrongly, I guess) that the call to |ImplicitConvert| would check that nothing could go wrong at http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#6585

Now, as you suggest, I could easily add a safety net when I obtain |objBestArgType|, by checking that |objBestArgType| and |objArgType| have the same size and are pointers. Is there a way of structurally comparing two CType to ensure that I only check that |objBestArgType| is a pointer only if it is not equal to |objArgType|?
Comment 7 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-07 00:29:58 PDT
Created attachment 621522 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

Added more careful checks. Do you think that this is sufficient?
Comment 8 Jason Orendorff [:jorendorff] 2012-05-07 07:59:46 PDT
Comment on attachment 621522 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

Review of attachment 621522 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.

You might consider adding a test for a case where the type of the CData object is clearly wrong. It should throw TypeError.

::: js/src/ctypes/CTypes.cpp
@@ +6629,5 @@
> +      objBestArgType = CData::GetCType(objData);
> +      size_t sizeBestArg;
> +      if (!CType::GetSafeSize(objBestArgType, &sizeBestArg)) {
> +        return TypeError(cx, "(an object with known size)", valData);
> +      }

You can add a MOZ_UNREACHED here. We only create CData objects of types that have a size.

The only types without sizes are void (for use as a return type) and array types with unspecified length (for use as argument types); there are no CData objects of either type.
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-07 08:59:16 PDT
Created attachment 621623 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct

Added a test, replaced exception with assertion. Thanks for the quick review!
Comment 10 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-09 01:34:06 PDT
Created attachment 622315 [details] [diff] [review]
Recording a more precise type in CDataFinalizer::Construct
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-05-09 16:03:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d04efcf7237e
Comment 12 Ed Morley [:emorley] 2012-05-10 08:01:12 PDT
https://hg.mozilla.org/mozilla-central/rev/d04efcf7237e

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