Reimplement Components.*.lookupMethod in terms of Xray wrapper

RESOLVED FIXED in mozilla17

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

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.
Created attachment 642607 [details] [diff] [review]
Part 1 - Factor out single-level checked unwrapping into a helper function. v1
Attachment #642607 - Flags: review?(mrbkap)
Created attachment 642608 [details] [diff] [review]
Part 2 - Add WrapperFactory and XrayWrapper machinery to allow same-compartment Xray wrapping. v1
Attachment #642608 - Flags: review?(mrbkap)
Created attachment 642609 [details] [diff] [review]
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.
Attachment #642609 - Flags: review?(mrbkap)
Created attachment 642610 [details] [diff] [review]
Part 4 - Reimplement LookupMethod. v1
Created attachment 642611 [details] [diff] [review]
Part 5 - Tests. v1
Attachment #642611 - Flags: review?(mrbkap)
Attachment #642610 - Flags: review?(mrbkap)
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.
Attachment #642607 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #642608 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #642609 - Flags: review?(mrbkap) → review+
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?
Attachment #642610 - Flags: review?(mrbkap) → review+
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.
Attachment #642611 - Flags: review?(mrbkap) → review+
(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.
https://tbpl.mozilla.org/?tree=Try&rev=cd296732f331
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
I think you can remove the memset now because you landed your other patch to initialize JSPropertyDescriptor.
(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
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 776328
Depends on: 776333
Depends on: 798264
Blocks: 760887
Depends on: 825697
You need to log in before you can comment on or make changes to this bug.