380970 - Change the URI argument to Components.utils.import

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Robert Sayre]

Right now it uses the ancient rel:, abs:, etc format. It shouldn't, since they end up in the components dir, where they don't belong.

Comment 1

[(not currently active) Ted Mielczarek]

I'm curious as to what the right solution is here.  Didn't you say at some point that we can't load them from chrome:// urls because that probably maps to a jar entry, and that was a problem somehow?

Comment 2

[Mike Kaply [:mkaply]]

What about a separate directory for storing scripts? Separate from components?

Is the plan for extensions to be able to make scripts available via Component.utils.import?

Comment 3

[Benjamin Smedberg]

If we can read the files using the network (nsIInputStream.read()) then URIs are a fine solution... and the chrome registry has some code to track fastload dependencies correctly. If we absolutely have to map to files, we should probably limit it to resource: URIs that implement nsIFileURL.

Comment 4

[Robert Sayre]

(In reply to comment #1)
> I'm curious as to what the right solution is here.  Didn't you say at some
> point that we can't load them from chrome:// urls because that probably maps to
> a jar entry, and that was a problem somehow?
> 

Just that I wasn't sure we could do that without rewriting the already-reviewed patch, and I was more focused on getting the JS API stuff right.

(In reply to comment #3)
> If we can read the files using the network (nsIInputStream.read()) then URIs
> are a fine solution... and the chrome registry has some code to track fastload
> dependencies correctly. 

That sounds good, but I think it should still take a string argument. The argument should be evaluated relative to something, so all the JS modules we ship are available as short strings? Where should that be?

Comment 5

[Benjamin Smedberg]

resource:///libraries/FUEL.js

Comment 6

[Alex Vincent [:WeirdAl]]

Wouldn't it be better to say:
resource://gre/libraries/FUEL.js

So that it's clear what's coming from the XR core, and what comes from the app (resource://app/...)?

Comment 7

[Benjamin Smedberg]

Well, currently FUEL is only shipped with Firefox, and is not part of the XR core. That may change before 1.9 ships, we'll see.

So right now resource:/// or resource://app/ are correct (they should point to the same place).

Comment 8

[Robert Sayre]

(In reply to comment #3)
> the chrome registry has some code to track fastload dependencies correctly.

Is this comment referencing the code located in nsChromeProtocolHandler?

<http://lxr.mozilla.org/seamonkey/source/chrome/src/nsChromeProtocolHandler.cpp#596>

Comment 9

[Benjamin Smedberg]

yes

Comment 10

[Alex Vincent [:WeirdAl]]

Taking - this should be relatively easy to fix, and sayrer indicates he'll review, recommending bsmedberg for the sr.

Comment 11

[Alex Vincent [:WeirdAl]]

(In reply to comment #2)
> What about a separate directory for storing scripts? Separate from components?

My thoughts on this were that we do want a separate directory - say, resource://gre/res/modules/ - and a new Makefile environment variable, EXTRA_JS_MODULES, to copy files there.  (EXTRA_JS_PP_MODULES if anyone wants preprocessing.)

Given how MODULE has a different meaning inside our Makefiles, I felt EXTRA_MODULES would be confusing.  I wouldn't object to a better name, but ultimately that should be a different bug than this one.

Comment 12

[Benjamin Smedberg]

Skip the res/, you want resource:///modules and resource://gre/modules

Comment 13

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1

Note I'm not claiming this is correct.  This is simply my first pass at this bug, and it seems to work.

I decided to skip loading files by jar: or by chrome: for now.  Though you might be able to make it work for jar:, there's the danger of remote files (jar:http://...).  For chrome:, component files don't have access to that at startup.

I'm also deferring on the components -> modules change for now, as I believe that's a separate bug.

Comment 14

[Robert Sayre]

Comment on attachment 267361 [details] [diff] [review]
patch, v1

ok, this almost works as a first step. Couldn't we move XPCOMUtils.js somewhere else, right now, in this exact bug? ;)

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 267361 [details] [diff] [review]
patch, v1

Thanks for doing this!

This is a total drive-by so please feel free to disregard, but I think this is slightly cleaner as you only have to do one GetService and no manual string comparison:

  nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);
  
  nsCOMPtr<nsIURI> resURI;
  rv = ioService->NewURI(aLocation, nsnull, nsnull,
                         getter_AddRefs(resURI));
  
  nsCAutoString scheme;
  rv = resURI->GetScheme(scheme);
  
  nsCOMPtr<nsIProtocolHandler> protocolHandler;
  rv = ioService->GetProtocolHandler(scheme.get(),
                                     getter_AddRefs(protocolHandler));
  
  nsCOMPtr<nsIResProtocolHandler> resHandler =
    do_QueryInterface(protocolHandler, &rv);
  
  nsCAutoString fileLocation;
  rv = resHandler->ResolveURI(resURI, fileLocation);
 
  nsCOMPtr<nsIFile> file;
  rv = NS_GetFileFromURISpec(fileLocation, getter_AddRefs(file),
                             ioService);

I omitted the |NS_ENSURE_SUCCESS(rv, rv);| that should appear before each of the empty lines.

More seriously, though, you need to make sure and check the result of do_GetIOService. And you could always do |if (scheme.EqualsLiteral("resource"))| rather than strcmp.

Comment 16

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.1

(In reply to comment #14)
> ok, this almost works as a first step. Couldn't we move XPCOMUtils.js somewhere
> else, right now, in this exact bug? ;)

Your wish is my command.  Since I'd likely be the one to do it anyway...  I'm really surprised that was your only objection.

(In reply to comment #15)
>   nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);
> 
>   nsCOMPtr<nsIURI> resURI;
>   rv = ioService->NewURI(aLocation, nsnull, nsnull,
>                          getter_AddRefs(resURI));

I felt that it'd be more efficient to ask the IO service about the scheme before trying to construct a new URI object.  The former is simple string manipulation.

>   nsCOMPtr<nsIProtocolHandler> protocolHandler;
>   rv = ioService->GetProtocolHandler(scheme.get(),
>                                      getter_AddRefs(protocolHandler));

I personally don't think this is faster than do_GetService(), since I already know which protocol handler (and thus, the contract ID) I want.  But I could be wrong - io service has caching, and I don't know if getService() does.

> More seriously, though, you need to make sure and check the result of
> do_GetIOService.

From having peterv review my C++ approach to XPathGenerator, I know that checking the rv argument of a do_GetService(aContractID, &rv); is sufficient - checking for the return of do_GetService becomes redundant.

> And you could always do |if
> (scheme.EqualsLiteral("resource"))| rather than strcmp.

Oh, yes, I forgot about that.  :)

Comment 17

[Alex Vincent [:WeirdAl]]

Comment on attachment 267361 [details] [diff] [review]
patch, v1

>+  // make sure we throw when the module file doesn't exist
>   var didThrow = false;
>   try {
>+      Components.utils.import("resource:///foo/XPCOMUtils.jsm", "wrong");
>+  } catch (ex) {
>+      print("ex: " + ex);
>+      didThrow = true;
>+  }
>+  do_check_true(didThrow);

Whoops!  Somehow I lost this whole test when I patched my tree.  This should be part of the patch.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #16)
> checking the rv argument of a do_GetService(aContractID, &rv); is sufficient

True... but you didn't do that in v1 ;)

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

for do_GetIOService, I mean.

Comment 20

[Benjamin Smedberg]

This is not the correct way to convert a resource URI to a file. You should call nsIIOService->NewURI to get the resource URI, then QI it to nsIFileURL

Comment 21

[Mike Kaply [:mkaply]]

Are we going to allow imported scripts to be in extensions?

Comment 22

[Benjamin Smedberg]

The extension could set up a resource mapping to allow that, sure. It would have some timing issues (e.g. you couldn't do it until components were all registered).

Comment 23

[Alex Vincent [:WeirdAl]]

Attachment: patch, v1.2

updated to reflect bsmedberg's comments

Comment 24

[Robert Sayre]

Comment on attachment 267461 [details] [diff] [review]
patch, v1.2

Benjamin should review the rules.mk changes.

Comment 25

[Benjamin Smedberg]

Comment on attachment 267461 [details] [diff] [review]
patch, v1.2

>Index: js/src/xpconnect/tests/unit/component_import.js


>-Components.utils.import("rel:XPCOMUtils.jsm");
>+Components.utils.import("resource:///modules/XPCOMUtils.jsm");

This and elsewhere should be resource://gre/modules/XPCOMUtils.jsm

Comment 26

[Alex Vincent [:WeirdAl]]

We can't check this patch in yet; I just re-ran the regression tests, and we break on test_bogus_files.js.  Sorry.

Comment 27

[Alex Vincent [:WeirdAl]]

Attachment: final patch

This patch should be ready for check-in now.

Comment 28

[Alex Vincent [:WeirdAl]]

Attachment: bustage fix

sorry about that

Comment 29

[Alex Vincent [:WeirdAl]]

Fixed with checkins by bz.

Comment 30

[Alex Vincent [:WeirdAl]]

sayrer, bsmedberg, do we need to make changes to the installer code to package dist/bin/modules as well?  I would think so, but I'm not sure what changes to make.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

For Firefox, you need to add the files to http://lxr.mozilla.org/seamonkey/source/browser/installer/unix/packages-static and http://lxr.mozilla.org/seamonkey/source/browser/installer/windows/packages-static .

Comment 32

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #31)
> For Firefox, you need to add the files to [...]

Filed as bug 385611.

Comment 33

[Sylvain Pasche]

Attachment: Synchronize documentation

yes, duplication sucks.

Comment 34

[Sylvain Pasche]

Attachment: Synchronize documentation, v2

a small other missed replace

Comment 35

[Sylvain Pasche]

please check last patch in (I guess comment only patches don't need sr?)

Comment 36

[Nickolay_Ponomarev]

Checked in attachment 271463 [details] [diff] [review].

mozilla/js/src/xpconnect/idl/xpccomponents.idl  1.35
mozilla/js/src/xpconnect/loader/XPCOMUtils.jsm  1.4