Last Comment Bug 741041 - Wrapper hygiene for typed arrays and friends
: Wrapper hygiene for typed arrays and friends
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Steve Fink [:sfink] [:s:]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 741040 758398 760904 786903
Blocks: 755122
  Show dependency treegraph
 
Reported: 2012-03-30 21:39 PDT by Steve Fink [:sfink] [:s:]
Modified: 2012-08-30 06:26 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement isProxy testing command for JS shell (2.17 KB, patch)
2012-03-30 21:39 PDT, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Splinter Review
Implement isSameCompartment JS shell command for tests (2.20 KB, patch)
2012-03-30 21:40 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
Use UnwrapObjectChecked instead of UnwrapObject wherever applicable, and ensure that array buffer views and their array buffers are in the same compartment (26.70 KB, patch)
2012-03-30 21:40 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
patch 1 - Use UnwrapObjectChecked instead of UnwrapObject wherever applicable (21.71 KB, patch)
2012-04-25 16:03 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
patch 2 - Ensure array buffer views and their buffers are in the same compartment (15.93 KB, patch)
2012-04-25 16:04 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
Use UnwrapObjectChecked instead of UnwrapObject wherever applicable (21.84 KB, patch)
2012-05-11 14:38 PDT, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Splinter Review
Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment (16.21 KB, patch)
2012-05-11 14:39 PDT, Steve Fink [:sfink] [:s:]
luke: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2012-03-30 21:39:22 PDT
Use UnwrapObjectChecked where possible, and ensure that array buffers and their views are in the same compartment: when creating a view, it should be created in the same compartment as the array buffer and then wrapped for the calling compartment
Comment 1 Steve Fink [:sfink] [:s:] 2012-03-30 21:39:29 PDT
Created attachment 611128 [details] [diff] [review]
Implement isProxy testing command for JS shell
Comment 2 Steve Fink [:sfink] [:s:] 2012-03-30 21:40:22 PDT
Created attachment 611129 [details] [diff] [review]
Implement isSameCompartment JS shell command for tests

Shell test function for checking if the fully unwrapped form of two objects are in the same compartment
Comment 3 Steve Fink [:sfink] [:s:] 2012-03-30 21:40:45 PDT
Created attachment 611130 [details] [diff] [review]
Use UnwrapObjectChecked instead of UnwrapObject wherever applicable, and ensure that array buffer views and their array buffers are in the same compartment
Comment 4 Steve Fink [:sfink] [:s:] 2012-03-30 21:41:20 PDT
I'm not sure if  it should be isProxy or isWrapper.
Comment 5 Luke Wagner [:luke] 2012-04-01 22:26:41 PDT
Comment on attachment 611128 [details] [diff] [review]
Implement isProxy testing command for JS shell

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

::: js/src/builtin/TestingFunctions.cpp
@@ +134,5 @@
> +        JS_ReportError(cx, "the function takes exactly one argument");
> +        return false;
> +    }
> +    if (!args[0].isObject()) {
> +        args.rval().setBoolean(true);

Perhaps setBoolean(false) ?
Comment 6 Luke Wagner [:luke] 2012-04-01 22:38:52 PDT
Comment on attachment 611129 [details] [diff] [review]
Implement isSameCompartment JS shell command for tests

The semantics of this function are a bit twitchy.  What about instead phrasing the test in terms of globals (which will hopefully soon have the same meaning as "compartment") by adding a "getGlobalForObject(o)" which returns JS_GetGlobalForObject?  Then you could assert things in terms of globals.

This just made me realize something about these typed array changes, though: they change visible semantics by changing the global of the typed array.  Conceivably, this breaks 'instanceof' and I think implicit 'this'?  I suppose this could work by wrapping the correct prototype (from the correct global).
Comment 7 Steve Fink [:sfink] [:s:] 2012-04-25 16:02:26 PDT
(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 611129 [details] [diff] [review]
> Implement isSameCompartment JS shell command for tests
> 
> The semantics of this function are a bit twitchy.  What about instead
> phrasing the test in terms of globals (which will hopefully soon have the
> same meaning as "compartment") by adding a "getGlobalForObject(o)" which
> returns JS_GetGlobalForObject?  Then you could assert things in terms of
> globals.

That's a good idea. On further thought, though, I don't think this test is necessary. I have asserts in the code that would trigger if it were possible to create a cross-compartment typed array. So I think I'll just scrap this.

> This just made me realize something about these typed array changes, though:
> they change visible semantics by changing the global of the typed array. 
> Conceivably, this breaks 'instanceof' and I think implicit 'this'?  I
> suppose this could work by wrapping the correct prototype (from the correct
> global).

Uploading a new pair of patches, one to do everything but the cross-compartment case, and one to set the proto to a wrapper of the proto from the calling compartment. I won't request review until I land some other patches, though.
Comment 8 Steve Fink [:sfink] [:s:] 2012-04-25 16:03:47 PDT
Created attachment 618474 [details] [diff] [review]
patch 1 - Use UnwrapObjectChecked instead of UnwrapObject wherever applicable
Comment 9 Steve Fink [:sfink] [:s:] 2012-04-25 16:04:32 PDT
Created attachment 618475 [details] [diff] [review]
patch 2 - Ensure array buffer views and their buffers are in the same compartment
Comment 10 Steve Fink [:sfink] [:s:] 2012-05-11 14:38:15 PDT
Created attachment 623306 [details] [diff] [review]
Use UnwrapObjectChecked instead of UnwrapObject wherever applicable
Comment 11 Steve Fink [:sfink] [:s:] 2012-05-11 14:39:42 PDT
Created attachment 623309 [details] [diff] [review]
Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment
Comment 12 Luke Wagner [:luke] 2012-05-11 15:17:02 PDT
Comment on attachment 623306 [details] [diff] [review]
Use UnwrapObjectChecked instead of UnwrapObject wherever applicable

>+js_NewTypedArray(JSContext *cx, uint32_t atype, int32_t nelements);

I don't see any use/definition of this...
Comment 13 Luke Wagner [:luke] 2012-05-11 15:31:43 PDT
Comment on attachment 623309 [details] [diff] [review]
Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment

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

This is really pretty clean, nice job.

::: js/src/jstypedarray.cpp
@@ +1537,5 @@
> +    // compartment. constructWrapped() is invoked in the ArrayBuffer's
> +    // compartment, and is passed in a (wrapped) prototype object from the
> +    // origin compartment to use as the newly-created view's proto.
> +    static JSBool
> +    constructWrapped(JSContext *cx, unsigned argc, Value *vp)

How about just naming this fromBuffer (overload resolution should pick the right one for Proxy::nativeCall) and dropping all mention of wrappers since, technically, this is just a native that calls fromBuffer ; nothing wrapper-y about it.  Instead, the comment can go next to...

@@ +1726,5 @@
> +        if (bufobj->isWrapper()) {
> +            JSObject *wrapped = UnwrapObjectChecked(cx, bufobj);
> +            if (!wrapped)
> +                return NULL;
> +            if (wrapped->isArrayBuffer()) {

... this piece of code.  Now, instead of explicit wrapper testing, could you add a new ESClass so that this could be written:

  /*
   * Normally, NonGenericMethodGuard handles the case of transparent wrappers.
   * However, we have a peculiar situation: we want to construct the new typed
   * array in the compartment of the buffer. This is necessary since typed arrays
   * need to point directly at their buffer's data. Instead, we use the underlying
   * machinery directly to proxy the native call.
   *
  if (!ObjectClassIs(cx, *bufobj, ESClass_ArrayBuffer, cx)) {
      JS_ReportErrorNumber(cx, ...);
      return NULL;
  }

  JS_ASSERT(bufobj->isArrayBuffer() || bufobj->isProxy());
  if (bufobj->isProxy()) {
     ... Proxy::nativeCall ...
  }

  ... normal path

@@ +1741,5 @@
> +
> +                CallArgs args = CallArgsFromVp(argc, argv);
> +                if (!Proxy::nativeCall(cx, bufobj, &ArrayBufferClass, constructWrapped, args))
> +                    return NULL;
> +                return args.rval().toObjectOrNull();

Can this be &args.rval().toObject()?

@@ +2301,5 @@
>      return true;
>  }
>  
>  JSBool
> +DataViewObject::constructWrapped(JSContext *cx, unsigned argc, Value *vp)

Same comments as above, mutatis mutandis.
Comment 14 Luke Wagner [:luke] 2012-05-11 16:40:30 PDT
Comment on attachment 623309 [details] [diff] [review]
Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment

r+ with the UnwrapObject's replaced with ObjectClassIs as mentioned above.
Comment 15 Steve Fink [:sfink] [:s:] 2012-05-16 12:42:11 PDT
*** Bug 734215 has been marked as a duplicate of this bug. ***
Comment 16 Steve Fink [:sfink] [:s:] 2012-05-16 13:34:57 PDT
Requested changes made. I landed the two non-test patches merged because it was easier to do the update for RootedVarObject&friends that way.

https://hg.mozilla.org/integration/mozilla-inbound/rev/df84190b1c0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/73b380d3edd8
Comment 18 David Mandelin [:dmandelin] 2012-05-24 10:30:22 PDT
Marking assignee according to patch author.

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