Last Comment Bug 705344 - Use IDL for Components.(utils.)lookupMethod
: Use IDL for Components.(utils.)lookupMethod
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 705324
  Show dependency treegraph
 
Reported: 2011-11-25 11:24 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-12-18 07:12 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.45 KB, patch)
2011-11-25 11:24 PST, :Ms2ger (⌚ UTC+1/+2)
bobbyholley: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-11-25 11:24:03 PST
Created attachment 576974 [details] [diff] [review]
Patch v1
Comment 1 Andreas Gal :gal 2011-11-25 11:28:12 PST
Comment on attachment 576974 [details] [diff] [review]
Patch v1

If you enter a request also enter a compartment. However is the auto request actually needed?
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-11-25 11:40:13 PST
No idea, this is pre-existing code.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2011-12-01 15:12:15 PST
Comment on attachment 576974 [details] [diff] [review]
Patch v1

I'm hoping this method will go away entirely (see bug693733 comment 13). But in the mean time, this is a righteous change. :-)

Also, I'm not too worried about Andreas' comment, since the code stays the same, and requests are going away anyway in 8 weeks.


>-    argv[0] = OBJECT_TO_JSVAL(obj);
>-    rv = nsXPConnect::GetXPConnect()->GetJSObjectOfWrapper(cx, obj, &obj);
>+    nsresult rv = nsXPConnect::GetXPConnect()->GetJSObjectOfWrapper(aCx, obj, &obj);
>     if (NS_FAILED(rv))
>         return rv;

Looks like we've been assuming nsXPConnect::GetXPConnect() is never null here, which is a likely indicator that the thing in bug 705333 is ok.

r=bholley.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2011-12-01 17:50:25 PST
As mentioned in bug 705333, the aFoo argument naming convention needs to be removed here as well.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-12-03 02:32:12 PST
Looks like you were right after all: https://tbpl.mozilla.org/php/getParsedLog.php?id=7722856&tree=Try
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2011-12-03 10:08:19 PST
I'm totally confused - where in the removed code was the compartment being entered?
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-12-03 10:30:26 PST
No idea; I'm removing this code, not claiming to understand what it does :)
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2011-12-05 21:11:37 PST
Comment on attachment 576974 [details] [diff] [review]
Patch v1

Ah, right.

>+    nsresult rv = nsXPConnect::GetXPConnect()->GetJSObjectOfWrapper(aCx, obj, &obj);

So this call, among other things, strips security wrappers (including cross-compartment wrappers) off of |obj|...

>-    if (!member->NewFunctionObject(inner_cc, iface,
>-                                   JSVAL_TO_OBJECT(argv[0]),
>-                                   &funval))
>+    if (!member->NewFunctionObject(inner_cc, iface, obj, &funval))

which is precisely why the old code here passes the original argument, rather than obj (which has been clandestinely mutated).

So that needs to be fixed. This was hard to catch because the change in obj happened without any explicit assignment. So let's fix it by introducing a new JSObject called unwrappedObj (or something), and doing GetJSObjectOfWrapper(aCx, obj, &unwrappedObj);
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-12-06 03:16:46 PST
Eek! Thanks for catching that.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2011-12-18 07:12:04 PST
https://hg.mozilla.org/mozilla-central/rev/50b70f639c2c

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