Allow Wrapper::New to take a prototype

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla29
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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...
Attachment #8343422 - Flags: review?(bobbyholley+bmo)
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".
Attachment #8343422 - Flags: review?(bobbyholley+bmo) → review-
(Assignee)

Comment 2

5 years ago
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()
Attachment #8343422 - Attachment is obsolete: true
Attachment #8343490 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8343490 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/f6395f80b24f
https://hg.mozilla.org/mozilla-central/rev/4e40f5a0bd94
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.