Last Comment Bug 705019 - CTypes prototype hierarchy should behave more realistically
: CTypes prototype hierarchy should behave more realistically
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-23 17:05 PST by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2011-11-30 03:59 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1 - Introduce CType::IsCTypeProto() and CData::isCDataProto(). v1 (3.30 KB, patch)
2011-11-23 17:15 PST, Bobby Holley (:bholley) (busy with Stylo)
jorendorff: review+
Details | Diff | Splinter Review
part 2 - Make toSource() and toString() work on sCTypeProtoClass and sCDataProtoClass objects. v1 (6.58 KB, patch)
2011-11-23 17:16 PST, Bobby Holley (:bholley) (busy with Stylo)
jorendorff: review+
Details | Diff | Splinter Review
part 3 - Allow access to {C,Pointer,Struct,Array,Function}Type.prototype.prototype. v1 (4.44 KB, patch)
2011-11-23 17:17 PST, Bobby Holley (:bholley) (busy with Stylo)
jorendorff: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2011-11-23 17:05:32 PST
Over in bug 704147, jorendorff suggested (via me) doing FunctionType.prototype.prototype.call = Function.prototype.call.

This broke for two reasons:
1 - We don't support toString or toSource on CTypeProto and CDataProto objects, meaning that the web console would hit an exception every time it tried to print the result. This is arguably a red herring, but nasty nonetheless.

2 - We don't support accessing the 'prototype' property of CTypeProto objects. This loses us some transparency, and generally represents a hole in our otherwise impeccable object graph.

I just wrote some patches to make this all work. Coming right up.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2011-11-23 17:15:44 PST
Created attachment 576659 [details] [diff] [review]
part 1 - Introduce CType::IsCTypeProto() and CData::isCDataProto(). v1
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2011-11-23 17:16:23 PST
Created attachment 576660 [details] [diff] [review]
part 2 - Make toSource() and toString() work on sCTypeProtoClass and sCDataProtoClass objects. v1
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2011-11-23 17:17:07 PST
Created attachment 576661 [details] [diff] [review]
part 3 - Allow access to {C,Pointer,Struct,Array,Function}Type.prototype.prototype. v1
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2011-11-23 17:21:48 PST
Gah. My git-to-bugzilla patch uploader broke with a recent bugzilla change, so I just manually generated and uploaded these patches. But, I forgot to do -U8.

Rather than doing a bunch of churn, here's the link to the patches on github in case you need any more context: https://github.com/bholley/mozilla-central/commits/ctypeproto
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2011-11-23 17:26:08 PST
Also, the reason I sunk time into this today was because I thought it would be good to support jorendorff's little prototype.prototype hack, a la bug 704147. But I just remembered that we actually share those objects across compartments, and thus don't want people to modify the prototypes. The only reason they're modifiable right now is that the Freeze call is commented out:

http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#697

So this patch is less useful, but what the heck.

Also, it looks like the bug blocking the Freeze call has been fixed. Maybe we should take care of that while we're at it?
Comment 6 Jason Orendorff [:jorendorff] 2011-11-23 18:54:24 PST
Comment on attachment 576660 [details] [diff] [review]
part 2 - Make toSource() and toString() work on sCTypeProtoClass and sCDataProtoClass objects. v1

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

Looks good. r=me.

::: js/src/ctypes/CTypes.cpp
@@ +3159,5 @@
>    JSObject* obj = JS_THIS_OBJECT(cx, vp);
> +  if (!obj ||
> +      !(CType::IsCType(cx, obj) || CType::IsCTypeProto(cx, obj)))
> +  {
> +    JS_ReportError(cx, "not a CType or a CTypeProto");

Please change all three error messages back. The user isn't expected to know what a CTypeProto is, so the error message "not a CType" is better.

::: toolkit/components/ctypes/tests/unit/test_jsctypes.js.in
@@ +299,5 @@
> +
> +  // toString and toSource are called by the web console during inspection,
> +  // so we don't want them to throw.
> +  do_check_eq(ctypes.CType.prototype.toString(), '[CType proto object]');
> +  do_check_eq(ctypes.CType.prototype.toSource(), '[CType proto object]');

When I make a change like this where I really don't care what the output is, as long as the function doesn't fall over and die, I generally don't make the test check for the specific arbitrarily chosen behavior I implemented--so for example, this could just check that these methods return strings. Just a thought.
Comment 7 Jason Orendorff [:jorendorff] 2011-11-28 14:31:25 PST
Comment on attachment 576661 [details] [diff] [review]
part 3 - Allow access to {C,Pointer,Struct,Array,Function}Type.prototype.prototype. v1

In the test, please use Object.getPrototypeOf(obj) instead of obj.__proto__.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2011-11-29 12:18:56 PST
Addressed jorendorff's comments and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=90e36f26720a
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2011-11-29 18:32:20 PST
This had a bit of a rocky try run, but all of the failures seemed to be known intermittent (maybe the machine was bogged or something)?

Anyway, appears to be OK. Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c716afd37a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1488f748ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2b7e489b20

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