It can only return an object, in practice. Some callers probably find it convenient that the outparam is a Value, since they can pass args.rval(). ProxyHandler::construct()'s arguments include a CallArgs, so the result is necessarily stored in a Value. OTOH at least two callers create a temporary RootedValue for the outparam and then immediately do v.toObject() without checking. Given these conflicting needs, it seems best to just use the actual type.

Please review the patch.!!

Comment on attachment 8678075 [details] [diff] [review] Changed the out-param of js/JS::Construct from MutableHandleValue to MutableHandleObject Review of attachment 8678075 [details] [diff] [review]: ----------------------------------------------------------------- As talked in IRC, clearing r? for now.

Comment on attachment 8678925 [details] [diff] [review] Bug1212533.diff Review of attachment 8678925 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for your patch! :D Looks like this patch is based in Attachment #8678075 [details] [diff], please merge these 2 patches into single patch. Also, please address following comments and post the patch again :) ::: js/src/builtin/Reflect.cpp @@ +88,5 @@ > static bool > Reflect_construct(JSContext* cx, unsigned argc, Value* vp) > { > CallArgs args = CallArgsFromVp(argc, vp); > + RootedObject obj(cx); Please move this under "// Step 6." ::: js/src/jit/VMFunctions.cpp @@ +84,5 @@ > > // If |this| hasn't been created, we can use normal construction code. > + if (thisv.isMagic(JS_IS_CONSTRUCTING)) { > + RootedObject obj(cx); > + if (!Construct(cx, fval, cargs, newTarget, &obj)) Please remove trailing space here. ::: js/src/jsapi.cpp @@ +3040,5 @@ > } > > JS_PUBLIC_API(bool) > JS::Construct(JSContext* cx, HandleValue fval, HandleObject newTarget, const JS::HandleValueArray& args, > + MutableHandleObject rval) now rval's type is not Value but Object, so it would be nice to change the name (both in .cpp and .h). looks like |objp| is mostly used in jsapi.cpp/h ::: js/src/jsfun.cpp @@ +1514,5 @@ > bool > js::CallOrConstructBoundFunction(JSContext* cx, unsigned argc, Value* vp) > { > CallArgs args = CallArgsFromVp(argc, vp); > + RootedObject obj(cx); Please move this just before |if (!Construct(cx, ...|. ::: js/src/proxy/DirectProxyHandler.cpp @@ +94,5 @@ > > + RootedObject obj(cx); > + if (!Construct(cx, target, cargs, args.newTarget(), &obj)) > + return false; > + Please remove spaces here. ::: js/src/proxy/ScriptedIndirectProxyHandler.cpp @@ +484,5 @@ > + if (!Construct(cx, construct, cargs, args.newTarget(), &obj)) > + return false; > + > + args.rval().setObject(*obj); > + return true; Please remove trailing space. ::: js/src/vm/Interpreter.cpp @@ +907,2 @@ > js::Construct(JSContext* cx, HandleValue fval, const ConstructArgs& args, HandleValue newTarget, > MutableHandleObject rval) |rval| here should also be renamed. @@ -912,5 @@ > args.newTarget().set(newTarget); > if (!InternalConstruct(cx, args)) > return false; > - > - rval.set(args.rval()); Please return args.rval().toObject() via outparam. Also, asserting args.rval().isObject() would be nice. @@ +4838,4 @@ > return false; > + > + res.setObject(*obj); > + Please remove a blank line here. @@ +5129,5 @@ > > ConstructArgs constArgs(cx); > if (!FillArgumentsFromArraylike(cx, constArgs, args)) > return false; > + Please remove trailing spaces here.

Please provide feedback on the patch :)

Comment on attachment 8679312 [details] [diff] [review] 1212533.diff Review of attachment 8679312 [details] [diff] [review]: ----------------------------------------------------------------- Great, almost there :D Only some styling fixes are needed. ::: js/src/jit/VMFunctions.cpp @@ +91,5 @@ > + rval.setObject(*obj); > + return true; > + } > + > + Please remove above 2 empty lines. sorry, I've overlooked them last time. ::: js/src/proxy/DirectProxyHandler.cpp @@ +94,5 @@ > > + RootedObject obj(cx); > + if (!Construct(cx, target, cargs, args.newTarget(), &obj)) > + return false; > + whitespaces are still there ;) please remove them. ::: js/src/vm/Interpreter.cpp @@ +4841,3 @@ > return false; > + res.setObject(*obj); > + please remove a blank line after |res.setObject| @@ +5131,5 @@ > > ConstructArgs constArgs(cx); > if (!FillArgumentsFromArraylike(cx, constArgs, args)) > return false; > + remove whitespaces ::: js/src/vm/Interpreter.h @@ +94,5 @@ > // NOTE: As with the ES6 spec operation, it's the caller's responsibility to > // ensure |fval| and |newTarget| are both |IsConstructor|. > extern bool > Construct(JSContext* cx, HandleValue fval, const ConstructArgs& args, HandleValue newTarget, > + MutableHandleObject rval); s/rval/objp/

Hi, hope there no whitespaces now :)

Please review the patch :)

Comment on attachment 8679427 [details] [diff] [review] Bug1212533.patch Review of attachment 8679427 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for rebasing :) I overlooked an error, sorry. r+ with the fix. After following fix, please ask review from jorendorff, as this patch changes public API. ::: js/src/vm/Interpreter.cpp @@ +616,5 @@ > if (!InternalConstruct(cx, args)) > return false; > > + MOZ_ASSERT(args.rval().isObject()); > + objp.set(args.rval().toObject()); this needs "&" before |args.rval().toObject()|. |.set| accepts |JSObject*| but |.toObject()| returns |JSObject&|

Please review the patch. Thanks :)

pushed to try and it hits compile error https://treeherder.mozilla.org/#/jobs?repo=try&revision=cba596c4dd39 looks like we missed following 2 JS::Construct callsites: https://dxr.mozilla.org/mozilla-central/rev/6c7c983bce46a460c2766fbdd73283f6d2b03a69/js/ipc/WrapperAnswer.cpp#430 https://dxr.mozilla.org/mozilla-central/rev/6c7c983bce46a460c2766fbdd73283f6d2b03a69/js/xpconnect/src/ExportHelpers.cpp#348

Please review the patch :)

Comment on attachment 8679971 [details] [diff] [review] bug1212533.patch Review of attachment 8679971 [details] [diff] [review]: ----------------------------------------------------------------- thanks :)

Comment on attachment 8679434 [details] [diff] [review] Bug1212533.patch This patch is obsoleted by the other one, right? I'll review the other one.

Comment on attachment 8679971 [details] [diff] [review] bug1212533.patch Review of attachment 8679971 [details] [diff] [review]: ----------------------------------------------------------------- Great. This is not as pretty as I hoped, but it's not your fault! We'll take it. Types are good. ::: js/src/vm/Interpreter.cpp @@ +615,5 @@ > args.newTarget().set(newTarget); > if (!InternalConstruct(cx, args)) > return false; > > + MOZ_ASSERT(args.rval().isObject()); Please delete this assertion. It duplicates an assertion inside Value::toObject().

Hope this looks good now :)

Comment on attachment 8711563 [details] [diff] [review] 1212533.patch Ugh, the previously-reviewed patch got dropt on the floor by us rather than landed with no further effort required on your part. :-( As penance for that, I've imported it locally and made it work against inbound again, and I'll land it tomorrow -- sorry for duplicating your work! (Skimming your patch, I see the changes I had to make -- except for one to vm/SelfHosting.cpp, updating its use of Construct. I don't know if my tree's out of date or if yours is -- just noting that at least one of our patches is definitely not right against the very latest tree. No matter which, just figured I'd note it for educational purposes if indeed yours is wrong -- and if my tree just happens to be too old, I'll find out when I rebase my fixed-up import of your patch against tippy-top inbound.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b54b8458bf70534cc442284f27ee43b877c996 Backed out changeset 66d4205c2958 (bug 1212533) for build bustage ON A CLOSED TREE

If I had to guess, previous patchwork here was tested only by building the JS shell (i.e. autoconf/configure/make all in js/src), and the full browser wasn't built (./mach build at top level). Turns out the latter busts. :-) (I can neither confirm nor deny that locally I tested by building only the JS shell and not the full browser...) Both of us should have been more careful to build the browser as well, it would appear. Oh well. I'd have fixed it in situ, but I have no business being up and patching stuff right now. :-) Will deal in the morning with a stupid spot-fix. (But it looks like mozilla::dom::Promise::PromiseCapability::mPromise really can/should be JSObject* now, not JS::Value, so I will probably do my own mini-cleanup atop that stupid spot-fix to clean things up very slightly.)