Closed
Bug 951948
Opened 11 years ago
Closed 10 years ago
Remove same-compartment Components wrapper
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: bholley, Assigned: bholley)
References
(Blocks 1 open bug)
Details
Attachments
(11 files)
1.38 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
7.06 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
7.19 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
19.57 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
7.45 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
Part 6 - Get rid of manual nsIClassInfo and nsIXPCScriptable implementations for nsXPCComponents. v1
9.44 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
11.87 KB,
patch
|
mrbkap
:
review+
gkrizsanits
:
feedback+
|
Details | Diff | Splinter Review |
4.93 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
10.66 KB,
patch
|
mrbkap
:
review+
ted
:
review+
|
Details | Diff | Splinter Review |
1.83 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
12.90 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
Along with bug 825392, this will let us kill same-compartment security wrappers. This wrapper only applies (at present) in the XBL scope, which is pseudo-privileged already. So I think we can just do some simple stuff to prevent the creation of .classes and .utils and whatnot, and get rid of the security wrapper. Doing so will let us rip out nsISecurityCheckedComponent, which will be awesome.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bobbyholley+bmo
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8b65a9b882d3
Assignee | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7804a5645fed
Assignee | ||
Comment 3•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5ba063f385b7 Also, an experimental simplification of the Components object in automation: https://tbpl.mozilla.org/?tree=Try&rev=a434439faf13
Assignee | ||
Comment 4•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=903345a90058
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #4) > https://tbpl.mozilla.org/?tree=Try&rev=903345a90058 This full try push looks good, modulo one Windows build issue. Doing another followup push for that: https://tbpl.mozilla.org/?tree=Try&rev=f3887bac5ff4
Assignee | ||
Comment 6•10 years ago
|
||
Uploading patches and flagging for review.
Assignee | ||
Comment 7•10 years ago
|
||
This lets us remove the usage of nsISecurityCheckedComponent. See the next patch.
Attachment #8358452 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8358453 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8358454 -
Flags: review?(mrbkap)
Assignee | ||
Comment 10•10 years ago
|
||
This thing is only created in non-content scopes for XBL scopes, and during automation (with Cu.getComponentsForScope). At present, we currently have the same-compartment Components wrapper which should do the right thing in those situations. Next, we'll focus on replacing that.
Attachment #8358455 -
Flags: review?(mrbkap)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8358456 -
Flags: review?(mrbkap)
Assignee | ||
Comment 12•10 years ago
|
||
The macro-driven ClassInfo stuff doesn't do getClassDescription, so we need to change that test.
Attachment #8358458 -
Flags: review?(mrbkap)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8358459 -
Flags: review?(mrbkap)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8358461 -
Flags: review?(mrbkap)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8358462 -
Flags: review?(ted)
Attachment #8358462 -
Flags: review?(mrbkap)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8358463 -
Flags: review?(mrbkap)
Assignee | ||
Comment 17•10 years ago
|
||
We fix up the tests here to test the new behavior, and fix some bugs in the test while we're at it.
Attachment #8358464 -
Flags: review?(mrbkap)
Assignee | ||
Comment 18•10 years ago
|
||
Comment on attachment 8358459 [details] [diff] [review] Part 8 - Separate out the unprivileged parts of nsXPCComponents into a separate interface and class. v1 Flagging gabor for feedback here just so that he's in the loop of the new setup.
Attachment #8358459 -
Flags: feedback?(gkrizsanits)
Comment 19•10 years ago
|
||
Comment on attachment 8358462 [details] [diff] [review] Part 10 - Add a way for automation to force the creation of a privileged Components object for an unprivileged scope. v1 Review of attachment 8358462 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty much Greek to me, so consider this rs=me on the specialpowers changes.
Attachment #8358462 -
Flags: review?(ted) → review+
Updated•10 years ago
|
Attachment #8358459 -
Flags: feedback?(gkrizsanits) → feedback+
Updated•10 years ago
|
Attachment #8358452 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8358453 -
Flags: review?(mrbkap) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8358454 [details] [diff] [review] Part 3 - Remove most nsIXPCScriptable junk on nsXPCComponents. v1 Review of attachment 8358454 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCComponents.cpp @@ +3776,5 @@ > + XPCContext* xpcc = XPCContext::GetXPCContext(aCx); > + if (!xpcc) > + return NS_ERROR_FAILURE; > + nsresult res = xpcc->GetLastResult(); > + *aOut = JS_NumberValue((double)(uint32_t) res); I guess you're using what was there before, but any reason to not make this static_cast<double>(static_cast<uint32_t>(res))?
Attachment #8358454 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8358455 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8358456 -
Flags: review?(mrbkap) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8358458 [details] [diff] [review] Part 6 - Get rid of manual nsIClassInfo and nsIXPCScriptable implementations for nsXPCComponents. v1 Review of attachment 8358458 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCComponents.cpp @@ +3606,5 @@ > +/**********************************************/ > + > +class ComponentsSH : public nsIXPCScriptable { > +public: > + ComponentsSH(unsigned dummy) {} Please make this class conform with the recent decisions about brace placement. @@ +3609,5 @@ > +public: > + ComponentsSH(unsigned dummy) {} > + NS_DECL_ISUPPORTS > + NS_DECL_NSIXPCSCRIPTABLE > + static ComponentsSH singleton; For general C++ cleanliness, mind making the singleton object private?
Attachment #8358458 -
Flags: review?(mrbkap) → review+
Comment 22•10 years ago
|
||
Comment on attachment 8358459 [details] [diff] [review] Part 8 - Separate out the unprivileged parts of nsXPCComponents into a separate interface and class. v1 Review of attachment 8358459 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCComponents.cpp @@ +3503,2 @@ > NS_IF_RELEASE(mID); > NS_IF_RELEASE(mException); Mind filing a bug on converting all of these to use smart pointers?
Attachment #8358459 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8358461 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8358462 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8358463 -
Flags: review?(mrbkap) → review+
Comment 23•10 years ago
|
||
Comment on attachment 8358464 [details] [diff] [review] Part 12 - Remove Components wrappers. v1 Review of attachment 8358464 [details] [diff] [review]: ----------------------------------------------------------------- With this patch, do we still need to call JS_WrapObject in nsXPCComponents_Utils::GetComponentsForScope?
Attachment #8358464 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #22) > Mind filing a bug on converting all of these to use smart pointers? filed bug 959413. (In reply to Blake Kaplan (:mrbkap) from comment #23) > With this patch, do we still need to call JS_WrapObject in > nsXPCComponents_Utils::GetComponentsForScope? I think so. In general, the |scope| param to Cu.getComponentsForScope is not same-compartment with the caller.
Assignee | ||
Comment 25•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=301083af0a93
Comment 26•10 years ago
|
||
Comment on attachment 8358453 [details] [diff] [review] Part 2 - Stop using nsISecurityCheckedComponent for nsJSID. v1 Review of attachment 8358453 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCJSID.cpp @@ +261,5 @@ > { 0x00000000, 0x0000, 0x0000, \ > { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 } } > > +// We pass nsIClassInfo::DOM_OBJECT so that nsJSIID instances may be created > +// in unprivilged scopes. nit: unprivileged
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #25) > https://tbpl.mozilla.org/?tree=Try&rev=301083af0a93 Looks mostly green, except potentially some b2g failures. Re-running those to make sure they're real before I spend too much time on it: https://tbpl.mozilla.org/?tree=Try&rev=2b72786de839
Assignee | ||
Comment 28•10 years ago
|
||
Looks like the marionette failures were thankfully related to the base revision, and disappeared on the next try run. Pushed to inbound, with try pushes in comment 25 and comment 27: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7cc30ae56811
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4f9de8b26721 https://hg.mozilla.org/mozilla-central/rev/f24fce132e1a https://hg.mozilla.org/mozilla-central/rev/1469d62ddbf4 https://hg.mozilla.org/mozilla-central/rev/8773a793e758 https://hg.mozilla.org/mozilla-central/rev/9469174aff9b https://hg.mozilla.org/mozilla-central/rev/efffc53426b4 https://hg.mozilla.org/mozilla-central/rev/fbdd66a8b18f https://hg.mozilla.org/mozilla-central/rev/4c687bba563b https://hg.mozilla.org/mozilla-central/rev/b61deb1bee05 https://hg.mozilla.org/mozilla-central/rev/707abbb92a8b https://hg.mozilla.org/mozilla-central/rev/7cc30ae56811
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 30•10 years ago
|
||
Actually, I think this patch queue is broken, specifically part 3. Please bear with me while I explain. Components.lastResult is supposed to get the result of the last exception thrown. However, when going through the xpconnect dispatch mechanism, the call is first routed through CallMethodHelper::Call() here: <http://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#1868>. Several lines later, the last result is reset to NS_ERROR_UNEXPECTED. I'm guessing that the xpcscriptable doesn't go through this helper, so the last result isn't reset and properly reflects the last result. This change is what is causing bug 961054.
Assignee | ||
Comment 31•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b0f32bb816c1
You need to log in
before you can comment on or make changes to this bug.
Description
•