Closed
Bug 947014
Opened 12 years ago
Closed 11 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•12 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•12 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•12 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•11 years ago
|
||
Comment 5•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f6395f80b24f
https://hg.mozilla.org/mozilla-central/rev/4e40f5a0bd94
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Updated•11 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•