947014 - Allow Wrapper::New to take a prototype

Status: RESOLVED FIXED

Description

[Eric Faust [:efaust]]

Created attachment 8343422 [details] [diff] [review]
wrapperProto.patch

As a nit in bug 926012, Bobby was unhappy with the WrapperWithProto::New duplicating, rather than leveraging the code in Wrapper::New.

Attached is a patch which will rectify that problem, on top of the machinery introduced later in bug 924720. I didn't know where exactly to put it, since it has so many dependencies on code that hasn't landed yet, but here we are...

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 8343422 [details] [diff] [review]
wrapperProto.patch

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

::: js/src/jsproxy.h
@@ +437,5 @@
> +     *             the stack, it's easy to make a GC mistake. WrapperOptions
> +     *             makes no guarantees about marking this saved proto pointer.
> +     *             Since Wrapper::New guarantees that there are no GCing calls
> +     *             between its invocation and NewProxyObject(), this should be
> +     *             relatively safe. Just...be careful out there, OK?

Erm, not so happy about this. Waiting for the static analysis to catch changes in Wrapper::New isn't great, because the poor programmer won't find out until {s}he pushes.

Let's add an additional Wrapper constructor that takes a JSContext, and uses to construct proto_, which is a Maybe<RootedObject>.

setProto will then assert that the these WrapperOptions were constructed with a cx, and thus are "object ready".

Comment 2

[Eric Faust [:efaust]]

Created attachment 8343490 [details] [diff] [review]
Now with less footgun

Implement the suggestion above.

Note that WrapperOption(cx) construction sites will have to setStandardClass() for themselves, but that's OK, as it's the same behavior as NewProxyObject()

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8343490 [details] [diff] [review]
Now with less footgun

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

Thanks for doing this. r=bholley with comments addressed.

::: js/src/jsproxy.h
@@ +443,5 @@
>  
>  class MOZ_STACK_CLASS WrapperOptions : public ProxyOptions {
>    public:
> +    WrapperOptions() : ProxyOptions(false, nullptr),
> +                       proto_()

I don't understand why you need the proto_(), here and below.

@@ +448,3 @@
>      {}
> +
> +    WrapperOptions(JSContext *cx) : ProxyOptions(false, nullptr),

Please provide ample documentation for this constructor and the setProto accessor explaining the situation.

@@ +456,5 @@
> +    JSObject *proto() const {
> +        return proto_.empty() ? js::Proxy::LazyProto : proto_.ref();
> +    }
> +    WrapperOptions &setProto(JSObject *argProto) {
> +        proto_.ref() = argProto;

I know it's implicit here, but let's explicitly MOZ_ASSERT(!proto_.empty()) first.

Comment 4

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6395f80b24f

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f6395f80b24f
https://hg.mozilla.org/mozilla-central/rev/4e40f5a0bd94