Closed Bug 774245 Opened 10 years ago Closed 10 years ago

Reimplement Components.*.lookupMethod in terms of Xray wrapper

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files)

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: 776328
Depends on: 776333
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.