Closed
Bug 947014
Opened 11 years ago
Closed 10 years ago
Allow Wrapper::New to take a prototype
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: efaust, Assigned: efaust)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
4.32 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
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 1•11 years ago
|
||
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•11 years ago
|
||
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 3•11 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6395f80b24f
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f6395f80b24f https://hg.mozilla.org/mozilla-central/rev/4e40f5a0bd94
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•