Closed
Bug 774245
Opened 12 years ago
Closed 12 years ago
Reimplement Components.*.lookupMethod in terms of Xray wrapper
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(5 files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #642607 -
Flags: review?(mrbkap)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #642608 -
Flags: review?(mrbkap)
Assignee | ||
Comment 3•12 years ago
|
||
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)
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #642611 -
Flags: review?(mrbkap)
Assignee | ||
Updated•12 years ago
|
Attachment #642610 -
Flags: review?(mrbkap)
Comment 6•12 years ago
|
||
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•12 years ago
|
Attachment #642608 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #642609 -
Flags: review?(mrbkap) → review+
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
(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.
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Comment 11•12 years ago
|
||
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
Comment 12•12 years ago
|
||
I think you can remove the memset now because you landed your other patch to initialize JSPropertyDescriptor.
Assignee | ||
Comment 13•12 years ago
|
||
(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
Comment 14•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Updated•12 years ago
|
Depends on: CVE-2012-4208
Updated•12 years ago
|
Depends on: CVE-2013-0795
You need to log in
before you can comment on or make changes to this bug.
Description
•