Reimplement Components.*.lookupMethod in terms of Xray wrapper

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
7 years ago
7 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.
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)
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+
Attachment #642608 - Flags: review?(mrbkap) → review+
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.
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
Depends on: CVE-2012-4208
Blocks: 760887
Depends on: CVE-2013-0795
You need to log in before you can comment on or make changes to this bug.