Closed Bug 987007 Opened 6 years ago Closed 6 years ago

Remove all remaining uses of JSRESOLVE_ASSIGNING

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

Attached patch Codegen.py WIP 1 (obsolete) — Splinter Review
Attached: a draft of a patch that removes the JSRESOLVE_ASSIGNING uses from dom/bindings/Codegen.py.

However it fails a test:

$ ./mach mochitest-chrome dom/tests/mochitest/chrome/test_indexedSetter.html
 0:17.68 4 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_indexedSetter.html | setting an indexed property through an Xray wrapper should work - got 0, expected 5

    var doc = document.getElementById("testFrame").contentDocument;
    var options = doc.createElement("select").options;
    ok(Components.utils.isXrayWrapper(options), "should be an Xray wrapper");
    var option = doc.createElement("option");
    options[4] = option;
    is(options.length, 5, "setting an indexed property through an Xray wrapper should work")

Now I need to figure out how on earth this is working currently, and add similar plumbing for the set() method.
Copying over some stuff from Bug 824217 …

Does this still depend on Bug 789261 and Bug 826587?
Blocks: 751377
Component: JavaScript Engine → DOM
(In reply to Florian Bender from comment #1)
> Does this still depend on Bug 789261 and Bug 826587?

No.

OK, the way things currently work is:

  - The interpreter calls Proxy::set on the x-ray wrapper.

  - XrayWrapper<>::set() just delegates to the base-class set(), which is defined in terms of
    getOwnPropertyDescriptor() and defineProperty().

  - So we immediately end up in XrayWrapper<>::getOwnPropertyDescriptor()

  - which (indirectly) calls the generated
    mozilla::dom::HTMLOptionsCollectionBinding::ResolveOwnProperty() hook.

  - which calls
    mozilla::dom::HTMLOptionsCollectionBinding::DOMProxyHandler::getOwnPropertyDescriptor(),
    also in generated code, with the JSRESOLVE_ASSIGNING flag.

A slightly crazy thing is happening in that last step: ProxyHandler hooks being passed a proxy that is *not* a proxy of that type. Of course that particular DOMProxyHandler class is specially coded to cope with that weird case.

So, I think I know what plumbing to add.
When DOMXrayTraits::HasPrototype is toggled to true, this set method won't be called anymore. So at some point we're going to have to deal with that. I don't see the solution offhand. I'll put in a comment about it.
Depends on: 987618
Summary: Remove JSRESOLVE_ASSIGNING from DOM bindings → Remove all remaining uses of JSRESOLVE_ASSIGNING
Blocks: 547140
r?bz for Codegen changes, r?bholley for the XrayWrapper stuff
Attachment #8395509 - Attachment is obsolete: true
Attachment #8397224 - Flags: review?(bzbarsky)
Attachment #8397224 - Flags: review?(bobbyholley)
Comment on attachment 8397223 [details] [diff] [review]
Part 1 - Make CGProxyIndexed{Getter,Setter,PresenceChecker,Deleter} return finished C++ code with the type already in it (rather than a template to be filled in later).

r=me
Attachment #8397223 - Flags: review?(bzbarsky) → review+
Comment on attachment 8397224 [details] [diff] [review]
Part 2 - Handle assignment to named and indexed setters without using JSRESOLVE_ASSIGNING.

This is going to have minor merge issues with bug 843840, but we can just deal with that; whoever lands second has to merge.

>+        return dedent("""\

Why the backslash?

>+class CGDOMJSProxyHandler_set(ClassMethod):

Why can't this just live on DOMProxyHandler instead of being codegenned multiple times?  It doesn't seem to need to be different per-proxy-type, given that we have a default no-op implementation of setCustom anyway.

>+                raise ValueError("can't cope with [OverrideBuiltins] and an indexed getter")

Please include the interface name in the string there, and capitalize the first letter in the string?  And similar for the other exceptions raised here.

>(If we're doing it through an
>+        # X-ray wrapper, though, let it go.)

This comment doesn't seem to match the code.  Which is wrong?

>+        if self.descriptor.operations['IndexedSetter'] is not None:

Can you please raise an exception if the IndexedCreator is not the same as the IndexedSetter?  I believe this code is in fact assuming they're the same, right?

>In addition, all proxies which must
>+     * implement get, set, and delete_ at least for the case where an own
>+     * property exists and the default implementation isn't good enough.

I'm not following this sentence.  What is it trying to say?

>+++ b/js/xpconnect/wrappers/XrayWrapper.cpp

>+    RootedObject obj(cx, js::UncheckedUnwrap(wrapper));

Seems to me like we may want to avoid setting if the unwrap fails, but check with bholley, please.

>+        JSAutoCompartment ac(cx, obj);

Entering the content compartment like this is not normal for Xrays...  I'll let Bobby comment on what he wants here, but this seems fishy.

>+        mozilla::dom::DOMProxyHandler *handler =

This file has "using namespace mozilla::dom" , so you can drop that bit.

r=me with those nits dealt with.
Attachment #8397224 - Flags: review?(bzbarsky) → review+
Comment on attachment 8397226 [details] [diff] [review]
Part 3 - Remove the last use of JSRESOLVE_ASSIGNING from nsWindowSH.

r=me
Attachment #8397226 - Flags: review?(bzbarsky) → review+
Comment on attachment 8397224 [details] [diff] [review]
Part 2 - Handle assignment to named and indexed setters without using JSRESOLVE_ASSIGNING.

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

r=me with comments addressed.

::: js/src/jsproxy.h
@@ +94,5 @@
> +     * traps. This is a convenience feature to make it easier to implement
> +     * proxy handlers.
> +     *
> +     * All proxies, even those with mHasPrototype == true, must implement
> +     * getOwnPropertyDescriptor and the other fundamental traps that do not

"fundamental" here refers to the wrong abstraction (fundamental vs derived). This should be "own" or something like it.

@@ +97,5 @@
> +     * All proxies, even those with mHasPrototype == true, must implement
> +     * getOwnPropertyDescriptor and the other fundamental traps that do not
> +     * consult the prototype chain. In addition, all proxies which must
> +     * implement get, set, and delete_ at least for the case where an own
> +     * property exists and the default implementation isn't good enough.

I think the wording here has confusing implications. How about:

In addition, any proxy with behavior that cannot be entirely described in terms of property descriptors (i.e. named setters) must also implement get, set, and delete_.

Thanks for adding the comment here!

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1487,5 @@
> +DOMXrayTraits::set(JSContext *cx, HandleObject wrapper, HandleObject receiver, HandleId id,
> +                   bool strict, MutableHandleValue vp)
> +{
> +    MOZ_ASSERT(xpc::WrapperFactory::IsXrayWrapper(wrapper));
> +    RootedObject obj(cx, js::UncheckedUnwrap(wrapper));

For consistency with the rest of this file, this should be (equivalently):

RootedObject target(cx, getTargetObject(wrapper));

@@ +1488,5 @@
> +                   bool strict, MutableHandleValue vp)
> +{
> +    MOZ_ASSERT(xpc::WrapperFactory::IsXrayWrapper(wrapper));
> +    RootedObject obj(cx, js::UncheckedUnwrap(wrapper));
> +    if (js::IsProxy(obj)) {

This is scary. Let's use IsDOMProxy here.

@@ +1491,5 @@
> +    RootedObject obj(cx, js::UncheckedUnwrap(wrapper));
> +    if (js::IsProxy(obj)) {
> +        JSAutoCompartment ac(cx, obj);
> +        mozilla::dom::DOMProxyHandler *handler =
> +            (mozilla::dom::DOMProxyHandler *) js::GetProxyHandler(obj);

Can we add a DOMProxyHandler() acccessor in DOMJSProxyHandler.h which asserts IsDOMPRoxy and then does the cast (though via static_cast, rather than c-style)? I'd rather centralize the assumptions that let us do this cast.

@@ +1492,5 @@
> +    if (js::IsProxy(obj)) {
> +        JSAutoCompartment ac(cx, obj);
> +        mozilla::dom::DOMProxyHandler *handler =
> +            (mozilla::dom::DOMProxyHandler *) js::GetProxyHandler(obj);
> +        MOZ_ASSERT(!handler->hasPrototype());

Peter's going to hit this assert soon with the patches in bug 787070, so please provide some context as to why it exists and what needs to be done to fix it.
Attachment #8397224 - Flags: review?(bobbyholley) → review+
(In reply to Boris Zbarsky [:bz] from comment #8)
> >+        return dedent("""\
> 
> Why the backslash?

Removed.

> >+class CGDOMJSProxyHandler_set(ClassMethod):
> 
> Why can't this just live on DOMProxyHandler instead of being codegenned
> multiple times?  It doesn't seem to need to be different per-proxy-type,
> given that we have a default no-op implementation of setCustom anyway.

Oh, good point. I have a choice though:
  - codegen a set() method for each class and have it call a common implementation; or 
  - implement set() in a base class shared by just these classes

Guess I'll try the latter. I'll do it in a second patch so it can be reviewed as a separate thing on top of what you've already reviewed.

> >+                raise ValueError("can't cope with [OverrideBuiltins] and an indexed getter")
> 
> Please include the interface name in the string there, and capitalize the
> first letter in the string?  And similar for the other exceptions raised
> here.

Done.

> >(If we're doing it through an
> >+        # X-ray wrapper, though, let it go.)
> 
> This comment doesn't seem to match the code.  Which is wrong?

Stale comment. Removed.

> >+        if self.descriptor.operations['IndexedSetter'] is not None:
> 
> Can you please raise an exception if the IndexedCreator is not the same as
> the IndexedSetter?  I believe this code is in fact assuming they're the
> same, right?

Done.

> >In addition, all proxies which must
> >+     * implement get, set, and delete_ at least for the case where an own
> >+     * property exists and the default implementation isn't good enough.
> 
> I'm not following this sentence.  What is it trying to say?

I'm rewriting the comment now. Here's what I've got so far:

    /*
     * A hack. This flag's unspeakable powers cannot be described in
     * the tongues we mortals use but it is way convenient.
     *
     * May cause fits.
     */
    bool mHasPrototype;

> >+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
> 
> >+    RootedObject obj(cx, js::UncheckedUnwrap(wrapper));
> 
> Seems to me like we may want to avoid setting if the unwrap fails, but check
> with bholley, please.
>
> >+        JSAutoCompartment ac(cx, obj);
> 
> Entering the content compartment like this is not normal for Xrays...  I'll
> let Bobby comment on what he wants here, but this seems fishy.

Will do.

> >+        mozilla::dom::DOMProxyHandler *handler =
> 
> This file has "using namespace mozilla::dom" , so you can drop that bit.

Done. I changed the cast to a static_cast.
> Guess I'll try the latter.

Please!   Seems like it would be less code.  Patch on top to do that makes sense.
(In reply to Bobby Holley (:bholley) from comment #10)
> I think the wording here has confusing implications.

This comment left bz perplexed as well. I completely rewrote it. Here's what I've got now.

    /*
     * Proxy handlers can use mHasPrototype to request the following special
     * treatment from the JS engine:
     *
     *   - When mHasPrototype is true, the engine never calls these methods:
     *     getPropertyDescriptor, has, set, enumerate, iterate.  Instead, for
     *     these operations, it calls the "own" traps like
     *     getOwnPropertyDescriptor, hasOwn, defineProperty, keys, etc., and
     *     consults the prototype chain if needed.
     *
     *   - When mHasPrototype is true, the engine calls handler->get() only if
     *     handler->hasOwn() says an own property exists on the proxy. If not,
     *     it consults the prototype chain.
     *
     * This is useful because it frees the ProxyHandler from having to implement
     * any behavior having to do with the prototype chain.
     */
    bool mHasPrototype;


> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> @@ +1487,5 @@
> > +DOMXrayTraits::set(JSContext *cx, HandleObject wrapper, HandleObject receiver, HandleId id,
> > +                   bool strict, MutableHandleValue vp)
> > +{
> > +    MOZ_ASSERT(xpc::WrapperFactory::IsXrayWrapper(wrapper));
> > +    RootedObject obj(cx, js::UncheckedUnwrap(wrapper));
> 
> For consistency with the rest of this file, this should be (equivalently):
> 
> RootedObject target(cx, getTargetObject(wrapper));

Done.

> > +    if (js::IsProxy(obj)) {
> 
> This is scary. Let's use IsDOMProxy here.

Perfect! Thank you, that's *much* better.

> Can we add a DOMProxyHandler() acccessor in DOMJSProxyHandler.h which
> asserts IsDOMPRoxy and then does the cast (though via static_cast, rather
> than c-style)?

Done.

> @@ +1492,5 @@
> > +    if (js::IsProxy(obj)) {
> > +        JSAutoCompartment ac(cx, obj);
> > +        mozilla::dom::DOMProxyHandler *handler =
> > +            (mozilla::dom::DOMProxyHandler *) js::GetProxyHandler(obj);
> > +        MOZ_ASSERT(!handler->hasPrototype());
> 
> Peter's going to hit this assert soon with the patches in bug 787070, so
> please provide some context as to why it exists and what needs to be done to
> fix it.

Fixed:

        // Dear peterv: ha ha ha. -jorendorff
        MOZ_ASSERT(!handler->hasPrototype());
> Peter's going to hit this assert soon with the patches in bug 787070, so
> please provide some context as to why it exists and what needs to be done to
> fix it.

I thought through this and decided to remove the assertion.

The logic behind it was "well, the handler->hasPrototype() contract says the engine promises not to call set(). setCustom() is a set() variant, so we shouldn't call it either." But setCustom() deals with "own" behavior and there's really no reason not to call it.

Comment 3 still applies, but that refers to the non-X-ray case, not the X-ray case which will be fine regardless.
Ok. Just as long as this will fail in an obvious way when we get back to bug 787070.
(In reply to Boris Zbarsky [:bz] from comment #8)
> >+bool
> >+DOMXrayTraits::set(JSContext *cx, HandleObject wrapper, HandleObject receiver, HandleId id,
> >+                   bool strict, MutableHandleValue vp)
> >+{
> >+    MOZ_ASSERT(xpc::WrapperFactory::IsXrayWrapper(wrapper));
> >+    RootedObject obj(cx, getTargetObject(wrapper));
> >+    if (IsDOMProxy(obj)) {
> >+        JSAutoCompartment ac(cx, obj);
> 
> Entering the content compartment like this is not normal for Xrays...  I'll
> let Bobby comment on what he wants here, but this seems fishy.

bholley, what do you think? XrayWrapper<Base, Traits>::set calls into this method without consulting Base at all. Is this JSAutoCompartment necessary/sufficient?
(In reply to Jason Orendorff [:jorendorff] from comment #16)
> bholley, what do you think? XrayWrapper<Base, Traits>::set calls into this
> method without consulting Base at all. Is this JSAutoCompartment
> necessary/sufficient?

Why do we need the JSAutoCompartment? In general, DOM code is supposed to handle the case where it's called with a cx-compartment that is different than the reflector of |this|.

I'm not a great Codegen-whisperer, so I can't quite figure out what setCustom implementations look like. Though the Codegen talks about handling an Xray case, which is fishy given the JSAutoCompartment. Can you upload a sample of the generated code?
Attachment #8409808 - Attachment description: bug-987007-part-2a-common-up-set-methods-v1.patch → Part 2a - Instead of generating identical DOMProxyHandler::set() methods for many interfaces, implement it in a common base class.
Comment on attachment 8409808 [details] [diff] [review]
Part 2a - Instead of generating identical DOMProxyHandler::set() methods for many interfaces, implement it in a common base class.

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

::: dom/bindings/DOMJSProxyHandler.h
@@ +132,5 @@
>  
> +class DOMProxyHandlerWithSetCustom : public DOMProxyHandler
> +{
> +public:
> +  bool set(JSContext *cx, JS::HandleObject proxy, JS::HandleObject receiver,

You're in dom/, don't use the typedefs.
Comment on attachment 8409808 [details] [diff] [review]
Part 2a - Instead of generating identical DOMProxyHandler::set() methods for many interfaces, implement it in a common base class.

I don't understand why we need the DOMProxyHandlerWithSetCustom class.  Why can we not just put the code this patch puts in DOMProxyHandlerWithSetCustom::set into DOMProxyHandler::set?
Flags: needinfo?(jorendorff)
I don't know why I did it that way. Here's the simpler patch.
Attachment #8409808 - Attachment is obsolete: true
Attachment #8409808 - Flags: review?(bzbarsky)
Attachment #8411431 - Flags: review?(bzbarsky)
Flags: needinfo?(jorendorff)
Comment on attachment 8411431 [details] [diff] [review]
Part 2a - Instead of generating identical DOMProxyHandler::set() methods for many interfaces, implement it in the base class.

Yes, that's more like it!

r=me
Attachment #8411431 - Flags: review?(bzbarsky) → review+
Depends on: 1047122
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.