774245 - Reimplement Components.*.lookupMethod in terms of Xray wrapper

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I don't really trust the current implementation, and I'd rather consolidate the native method lookup logic into one place (Xray wrapper). Patches coming right up.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Factor out single-level checked unwrapping into a helper function. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add WrapperFactory and XrayWrapper machinery to allow same-compartment Xray wrapping. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Re-implement moz_bug_r_a4 trickery. v1

In the next patch, we drop support for lookupMethod for location objects, since the security policy there is tricky and location objects are already unshadowable Xray wrappers.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Reimplement LookupMethod. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 5 - Tests. v1

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 642607 [details] [diff] [review]
Part 1 - Factor out single-level checked unwrapping into a helper function. v1

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

::: js/src/jswrapper.cpp
@@ +117,5 @@
> +
> +    Wrapper *handler = Wrapper::wrapperHandler(obj);
> +    bool rvOnFailure;
> +    if (!handler->enter(cx, obj, JSID_VOID,
> +                        Wrapper::PUNCTURE, &rvOnFailure))

Nit: need braces since the consequent is multi-line.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 642610 [details] [diff] [review]
Part 4 - Reimplement LookupMethod. v1

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +2742,5 @@
> +            methodObj = JS_FUNC_TO_DATA_PTR(JSObject *, desc.getter);
> +
> +        // Callers of this function seem to expect bound methods. Make it happen.
> +        if (methodObj && JS_ObjectIsCallable(cx, methodObj))
> +            methodObj = JS_BindCallable(cx, methodObj, obj);

This won't actually be necessary until we fix bug 658909, right?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 642611 [details] [diff] [review]
Part 5 - Tests. v1

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

It would be good to see tests for locations objects and explicitly using the fact that we have bound functions here.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #7)
> This won't actually be necessary until we fix bug 658909, right?

Right. I actually added this during development when I had another silly bug (doing the JS_GetPropertyDescriptorById on |obj| rather than |xray|), which is why it ended up there. But I think it's worth keeping around since we plan on fixing bug 658909 at some point. I've added a comment explaining what's up.

(In reply to Blake Kaplan (:mrbkap) from comment #8)
> It would be good to see tests for locations objects and explicitly using the
> fact that we have bound functions here.

Good idea.

Comment 10

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=cd296732f331

Comment 11

[Bobby Holley (:bholley)]

Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/90ff0bdfdc2c
http://hg.mozilla.org/integration/mozilla-inbound/rev/9582ecb8db2b
http://hg.mozilla.org/integration/mozilla-inbound/rev/3069c8d4a5ef
http://hg.mozilla.org/integration/mozilla-inbound/rev/500ccb7a5dd9
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a8efeb81c64

Comment 12

[Andrew McCreight [:mccr8]]

I think you can remove the memset now because you landed your other patch to initialize JSPropertyDescriptor.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> I think you can remove the memset now because you landed your other patch to
> initialize JSPropertyDescriptor.

http://hg.mozilla.org/integration/mozilla-inbound/rev/fa818c680cb2

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4a8efeb81c64
https://hg.mozilla.org/mozilla-central/rev/500ccb7a5dd9
https://hg.mozilla.org/mozilla-central/rev/3069c8d4a5ef
https://hg.mozilla.org/mozilla-central/rev/9582ecb8db2b
https://hg.mozilla.org/mozilla-central/rev/90ff0bdfdc2c
https://hg.mozilla.org/mozilla-central/rev/fa818c680cb2