Closed
Bug 828462
Opened 12 years ago
Closed 12 years ago
Exactly root Proxy and Wrapper
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(6 files, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
My current id is to do this trap by trap. Let's see how doable that is.
Assignee | ||
Comment 1•12 years ago
|
||
I had no idea we had so many Wrapper implementations. This roots all getPropertyDescriptor parameters, except PropertyDescriptor. I am not sure if that needs to be rooted at all.
Assignee: general → evilpies
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Blocks: ExactRooting
Assignee | ||
Updated•12 years ago
|
Attachment #699927 -
Flags: review?(terrence)
Comment 2•12 years ago
|
||
Comment on attachment 699927 [details] [diff] [review]
root Proxy::getPropertyDescriptor
Review of attachment 699927 [details] [diff] [review]:
-----------------------------------------------------------------
This looks pretty much perfect, BUT, as I should have mentioned, but we want to preserve the interface until the shell is fully exactly rooted and we have reached performance parity with the conservative scanner.
Lets hold on to this patch for a couple of months.
Attachment #699927 -
Flags: review?(terrence)
Comment 3•12 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #1)
> Created attachment 699927 [details] [diff] [review]
> root Proxy::getPropertyDescriptor
Thanks for doing this! GGC is a big deal for SpiderMonkey.
Comment 4•12 years ago
|
||
Comment on attachment 699927 [details] [diff] [review]
root Proxy::getPropertyDescriptor
Review of attachment 699927 [details] [diff] [review]:
-----------------------------------------------------------------
And a couple of months later we're ready to roll with this. Thanks for waiting!
::: dom/bindings/DOMJSProxyHandler.h
@@ +35,5 @@
> mClass(aClass)
> {
> }
>
> + bool getPropertyDescriptor(JSContext* cx, JS::Handle<JSObject *> proxy, JS::Handle<jsid> id, JSPropertyDescriptor* desc,
I think the browser may have a shorter line wrapping width than SpiderMonkey. You should probably check and make sure this and the changes in DOMJSProxyHandler.cpp don't overflow whatever the local style is.
::: js/xpconnect/src/XPCComponents.cpp
@@ +3190,5 @@
>
> bool
> xpc::SandboxProxyHandler::getOwnPropertyDescriptor(JSContext *cx,
> + JSObject *proxy_,
> + jsid id_,
XPConnect is trying to move to SpiderMonkey style, so these should probably be proxyArg/idArg.
Attachment #699927 -
Flags: review+
Comment 5•12 years ago
|
||
> I think the browser may have a shorter line wrapping width than
> SpiderMonkey.
Yes; it uses 80-char lines. See
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style.
Assignee | ||
Comment 6•12 years ago
|
||
I refreshed the patch locally and only after the fact I noticed that I accidentally also did it for getOwnProperty.
Attachment #699927 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #725794 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #725901 -
Flags: review?(terrence)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #725902 -
Flags: review?(terrence)
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #725903 -
Flags: review?(terrence)
Comment 11•12 years ago
|
||
Comment on attachment 725901 [details] [diff] [review]
getOwnPropertyNames & keys - v1
Review of attachment 725901 [details] [diff] [review]:
-----------------------------------------------------------------
Nice.
Attachment #725901 -
Flags: review?(terrence) → review+
Comment 12•12 years ago
|
||
Comment on attachment 725902 [details] [diff] [review]
defineProperty - v1
Review of attachment 725902 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsobj.cpp
@@ +970,2 @@
> if (obj->isProxy())
> + return Proxy::defineProperty(cx, obj, id, pd);
Move the root under obj->isProxy() so that only proxies have to pay the rooting cost here.
::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +181,5 @@
> JSPropertyDescriptor *desc, unsigned flags);
> virtual bool resolveOwnProperty(JSContext *cx, js::Wrapper &jsWrapper, JSObject *wrapper,
> JSObject *holder, jsid id, JSPropertyDescriptor *desc,
> unsigned flags);
> + static bool defineProperty(JSContext *cx, HandleObject wrapper, HandleId id,
I think these should use JS::Handle<JSObject *> and JS::Handle<jsid> to be consistent with the header.
Attachment #725902 -
Flags: review?(terrence) → review+
Comment 13•12 years ago
|
||
Comment on attachment 725901 [details] [diff] [review]
getOwnPropertyNames & keys - v1
Review of attachment 725901 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/xpconnect/src/XPCComponents.cpp
@@ +3228,5 @@
> return BaseProxyHandler::set(cx, proxy, receiver, id, strict, vp);
> }
>
> bool
> +xpc::SandboxProxyHandler::keys(JSContext *cx, HandleObject proxy,
Please make this JS::Handle<JSObject *> as well.
Comment 14•12 years ago
|
||
Comment on attachment 725903 [details] [diff] [review]
delete - v1
Review of attachment 725903 [details] [diff] [review]:
-----------------------------------------------------------------
\o/
Attachment #725903 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 725903 [details] [diff] [review]
delete - v1
Review of attachment 725903 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jswrapper.cpp
@@ +280,2 @@
> {
> + RootedId idCopy(cx, id);
While working on the big patch I noticed that this copying business is easy to get wrong. Somebody has an idea on how to make this nicer and safer?
Assignee | ||
Comment 16•12 years ago
|
||
This is patch should handle mostly anything inside js/src.
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
Attachment #726376 -
Attachment is obsolete: true
Attachment #727236 -
Flags: review?(terrence)
Assignee | ||
Comment 19•12 years ago
|
||
Boris are you fine with the changes to the bindings?
Attachment #727237 -
Flags: ui-review?(bzbarsky)
Attachment #727237 -
Flags: review?(terrence)
Assignee | ||
Comment 20•12 years ago
|
||
After you suggested this change :)
Attachment #727238 -
Flags: review?(Ms2ger)
Assignee | ||
Updated•12 years ago
|
Attachment #727237 -
Flags: ui-review?(bzbarsky)
Attachment #727237 -
Flags: ui-review?
Attachment #727237 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 727237 [details]
Root rest of the browser - v1
I am going to merge this with the "style" patch.
Attachment #727237 -
Attachment is obsolete: true
Attachment #727237 -
Attachment is patch: false
Attachment #727237 -
Flags: ui-review?
Attachment #727237 -
Flags: review?(terrence)
Attachment #727237 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•12 years ago
|
Attachment #727238 -
Attachment is obsolete: true
Attachment #727238 -
Flags: review?(Ms2ger)
Assignee | ||
Comment 22•12 years ago
|
||
I folded the two patches together, because the split made no sense.
Attachment #727262 -
Flags: review?(terrence)
Attachment #727262 -
Flags: review?(bzbarsky)
Comment 23•12 years ago
|
||
Comment on attachment 727262 [details] [diff] [review]
root proxy/wrapper in the browser - v2
r=me on the bindings bits
Attachment #727262 -
Flags: review?(bzbarsky) → review+
Comment 24•12 years ago
|
||
Comment on attachment 727236 [details] [diff] [review]
Root js/src - v1
Review of attachment 727236 [details] [diff] [review]:
-----------------------------------------------------------------
Nice.
::: js/src/jsproxy.cpp
@@ +534,4 @@
> {
> assertEnteredPolicy(cx, proxy, JSID_VOID);
> + RootedObject target(cx, GetProxyTargetObject(proxy));
> + return obj_toStringHelper(cx, target);
Why do we need a new root for target?
@@ +551,3 @@
> RegExpGuard *g)
> {
> + RootedObject target(cx, GetProxyTargetObject(proxy));
ditto.
::: js/src/jsproxy.h
@@ +141,4 @@
> virtual void finalize(JSFreeOp *fop, JSObject *proxy);
> + virtual bool getElementIfPresent(JSContext *cx, HandleObject obj, HandleObject receiver,
> + uint32_t index, MutableHandleValue vp, bool *present);
> + virtual bool getPrototypeOf(JSContext *cx, HandleObject proxy, JSObject **protop);
It looks like it should be easy to change |protop| to |MutableHandleObject| pretty easily as well.
@@ +356,5 @@
> #ifdef DEBUG
> : context(NULL)
> #endif
> {
> + RootedId id(cx, idArg);
8 spaces here.
::: js/src/vm/ScopeObject.cpp
@@ +1166,2 @@
> else
> + maybeframe.unaliasedVar(i) = vp.get();
|vp| should be automatically converting to Value with |operator T|. How many of these |.get()| are required? I think we should be able to treat MutableHandle exactly like we treat Handle, except for assignment.
Attachment #727236 -
Flags: review?(terrence) → review+
Comment 25•12 years ago
|
||
Comment on attachment 727236 [details] [diff] [review]
Root js/src - v1
Review of attachment 727236 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsproxy.cpp
@@ +534,4 @@
> {
> assertEnteredPolicy(cx, proxy, JSID_VOID);
> + RootedObject target(cx, GetProxyTargetObject(proxy));
> + return obj_toStringHelper(cx, target);
Oh, nevermind, I missed that obj_toStringHelper is in the set you are converting to Handles.
Comment 26•12 years ago
|
||
Comment on attachment 727262 [details] [diff] [review]
root proxy/wrapper in the browser - v2
Review of attachment 727262 [details] [diff] [review]:
-----------------------------------------------------------------
Awesome!
::: dom/bindings/BindingUtils.cpp
@@ +1361,5 @@
> str = nullptr;
> }
> } else {
> if (IsDOMProxy(obj)) {
> + JS::Rooted<JSObject*> proxy(cx, obj);
Could we just re-root |obj| at the top of the method? I'm worried we'll forget to change this back when we convert this method to Handles.
::: dom/bindings/Codegen.py
@@ +6288,5 @@
>
> class CGDOMJSProxyHandler_hasOwn(ClassMethod):
> def __init__(self, descriptor):
> + args = [Argument('JSContext*', 'cx'),
> + Argument('JS::Handle<JSObject *>', 'proxy'),
* next to JSObject
@@ +6333,5 @@
> class CGDOMJSProxyHandler_get(ClassMethod):
> def __init__(self, descriptor):
> + args = [Argument('JSContext*', 'cx'),
> + Argument('JS::Handle<JSObject *>', 'proxy'),
> + Argument('JS::Handle<JSObject *>', 'receiver'),
ditto for these 2
Attachment #727262 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 27•12 years ago
|
||
I made some last minute adjustment, hopefully it sticks nevertheless.
https://hg.mozilla.org/integration/mozilla-inbound/rev/207ff8c91b7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf4dc429c4fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/53a76b390b8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d6bf23b38e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/89a3c21daea6
https://hg.mozilla.org/integration/mozilla-inbound/rev/0596c6e4c260
Comment 28•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/207ff8c91b7d
https://hg.mozilla.org/mozilla-central/rev/bf4dc429c4fa
https://hg.mozilla.org/mozilla-central/rev/53a76b390b8c
https://hg.mozilla.org/mozilla-central/rev/9d6bf23b38e7
https://hg.mozilla.org/mozilla-central/rev/89a3c21daea6
https://hg.mozilla.org/mozilla-central/rev/0596c6e4c260
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•