Closed
Bug 1027402
Opened 7 years ago
Closed 7 years ago
Make Proxy Handler instances const
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: efaust, Assigned: efaust)
References
Details
Attachments
(4 files)
11.21 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
40.20 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
222.17 KB,
patch
|
jorendorff
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
34.79 KB,
patch
|
jorendorff
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
We want to do this anyway.
Attachment #8442489 -
Flags: review?(bobbyholley)
Comment 1•7 years ago
|
||
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+
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
||
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+
Assignee | ||
Comment 5•7 years ago
|
||
(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.
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8443307 -
Flags: review?(jorendorff)
Attachment #8443307 -
Flags: review?(bzbarsky)
Updated•7 years ago
|
Attachment #8443305 -
Flags: review?(jorendorff) → review+
Updated•7 years ago
|
Attachment #8443307 -
Flags: review?(jorendorff) → review+
![]() |
||
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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+
Assignee | ||
Comment 10•7 years ago
|
||
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•7 years ago
|
||
sorry had to back this out for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=42619990&tree=Mozilla-Inbound
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•