886237 - Splitting up XPCComponents.

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

XPCComponents is disturbingly big and should be split up into smaller files.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Attachment: XPCComponent split up

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.

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: XPCComponent split up. v2

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.

Comment 4

[Bobby Holley (:bholley)]

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.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(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.

Comment 6

[Bobby Holley (:bholley)]

(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.

Comment 7

[Gabor Krizsanits (INACTIVE)]

remote:   https://tbpl.mozilla.org/?tree=Try&rev=2edb1e3048ad
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b43d38c5c456

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/682e64c14c46

Comment 9

[Gabor Krizsanits (INACTIVE)]

#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.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yeah, missing includes is pretty dangerous right now, with all the removals that are happening :/

Comment 11

[Gabor Krizsanits (INACTIVE)]

round 2:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=c52c885156c2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/396f485ff2e3

Comment 12

[Gabor Krizsanits (INACTIVE)]

Attachment: follow-up part1: Moving sandbox and XPCComponents helpers into namespace xpc. v1

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.

Comment 13

[Gabor Krizsanits (INACTIVE)]

Attachment: follow-up part2: Comment formatting in sandbox. v1

Wanted to fix these comments for a while... I think this is a good time for that.

Comment 14

[Bobby Holley (:bholley)]

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.

Comment 15

[Bobby Holley (:bholley)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/396f485ff2e3

Comment 17

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=666bab527c33

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c8aac04c97da
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4b742e5df99b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c8aac04c97da
https://hg.mozilla.org/mozilla-central/rev/4b742e5df99b