Closed Bug 1027402 Opened 5 years ago Closed 5 years ago

Make Proxy Handler instances const

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: efaust, Assigned: efaust)

References

Details

Attachments

(4 files)

We want to do this anyway.
Attachment #8442489 - Flags: review?(bobbyholley)
Comment on attachment 8442489 [details] [diff] [review]
Part 1: remove set* from BaseProxyHandler

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

r=bholley with that fix.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1927,5 @@
>  }
>  
>  template <typename Base, typename Traits>
>  XrayWrapper<Base, Traits>::XrayWrapper(unsigned flags)
> +  : Base(flags | WrapperFactory::IS_XRAY_WRAPPER_FLAG, Traits::HasPrototype)

/* hasPrototype = */
Attachment #8442489 - Flags: review?(bobbyholley) → review+
Blocks: 1027425
Maybe we can remove or change some of those cast sites, but this is pretty good
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8442552 - Flags: review?(bobbyholley)
Comment on attachment 8442552 [details] [diff] [review]
Part 2: Sprinkle const all over the place...

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

::: js/src/vm/ProxyObject.cpp
@@ +73,2 @@
>  {
> +    initSlot(HANDLER_SLOT, PrivateValue(const_cast<BaseProxyHandler*>(handler)));

Waldo has tentatively approved the conversion of PrivateValue to taking const void *, which would allow us to elide a bunch of these const casts. I will put this in the final patch.
Comment on attachment 8442552 [details] [diff] [review]
Part 2: Sprinkle const all over the place...

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

r=me with those things sorted out.

::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
@@ +32,5 @@
>      MOZ_ASSERT(js::Wrapper::wrapperHandler(wrapper) ==
>                 &ChromeObjectWrapper::singleton);
>      bool bp;
> +    ChromeObjectWrapper *handler =
> +        const_cast<ChromeObjectWrapper*>(&ChromeObjectWrapper::singleton);

Why do we have to const_cast this?

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +2519,5 @@
>  XrayWrapper<Base, Traits>::call(JSContext *cx, HandleObject wrapper, const JS::CallArgs &args)
>  {
>      assertEnteredPolicy(cx, wrapper, JSID_VOID, BaseProxyHandler::CALL);
> +    // Hard cast the singleton since SecurityWrapper doesn't have one.
> +    return Traits::call(cx, wrapper, args, (js::Wrapper&)Base::singleton);

Why can't Traits::call and Traits::construct take a const ref?
Attachment #8442552 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #4)
> Comment on attachment 8442552 [details] [diff] [review]
> Part 2: Sprinkle const all over the place...
> 
> Review of attachment 8442552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with those things sorted out.
> 
> ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
> @@ +32,5 @@
> >      MOZ_ASSERT(js::Wrapper::wrapperHandler(wrapper) ==
> >                 &ChromeObjectWrapper::singleton);
> >      bool bp;
> > +    ChromeObjectWrapper *handler =
> > +        const_cast<ChromeObjectWrapper*>(&ChromeObjectWrapper::singleton);
> 
> Why do we have to const_cast this?
> 
> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> @@ +2519,5 @@
> >  XrayWrapper<Base, Traits>::call(JSContext *cx, HandleObject wrapper, const JS::CallArgs &args)
> >  {
> >      assertEnteredPolicy(cx, wrapper, JSID_VOID, BaseProxyHandler::CALL);
> > +    // Hard cast the singleton since SecurityWrapper doesn't have one.
> > +    return Traits::call(cx, wrapper, args, (js::Wrapper&)Base::singleton);
> 
> Why can't Traits::call and Traits::construct take a const ref?

As discussed on IRC, it's because none of the functions are const, and in order to avoid making them so, I have opted to cast on the way out from the proxy object. This works normally, but not when we reference them directly.
16:49 <efaust> the impl is a little distasteful. I didn't face the tedium of making all the methods const, and instead opted for a few 
               delicately placed const_casts
16:49 <efaust> which is "almost the right way"
16:49 <jorendorff> heh!
16:49 <jorendorff> yeah i totally would've made you const all the things


Be careful what you wish for ;)

Boris asked specifically to look at changes in dom/bindings/
Attachment #8443305 - Flags: review?(jorendorff)
Attachment #8443305 - Flags: review?(bzbarsky)
Attachment #8443307 - Flags: review?(jorendorff)
Attachment #8443307 - Flags: review?(bzbarsky)
Attachment #8443305 - Flags: review?(jorendorff) → review+
Attachment #8443307 - Flags: review?(jorendorff) → review+
Comment on attachment 8443305 [details] [diff] [review]
Part 3: Take the lid off the const shaker. (make all BPH member functions const)

r=me on the bindings bits
Attachment #8443305 - Flags: review?(bzbarsky) → review+
Comment on attachment 8443307 [details] [diff] [review]
Part 4: Make uses of proxy handlers use const pointers.

r=me on the DOM and xpconnect bits.
Attachment #8443307 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.