Closed Bug 886237 Opened 6 years ago Closed 6 years ago

Splitting up XPCComponents.

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

Details

Attachments

(3 files, 1 obsolete file)

XPCComponents is disturbingly big and should be split up into smaller files.
Attached patch XPCComponent split up (obsolete) — Splinter Review
This is nothing more than copy paste really. I want to get rid of some includes from xpccomponents, but I would prefer landing this first. Updating this part is going to be painful otherwise.

I'm not too happy about ThrowAndFail in Sandbox.h maybe it should go somewhere else, but you mentioned that you don't want to add more consumers to it, so I guess you want to get rid of it at some point?

And I had to add IsSandbox, so NukeSandbox does not have to use the direct reference to the sandbox class.

After I got rid of some includes, I want to just quickly go over Sandbox.cpp and fix up some annoyances I made earlier, like comments without periods, etc. But I would prefer landing this as soon as possible.
Attachment #792800 - Flags: review?(bobbyholley+bmo)
Comment on attachment 792800 [details] [diff] [review]
XPCComponent split up

Review of attachment 792800 [details] [diff] [review]:
-----------------------------------------------------------------

Per IRC discussion I don't think Sandbox.h needs to exist. The declaration of nsXPCComponents_Utils_Sandbox can move into Sandbox.cpp, and the miscellaneous helper functions can go in namespace xpc {} in either xpcprivate or xpcpublic.

::: js/xpconnect/src/Sandbox.cpp
@@ +2,5 @@
> + * vim: set ts=8 sw=4 et tw=78:
> + *
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

This should be:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
/* vim: set ts=8 sts=4 et sw=4 tw=99: */
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

See bug 866289 comment 5.

::: js/xpconnect/src/Sandbox.h
@@ +5,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* The Components.Sandbox object. *//***************************************************************************/
> +// Javascript constructor for the sandbox object

These comments are messy.

@@ +48,5 @@
> +nsresult xpc_EvalInSandbox(JSContext *cx, HandleObject sandboxArg, const nsAString& source,
> +                           const char *filename, int32_t lineNo,
> +                           JSVersion jsVersion, bool returnStringOnly, MutableHandleValue rval);
> +
> +bool IsSandbox(JSObject *obj);

These three functions should be in namespace xpc, and the prefix for xpc_EvalInSandbox should be removed. Fine to do as a followup.
Attachment #792800 - Flags: review?(bobbyholley+bmo) → review-
So if you don't mind I will put the utility functions into the xpc namespace in a follow up patch and keep this one as close to copy paste as possible.
Attachment #792800 - Attachment is obsolete: true
Attachment #793495 - Flags: review?(bobbyholley+bmo)
Comment on attachment 793495 [details] [diff] [review]
XPCComponent split up. v2

Review of attachment 793495 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/xpcprivate.h
@@ +3697,5 @@
> +bool
> +NewFunctionForwarder(JSContext *cx, JS::HandleId id, JS::HandleObject callable,
> +                     bool doclone, JS::MutableHandleValue vp);
> +
> +// This should be removed.

Explain what you mean here a bit more?

@@ +3707,5 @@
> +    return NS_OK;
> +}
> +
> +// Infallible.
> +nsCOMPtr<nsIXPCComponents_utils_Sandbox>

This should return an already_AddRefed<>, not an nsCOMPtr. r=bholley with that fixed.
Attachment #793495 - Flags: review?(bobbyholley+bmo) → review+
(In reply to Bobby Holley (:bholley) from comment #4)
> > +// This should be removed.
> 
> Explain what you mean here a bit more?

Actually, I was hoping you add some thoughts to this part. Last time I tried to use this ThrowAndFail function you told me not to add yet another consumer to it. So I kind of assumed that it is being deprecated and will be removed. But that was just an assumption. So, do you want to remove it? And if so what's the problem with it? All I know at this point that you would prefer JS_ReportError to be used instead.
(In reply to Gabor Krizsanits [:krizsa :gabor] (moving to Berlin: in and out for a few weeks) from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > > +// This should be removed.
> > 
> > Explain what you mean here a bit more?
> 
> Actually, I was hoping you add some thoughts to this part. Last time I tried
> to use this ThrowAndFail function you told me not to add yet another
> consumer to it. So I kind of assumed that it is being deprecated and will be
> removed. But that was just an assumption. So, do you want to remove it? And
> if so what's the problem with it? All I know at this point that you would
> prefer JS_ReportError to be used instead.

Yes, in general we want to move away from using the XPConnect exception reporting infrastructure. It just seemed a little odd to me to specifically indicate that ThrowAndFail should be removed, when there's not a clear path to removing it in the near term, and lots of other exception reporting machinery as well. I'd just remove the comment.
#include "jsdbgapi.h" is needed for JS_GetScriptFilename all of a sudden. XPCComponents.cpp had that include from one of the includes I don't use in Sandbox.cpp

The try push was against a bit earlier state so there I have not encounter the problem... *sigh* Thanks for the backing out.
Yeah, missing includes is pretty dangerous right now, with all the removals that are happening :/
I did the xpc_ -> xpc:: conversion for some other helper methods in XPCComponents while I was there. Also, I cannot imagine any meaning of those extern keywords in a header file in front of a C style function, but if you do, I can put them back.
Attachment #794081 - Flags: review?(bobbyholley+bmo)
Wanted to fix these comments for a while... I think this is a good time for that.
Attachment #794086 - Flags: review?(bobbyholley+bmo)
Attachment #794086 - Attachment is patch: true
Comment on attachment 794081 [details] [diff] [review]
follow-up part1: Moving sandbox and XPCComponents helpers into namespace xpc. v1

Review of attachment 794081 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! Thanks for doing this.
Attachment #794081 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 794086 [details] [diff] [review]
follow-up part2: Comment formatting in sandbox. v1

Review of attachment 794086 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/Sandbox.cpp
@@ +1101,5 @@
>      if (!JS_IsArrayObject(cx, arrayObj) ||
>          !JS_GetArrayLength(cx, arrayObj, &length) ||
>          !length)
>      {
> +        // We need a white list of principals or uri strings to create an

whitelist
Attachment #794086 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/396f485ff2e3
Assignee: nobody → gkrizsanits
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.