Closed
Bug 1083211
Opened 10 years ago
Closed 9 years ago
Change implementation of BaseProxyHandler::set() to follow ES6 [[Set]] specification
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
Attachments
(1 file)
9.27 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
Currently, BaseProxyHandler::set follows the obsolete specification of indirect scripted proxies. It works in terms of getOwnPropertyDescriptor, followed by the nonstandard getPropertyDescriptor, followed by a bunch of gunk. This base-class implementation is actually used by XrayWrapper and some other classes. This is the tail wagging the dog. No way this obsolete spec should be driving the semantics of wrappers. If we were designing this today, BPH::set would just follow the same algorithm that's specified for ordinary objects, ES6 9.1.9. This much works; I've try server'd it here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dc770becda77 More controversially I'd like to make the same changes to Proxy::set's mHasPrototype case--ideally, making Proxy::set finish by just calling: return handler->BaseProxyHandler::set(...);
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5ff0c763ba9d
Assignee | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f9dfd86e4f68
Assignee | ||
Comment 3•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=c8f94f753069
Assignee | ||
Comment 4•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=be1ef1e08650
Assignee | ||
Comment 5•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=be1ef1e08650 is actually all good. The orange there is also in the inbound parent https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=e655fbbc6e9f and was fixed by a later commit.
Assignee | ||
Comment 6•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=98658c9c0a1d
Assignee | ||
Comment 7•10 years ago
|
||
Part 1 - Change BaseProxyHandler::set() to follow ES6 [[Set]] specification Part 2 - Change the mHasPrototype case in Proxy::set() to follow ES6 [[Set]] specification (by making it a call to BaseProxyHandler::set()) Part 1 works. Part 2 is a little ambitious; I suspect try server is not going to like that one at first. I think it's necessary though. My patches in bug 1090636 cause the correct 'receiver' argument to be passed to handlers' set() methods for the first time. That shakes out some mind-numbing bugs, which I think might be best fixed by taking this change.
Assignee | ||
Comment 8•10 years ago
|
||
Hmm. It looks like the problems with part 2 are fixed by something in bug 1090636. Dependency cycle, ack! I think I'll try to land part 1 here, and then try to merge part 2 into the other bug in the right place.
Assignee | ||
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f1998a86cbc6
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a8fb45cfc25c
Assignee | ||
Comment 11•10 years ago
|
||
The handlers affected by this change are: SandboxProxyHandler XrayWrapper DeadObjectProxy (but not really) In the near future, I will change Proxy::set() to use this code when mHasPrototype is true. Handlers that do not override set() but nonetheless are not affected: - WindowNamedPropertiesHandler. Not affected yet because hasPrototype=true, so set() is never called. However it's worth thinking about this one. It will be changing to use this code soon. - ScriptedIndirectProxyHandler. This class was the original motivation for the old bad code; its old bad behavior has been preserved (by changing it to override set() with the old code). This is necessary, alas -- there's in-tree code depending on these details of Proxy.create()'s behavior.
Attachment #8522192 -
Flags: review?(bobbyholley)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment 12•10 years ago
|
||
Comment on attachment 8522192 [details] [diff] [review] Reimplement BaseProxyHandler::set from scratch to follow ES6 draft rev 27 9.1.9 Review of attachment 8522192 [details] [diff] [review]: ----------------------------------------------------------------- > alas -- there's in-tree code depending on these details of Proxy.create()'s behavior. Are you talking about the SpecialPowers wrappers? We should be able to do bug 989426 now that the Xray situation is improved. ::: js/src/jsapi.h @@ +3031,5 @@ > bool isIndex() const { return desc()->attrs & JSPROP_INDEX; } > bool hasAttributes(unsigned attrs) const { return desc()->attrs & attrs; } > > + bool isAccessorDescriptor() const { return hasGetterOrSetterObject(); } > + bool isDataDescriptor() const { return !isAccessorDescriptor(); } Add a comment here indicating that descriptors with JSPropertyOps are considered data descriptors. ::: js/src/proxy/BaseProxyHandler.cpp @@ +88,5 @@ > return false; > + > + // Step 4. > + if (!ownDesc.object()) { > + RootedObject parent(cx); Can you call this proto instead? @@ +94,5 @@ > return false; > + if (parent) > + return JSObject::setGeneric(cx, parent, receiver, id, vp, strict); > + ownDesc.clear(); > + ownDesc.setAttributes(JSPROP_ENUMERATE); It's a little unintuitive that clear()ing results in something for which isDataDescriptor() returns true, but I guess that's ok. Maybe comment that we'll fall through into the branch below? @@ +111,5 @@ > + } > + > + // Nonstandard SpiderMonkey special case: setter ops. > + StrictPropertyOp setter = ownDesc.setter(); > + if (setter && setter != JS_StrictPropertyStub) (I seem to remember making this comment somewhere, but can't find it) I thought that we never stuck JS_{,Strict}PropertyStub into JSPropertyDescriptors. Can we just assert against that case? @@ +117,5 @@ > + > + // Steps 5.c-d. Adapt for SpiderMonkey by using HasOwnProperty instead > + // of the standard [[GetOwnProperty]]. > + bool existingDescriptor; > + if (!HasOwnProperty(cx, receiver, id, &existingDescriptor)) Why do we use this and not GetOwnPropertyDescriptor?
Attachment #8522192 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #12) > > alas -- there's in-tree code depending on these details of Proxy.create()'s behavior. > > Are you talking about the SpecialPowers wrappers? We should be able to do > bug 989426 now that the Xray situation is improved. I don't know what in particular. I just remember a bunch of tests failed. Happy to hear about bug 989426. I'm dying to blow away Proxy.create(). > ::: js/src/proxy/BaseProxyHandler.cpp > > + RootedObject parent(cx); > > Can you call this proto instead? Sure. Commented: // The spec calls this variable "parent", but that word has weird // connotations in SpiderMonkey, so let's go with "proto". RootedObject proto(cx); > > + ownDesc.clear(); > > + ownDesc.setAttributes(JSPROP_ENUMERATE); > > It's a little unintuitive that clear()ing results in something for which > isDataDescriptor() returns true, but I guess that's ok. Maybe comment that > we'll fall through into the branch below? That's a good point. Added a comment. But we should change the name of clear(). I've added it to my list. > @@ +111,5 @@ > > + } > > + > > + // Nonstandard SpiderMonkey special case: setter ops. > > + StrictPropertyOp setter = ownDesc.setter(); > > + if (setter && setter != JS_StrictPropertyStub) > > (I seem to remember making this comment somewhere, but can't find it) > > I thought that we never stuck JS_{,Strict}PropertyStub into > JSPropertyDescriptors. Can we just assert against that case? You mentioned it in bug 1097713 comment 3. As a consequence, I wrote a bunch of patches to make this true in bug 1103368, and part 5 of that stack adds the desired assertion: // Nonstandard SpiderMonkey special case: setter ops. StrictPropertyOp setter = ownDesc.setter(); - if (setter && setter != JS_StrictPropertyStub) + MOZ_ASSERT(setter != JS_StrictPropertyStub); + if (setter) return CallSetter(cx, receiver, id, setter, ownDesc.attributes(), strict, vp); So, yay. > > + // Steps 5.c-d. Adapt for SpiderMonkey by using HasOwnProperty instead > > + // of the standard [[GetOwnProperty]]. > > + bool existingDescriptor; > > + if (!HasOwnProperty(cx, receiver, id, &existingDescriptor)) > > Why do we use this and not GetOwnPropertyDescriptor? The only difference is speed. HasOwnProperty is much faster.
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #13) > > > + // Steps 5.c-d. Adapt for SpiderMonkey by using HasOwnProperty instead > > > + // of the standard [[GetOwnProperty]]. > > > + bool existingDescriptor; > > > + if (!HasOwnProperty(cx, receiver, id, &existingDescriptor)) > > > > Why do we use this and not GetOwnPropertyDescriptor? > > The only difference is speed. HasOwnProperty is much faster. The speed may not matter here, I don't know. However I'm definitely using HasOwnProperty when implementing this same algorithm for native objects. Seems best to be consistent.
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c69e27e86565
b2g build bustage https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4173512&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/3670435aed44
Assignee | ||
Comment 17•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=638080573f72
Assignee | ||
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0d1b2727f2
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/551c3cd74dbd
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a0d1b2727f2 https://hg.mozilla.org/mozilla-central/rev/551c3cd74dbd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•