1027402 - Make Proxy Handler instances const

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Attachment: Part 1: remove set* from BaseProxyHandler

We want to do this anyway.

Comment 1

[Bobby Holley (:bholley)]

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 = */

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Sprinkle const all over the place...

Maybe we can remove or change some of those cast sites, but this is pretty good

Comment 3

[Eric Faust [:efaust]]

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 4

[Bobby Holley (:bholley)]

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?

Comment 5

[Eric Faust [:efaust]]

(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.

Comment 6

[Eric Faust [:efaust]]

Attachment: Part 3: Take the lid off the const shaker. (make all BPH member functions const)

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/

Comment 7

[Eric Faust [:efaust]]

Attachment: Part 4: Make uses of proxy handlers use const pointers.

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 10

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6403818106
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/346912776f97
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/285c853fedfa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/96443362db6f

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=42619990&tree=Mozilla-Inbound

Comment 12

[Eric Faust [:efaust]]

Let's try this again, with the bustage fixed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/636c7e0c143d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dc01b6b21d71
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5b4bc91c68
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f74b2c50ea0b

(some modicum of proof is here: https://tbpl.mozilla.org/?tree=Try&rev=5db39ec68152)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/636c7e0c143d
https://hg.mozilla.org/mozilla-central/rev/dc01b6b21d71
https://hg.mozilla.org/mozilla-central/rev/fe5b4bc91c68
https://hg.mozilla.org/mozilla-central/rev/f74b2c50ea0b