832299 - Handlify JSCompartment::wrap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S [:evilpie]]

Attachment: WIP

This just roots the Value version of ::wrap. I still need to do the other versions. I am unsure about quite a lot of stuff. Mostly related to *argv arrays. It might be useful to use more handles in Debugger before finishing this patch.

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 704069 [details] [diff] [review]
WIP

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

The bits in jswrapper.cpp look like they are going to be majorly performance sensitive. Once you've cleaned this up, build a browser (shell doesn't really use compartments) and run it against all the benchmarks you can think of. Also be sure and do a full Talos run so that we find out about any regressions it will see /before/ pushing.

::: js/src/jit-test/tests/debug/Debugger-multi-01.js
@@ +15,5 @@
>  addDebug('c');
>  
>  log = '';
> +//assertEq(g.eval("debugger; 0;"), 0);
> +//assertEq(log, 'abc');

Why did you need to remove these tests?

::: js/src/jscompartment.cpp
@@ +250,5 @@
>      return crossCompartmentWrappers.put(wrapped, wrapper);
>  }
>  
>  bool
> +JSCompartment::wrap(JSContext *cx, MutableHandleValue value, HandleObject existing_)

|existing_| should be |existingArg|.

@@ +434,5 @@
>      return true;
>  }
>  
>  bool
> +JSCompartment::wrap(JSContext *cx, JSObject **objp, JSObject *existing_)

|existingArg|

::: js/src/jscompartment.h
@@ +407,5 @@
>  
>      /* Mark cross-compartment wrappers. */
>      void markCrossCompartmentWrappers(JSTracer *trc);
>  
> +    bool wrap(JSContext *cx, js::MutableHandleValue value);

MutableHandleValue is part of the public API, so this should be JS::MutableHandleValue. The fact that it's also available in |js::| is an artifact of pre-existing badness.

Also, keep the name as |vp|: it is the standard name for outparam Values.

@@ +408,5 @@
>      /* Mark cross-compartment wrappers. */
>      void markCrossCompartmentWrappers(JSTracer *trc);
>  
> +    bool wrap(JSContext *cx, js::MutableHandleValue value);
> +    bool wrap(JSContext *cx, js::MutableHandleValue value, js::HandleObject existing);

You should be able to do ...JS::HandleObject existing = JS::NullPtr()); and avoid the wrapper.

::: js/src/jsobj.cpp
@@ +1647,5 @@
>      }
>  
>      size_t span = JSCLASS_RESERVED_SLOTS(from->getClass());
>      for (; n < span; ++n) {
> +        RootedValue v(cx, from->getSlot(n));

Move |RootedValue v(cx);| above the loop head.

::: js/src/jswrapper.cpp
@@ +489,5 @@
>  }
>  
>  bool
>  CrossCompartmentWrapper::get(JSContext *cx, JSObject *wrapperArg, JSObject *receiverArg,
> +                             jsid idArg, Value *valueArg)

|vpArg|

@@ +494,5 @@
>  {
>      RootedObject wrapper(cx, wrapperArg);
>      RootedObject receiver(cx, receiverArg);
>      RootedId id(cx, idArg);
> +    RootedValue value(cx, *valueArg);

|vp|

@@ +504,5 @@
>  }
>  
>  bool
>  CrossCompartmentWrapper::set(JSContext *cx, JSObject *wrapper_, JSObject *receiver_, jsid id_,
> +                             bool strict, Value *valueArg)

Hmm, in this method, valueArg isn't written, so valueArg is good. I'm worried now that a previous rooting refactor may have broken this subtly. Please find someone that knows what the semantics of this method should be and find out why this isn't |const Value &|.

@@ +605,5 @@
>      return true;
>  }
>  
>  bool
> +CrossCompartmentWrapper::iterate(JSContext *cx, JSObject *wrapper, unsigned flags, Value *valueArg)

|vpArg|

@@ +610,2 @@
>  {
> +    RootedValue value(cx, *valueArg);

|vp|

@@ +614,5 @@
> +           Wrapper::iterate(cx, wrapper, flags, value.address()),
> +           CanReify(value.address()) ? Reify(cx, cx->compartment, value.address())
> +            : cx->compartment->wrap(cx, &value));
> +    // do we need to put value back?
> +    *valueArg = value;

Probably. In /either/ case, in a large refactoring update it's best to aggressively preserve the existing semantics. Feel free to open up a different bug if you want to investigate making |vp| a |const Value &|.

@@ +619,4 @@
>  }
>  
>  bool
>  CrossCompartmentWrapper::call(JSContext *cx, JSObject *wrapper_, unsigned argc, Value *vp)

Normally this would be argc/argv, but it looks like they are using the existing stack |vp| in-place as the stack for the called function to avoid the copy. Super-gross.

@@ +626,3 @@
>      {
>          AutoCompartment call(cx, wrapped);
> +        // XXX this stuff, maybe fromMarkedLocation?

Actually, make argc/vp a CallArgs at the method head and add a MutableHandleValue accessor for use with ::wrap.

@@ +641,5 @@
>          if (!Wrapper::call(cx, wrapper, argc, vp))
>              return false;
>      }
> +    RootedValue rval(cx, *vp);
> +    bool ok = cx->compartment->wrap(cx, &rval);

Add an |args.mutableThisv()|.

@@ +689,5 @@
>          Value *src = srcArgs.base();
>          Value *srcend = srcArgs.array() + srcArgs.length();
>          Value *dst = dstArgs.base();
>          for (; src < srcend; ++src, ++dst) {
> +            RootedValue source(cx, *src);

Hoist |source|.

@@ +751,5 @@
>      return Wrapper::regexp_toShared(cx, wrapper, g);
>  }
>  
>  bool
> +CrossCompartmentWrapper::defaultValue(JSContext *cx, JSObject *wrapper, JSType hint, Value *valueArg)

|vpArg|  It may be worth checking to see if it would be easier to make |vp| a MutableHandleValue for this method.

@@ +758,3 @@
>          return false;
> +
> +    RootedValue value(cx, *valueArg);

|vp|

::: js/src/shell/jsheaptools.cpp
@@ +482,5 @@
>  
>  bool
>  ReferenceFinder::addReferrer(jsval referrer_, Path *path)
>  {
> +    RootedValue referrer(context, referrer_);

Change this to |referrerArg|.

::: js/src/vm/Debugger.cpp
@@ +882,5 @@
>          JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_DEBUG_BAD_RESUMPTION);
>          return handleUncaughtException(ac, vp, callHook);
>      }
>  
> +    // XX not sure here

What you have seems to preserve the existing semantics.

@@ +4497,3 @@
>          return false;
>      for (unsigned i = 0; i < callArgc; i++) {
> +        // XXX look for better solution

What you have seems okay for now.

Comment 3

[Tom S [:evilpie]]

Attachment: v1

I still need to ask some question about the possible const& arguments for wrappers. Some of your suggested changes made this much nicer!

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 711551 [details] [diff] [review]
v1

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

Awesome work!

::: js/src/jit-test/tests/debug/Debugger-multi-01.js
@@ +15,5 @@
>  addDebug('c');
>  
>  log = '';
> +//assertEq(g.eval("debugger; 0;"), 0);
> +//assertEq(log, 'abc');

I'm guessing this is a stray hunk.

::: js/src/jswrapper.cpp
@@ +610,5 @@
> +    }
> +
> +    bool ok = CanReify(vp.address())
> +                ? Reify(cx, cx->compartment, &vp)
> +                : cx->compartment->wrap(cx, &vp);

Align |?| and |:| under the 'C' in CanReify.

Comment 6

[Tom S [:evilpie]]

Awesome.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed6ca2ee354

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ed6ca2ee354