Closed Bug 828462 Opened 11 years ago Closed 11 years ago

Exactly root Proxy and Wrapper

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(6 files, 5 obsolete files)

My current id is to do this trap by trap. Let's see how doable that is.
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
Blocks: ExactRooting
Attachment #699927 - Flags: review?(terrence)
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)
(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 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+
> 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.
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
Attached patch defineProperty (obsolete) — Splinter Review
Attachment #725794 - Attachment is obsolete: true
Attachment #725901 - Flags: review?(terrence)
Attachment #725902 - Flags: review?(terrence)
Attached patch delete - v1Splinter Review
Attachment #725903 - Flags: review?(terrence)
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 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 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 on attachment 725903 [details] [diff] [review]
delete - v1

Review of attachment 725903 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #725903 - Flags: review?(terrence) → review+
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?
Attached patch WIP (obsolete) — Splinter Review
This is patch should handle mostly anything inside js/src.
Attached patch Root js/src - v1Splinter Review
Attachment #726376 - Attachment is obsolete: true
Attachment #727236 - Flags: review?(terrence)
Attached file Root rest of the browser - v1 (obsolete) —
Boris are you fine with the changes to the bindings?
Attachment #727237 - Flags: ui-review?(bzbarsky)
Attachment #727237 - Flags: review?(terrence)
Attached patch style changes (obsolete) — Splinter Review
After you suggested this change :)
Attachment #727238 - Flags: review?(Ms2ger)
Attachment #727237 - Flags: ui-review?(bzbarsky)
Attachment #727237 - Flags: ui-review?
Attachment #727237 - Flags: review?(bzbarsky)
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)
Attachment #727238 - Attachment is obsolete: true
Attachment #727238 - Flags: review?(Ms2ger)
I folded the two patches together, because the split made no sense.
Attachment #727262 - Flags: review?(terrence)
Attachment #727262 - Flags: review?(bzbarsky)
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 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 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 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: