Last Comment Bug 774245 - Reimplement Components.*.lookupMethod in terms of Xray wrapper
: Reimplement Components.*.lookupMethod in terms of Xray wrapper
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on: 776328 776333 CVE-2012-4208 CVE-2013-0795
Blocks: 760887
  Show dependency treegraph
 
Reported: 2012-07-16 05:52 PDT by Bobby Holley (busy)
Modified: 2013-01-31 13:23 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Factor out single-level checked unwrapping into a helper function. v1 (2.81 KB, patch)
2012-07-16 09:14 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 2 - Add WrapperFactory and XrayWrapper machinery to allow same-compartment Xray wrapping. v1 (3.67 KB, patch)
2012-07-16 09:14 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 3 - Re-implement moz_bug_r_a4 trickery. v1 (1.12 KB, patch)
2012-07-16 09:14 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 4 - Reimplement LookupMethod. v1 (5.48 KB, patch)
2012-07-16 09:14 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Part 5 - Tests. v1 (2.84 KB, patch)
2012-07-16 09:15 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review

Description Bobby Holley (busy) 2012-07-16 05:52:11 PDT
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 (busy) 2012-07-16 09:14:12 PDT
Created attachment 642607 [details] [diff] [review]
Part 1 - Factor out single-level checked unwrapping into a helper function. v1
Comment 2 Bobby Holley (busy) 2012-07-16 09:14:25 PDT
Created attachment 642608 [details] [diff] [review]
Part 2 - Add WrapperFactory and XrayWrapper machinery to allow same-compartment Xray wrapping. v1
Comment 3 Bobby Holley (busy) 2012-07-16 09:14:37 PDT
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.
Comment 4 Bobby Holley (busy) 2012-07-16 09:14:49 PDT
Created attachment 642610 [details] [diff] [review]
Part 4 - Reimplement LookupMethod. v1
Comment 5 Bobby Holley (busy) 2012-07-16 09:15:02 PDT
Created attachment 642611 [details] [diff] [review]
Part 5 - Tests. v1
Comment 6 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-07-17 19:00:37 PDT
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) (please use needinfo!) 2012-07-17 19:04:57 PDT
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) (please use needinfo!) 2012-07-17 19:06:20 PDT
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 (busy) 2012-07-18 02:21:12 PDT
(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 (busy) 2012-07-18 02:50:52 PDT
https://tbpl.mozilla.org/?tree=Try&rev=cd296732f331
Comment 12 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-07-18 05:52:28 PDT
I think you can remove the memset now because you landed your other patch to initialize JSPropertyDescriptor.
Comment 13 Bobby Holley (busy) 2012-07-18 05:57:38 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.