Closed
Bug 832299
Opened 11 years ago
Closed 11 years ago
Handlify JSCompartment::wrap
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(1 file, 1 obsolete file)
35.35 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
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.
Attachment #704069 -
Flags: review?(terrence)
Comment 2•11 years ago
|
||
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.
Attachment #704069 -
Flags: review?(terrence)
Assignee | ||
Comment 3•11 years ago
|
||
I still need to ask some question about the possible const& arguments for wrappers. Some of your suggested changes made this much nicer!
Attachment #704069 -
Attachment is obsolete: true
Attachment #711551 -
Flags: review?(terrence)
Comment 5•11 years ago
|
||
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.
Attachment #711551 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Awesome. https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed6ca2ee354
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2ed6ca2ee354
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•