183321 - xpconnect shoult not REQUIRE caps

Status: RESOLVED INCOMPLETE

(Core :: XPConnect, defect)

Description

[Brendan Eich [:brendan]]

The only reason I can see (I didn't look exhaustively) seems to be so that
nsISecurityCheckedComponent.idl can live in caps/idl instead of in
js/src/xpconnect/idl.  Jband and I think that source file should move and the
REQUIRES line in xpconnect/src/Makefile.in should not contain caps.

/be

Comment 1

[Brendan Eich [:brendan]]

Please fix this sooner rather than later.  Leaf can help preserve cvs history,
if anyone cares about that for this one file (nsISecurityCheckedComponent.idl).

/be

Comment 2

[David Bradley]

Attachment: Moves nsISecurityCheckedComponent.idl to XPConnect

The cvs history doesn't matter to me. I can always lookup the old version in
the caps directory in CVS if I need that. Even when removed, it's still there
right?

Comment 3

[David Bradley]

Attachment: Same as before but no caps in REQUIRES

Comment 4

[David Bradley]

Comment on attachment 108203 [details] [diff] [review]
Same as before but no caps in REQUIRES

Unfortunately I wasn't lucky and when I compiled after posting this patch, it
turned up a dependency in the component loader. mozJSComponentLoader.h included
in xpcmodule.cpp includes nsIPrincipal.h

Comment 5

[David Bradley]

I'm not sure how easy this is going to be address this. Since the component
loader is a static library instead of a DLL, they'll be link time dependencies
even if we eliminate the compile time dependencies. There's going to be more
work than just moving that IDL file in any case. Moving the component loader
back to a DLL might work, but I haven't looked at everything to know for sure.

Comment 6

[Brendan Eich [:brendan]]

Shaver can help, I bet.

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Hrm.  To fix this, we'd need to define an interface in XPConnect or the
component loader implemented by the caps stuff, which would let us ask for some
opaque principal and the give it to the script, or perhaps just bless a provided
object.  Couldn't happen to a nicer guy, though.

Comment 8

[David Bradley]

to Alpha 1.4

Comment 9

[David Bradley]

To 1.4b

Comment 10

[David Bradley]

Moving to 1.5a

Comment 11

[David Bradley]

There's the BackstagePass which inherits from nsIScriptObjectPrincipal coming
from caps which isn't going to be easy to deal with.

Also, there's the question of what type of dependencies we're trying to
eliminate. Include and/or library? Shaver's suggestion deals with the include,
but doesn't eliminate the library.

Maybe the best solution in view of all this is to move the loader back to a DLL?

Comment 12

[David Bradley]

Attachment: makes the loader a DLL and removes caps dependency for XPCONNECT_STANDALONE

This gets a little closer. This turns the JS loader back into a DLL and takes
out the caps entry in REQUIRES when in XPCONNECT_STANDALONE mode. In
XPCONNECT_STANDALONE mode we're pretty clean on dependencies.

I've been looking at how to get rid of XPCONNECT_STANDALONE all together, but
it's not looking all that easy. Even if we move some things out into JS and
remove compile time dependencies, we'll still have runtime dependencies. I'm
assuming the ultimate goal is to allow XPConnect and the JS Loader to build AND
run without caps, necko, and such.

I can get XPConnect's dependency on caps removed by moving the
nsISecurityCheckedComponent interface into XPConnect. Everything that is
currently using it in the tree also requires XPConnect. I'm not sure that move
would be a good thing.

If the nsISecurityCheckedComponent did move, that would mean that XPConnect
would only depend on xpcom, js, string. JSLoader would still depend on caps,
necko, dom, js, xpconnect, and string. Unless in XPCONNECT_STANDALONE mode,
which removes all but the js, xpconnect, string dependencies.

Comment 13

[Mitchell Stoltz (not reading bugmail)]

I'm fine with having nsISecurityCheckedComponent move; while we're at it we
should change it to take integer values instead of strings for the common arguments.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 126772 [details] [diff] [review]
makes the loader a DLL and removes caps dependency for XPCONNECT_STANDALONE

>+ifndef XPCONNECT_STANDALONE
>+REQUIRES	+= caps \
>+			   dom \
>+			   necko \
>+			   $(NULL)
>+endif

Nit: One too many tabs indenting the dom, necko, and $(NULL) lines, I think.

Looks good to me, shaver might want a look.

I think we should move nsISecurityCheckedComponent to xpconnect.  We ought to
consider the jsloader as a separate module, and see with which subsystem
(xpconnect, js, xpcom) it is most intimate, then move it if needed.  But I'm
hopeful that the only move we end up making is js/src/xpconnect to js/xpconnect
(or js/xpcom, to match extensions/python/xpcom and reduce our jargon penalty
;-).

/be

Comment 15

[David Bradley]

Attachment: Same as before, but moves nsISecurityCheckedComponent

I went ahead and moves nsISecurityCheckComponent.idl. xpconnect no longer has
any XPCONNECT_STANDALONE tests in it and only requires js, xpcom, and strings.

Loader still depends on caps, dom, necko, when not building standalone.

Comment 16

[David Bradley]

Comment on attachment 126937 [details] [diff] [review]
Same as before, but moves nsISecurityCheckedComponent

I think this is worth trying to get in now, since it makes XPConnect proper
pretty clean. JS loader is the only thing that's left that differs between
normal build and standalone build.

Comment 17

[Mike Shaver (:shaver emeritus)]

What does this do to non-JS bindings, like extensions/python?  Are they just not
going to be allowed to have security-checked components?

Comment 18

[Brendan Eich [:brendan]]

If we intend to kill off nsISecurityCheckedComponent some day, or at least not
let it spread beyond xpconnect, then I say move it.

Oop, looks like plugins and XUL content code use it too.  Still, better to
isolate it to xpconnect than make it seem like a public interface that's fair
game to use (I know, it's not marked with @status FROZEN, or with any @status --
maybe it should be marked as deprecated?).

/be

Comment 19

[David Bradley]

So looks like we should go ahead with this patch, can we get some reviews?

Comment 20

[David Bradley]

I had hoped to get this in for the 1.5b but unfortunately with all that
happened, I didn't keep pushing for the reviews.

-> 1.6a

Comment 21

[David Bradley]

Moving out

Comment 22

[Mike Shaver (:shaver emeritus)]

How about: move it to plugins, where we have to support it because we told
vendors to use it (I knew we would regret that), and then plugin code can map
that API to a saner CAPS substrate?

Comment 23

[Mike Shaver (:shaver emeritus)]

Comment on attachment 126937 [details] [diff] [review]
Same as before, but moves nsISecurityCheckedComponent

I remember thinking that I wanted to see that stuff go in a mozilla/script area
that was not as JS specific, but now I'm very fully "ehhhh" about it. 
r+sr=shaver.

Comment 24

[Andrew McCreight [:mccr8]]

This doesn't seem that useful. Somebody can file a new bug if they want to work on separating out the caps dependency from XPConnect.