Closed Bug 741041 Opened 9 years ago Closed 9 years ago

Wrapper hygiene for typed arrays and friends

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(3 files, 4 obsolete files)

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
Shell test function for checking if the fully unwrapped form of two objects are in the same compartment
Attachment #611129 - Flags: review?(luke)
I'm not sure if  it should be isProxy or isWrapper.
Depends on: 741040
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) ?
Attachment #611128 - Flags: review?(luke) → review+
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).
Attachment #611129 - Flags: review?(luke)
Attachment #611130 - Flags: review?(luke)
(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.
Assignee: sphink → nobody
Component: JavaScript Engine → Keyboard: Navigation
QA Contact: general → keyboard.navigation
Target Milestone: --- → mozilla14
Version: unspecified → Trunk
Assignee: nobody → sphink
Attachment #611129 - Attachment is obsolete: true
Attachment #611130 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee: sphink → general
Component: Keyboard: Navigation → JavaScript Engine
QA Contact: keyboard.navigation → general
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...
Attachment #623306 - Flags: review?(luke) → review+
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 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.
Attachment #623309 - Flags: review?(luke) → review+
Attachment #618474 - Attachment is obsolete: true
Attachment #618475 - Attachment is obsolete: true
Duplicate of this bug: 734215
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
Target Milestone: mozilla14 → mozilla15
Depends on: 757682
Marking assignee according to patch author.
Assignee: general → sphink
No longer depends on: 757682
Depends on: 760904
Depends on: 786903
You need to log in before you can comment on or make changes to this bug.