121438 - Problem in mozJSComponentLoader.cpp when building with XPCONNECT_STANDALONE

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Tobias Oberstein]

mozJSComponentLoader.cpp lacks #include "nsNetUtil.h" with XPCONNECT_STANDALONE 
defined - needs NS_GetURLSpecFromFile

Fix: add #include "nsNetUtil.h"

appeared with ..
gmake -f client.mk pull_all BUILD_MODULES=xpconnect
as of 23.01.2002 17:30 CET

Comment 1

[Phil Schwartau]

Formally confirming bug for consideration -

Comment 2

[John Bandhauer]

This is a alecf's doing (changes from bug 100212). It can't be fixed by just
adding the #include because (as things stand) xpconnect_standalone is not
supposed to require the existence of necko.

I'm not certain why the code needs to resort to getting the URL spec to hand to
JS for displaying the path from which the file was loaded. nsIFile::path would
probably just be good enough here.

Comment 3

[Alec Flett]

yes, note that the dependency existed before - it was just hidden by the fact
that you were calling through XPCOM to get to necko. I think that with
XPCOM_STANDALONE you were just getting an empty string back from GetURL()

my changes simply updated consumers which previously implicitly depended on
necko, to make that dependency explicit since xpcom no longer provides this
backdoor to necko....

Comment 4

[Alec Flett]

Attachment: remove the dependency, switch to GetPath()

and update to the right string API, I don't know why the old cast should even
work!

reviewers?

Comment 5

[Alec Flett]

over to me.. 

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 66311 [details] [diff] [review]
remove the dependency, switch to GetPath()

That's the stuff. r=shaver.

Comment 7

[John Bandhauer]

This is fine with me. One quick question before I stamp it with my sr=... Is
this going to give us a string with a different form (e.g. w/ or w/o file:/// or
escaped or whatever) that the js debugger might care about in its quest to find
the right source to show to the user?

Comment 8

[Robert Ginda]

Ya man, I *need* a URL, not a platform dependant pathname.  See bug 85968.

Comment 9

[Mike Shaver (:shaver emeritus)]

Can't the JS debugger call into necko and convert from nsIFile to nsIFileURL or
whatever?

Comment 10

[Robert Ginda]

Everyone else gives me URLs, I'm not happy about the idea of adding a "does
script.fileName look like url, or a native path?" check.

Do we really need necko to convert necko to convert a native path to a file:// url?

Comment 11

[Alec Flett]

well, looking over that bug I see the issue, but I'm not sure what to do. as it
stands you do need necko - we removed nsIFile::GetURL because the whole
GetURL/SetURL mechanism depends on necko... dougt may have implemented it, but
it was wrong. 

I guess the only options I see are:
1) hand-roll a file:// creation routine in mozJSComponentLoader - yuck!
2) in XPCOM_STANDALONE builds, make this do native paths, and make it do file://
urls elsewhere. That may sounds bad, but I'm not sure what else to do.

In this case where you're getting a native path from .scriptName, do you know
that it's coming from a native JS component, or are all you blind to the actual
source of the script? Because if you know it's a JS component, then at least you
wouldn't have to guess if .scriptName is a native path or not.

Comment 12

[Robert Ginda]

Unfortunatley, there is no way for me to tell who made a particular script.

We only need necko in InitFileFromURL, where it provides
GetParserForScheme("file:"), and the parser itself.  Could the url parsers (just
nsURLParsers.cpp,h?) be moved into xpcom/io?  InitFromURL and GetURL could then
be placed on nsIFile, which makes slightly more sense.  URLs aren't strictly for
networking anyway (as we see here.)

Assuming that idea gets nixed, how about moving *just* GetURLFromFile back onto
nsIFile.  It doesn't require necko at all.  nsIOService::GetURLFromFile could
even stay where it is, and just forward the call to the nsIFile object.

Comment 13

[Alec Flett]

ok, I guess the one option I would be ok with would be adding a 

readonly attribute url;

to nsIFile, since (as I see now) generating the url doesn't actually need necko.
I'm really concerned that people are going to bitch about the symmetry here and
demand a SetURL() again...*sigh*

Comment 14

[Mike Shaver (:shaver emeritus)]

...and necko isn't strictly for networking.   I wouldn't want to see the data:
handling move into XPCOM.

If the real problem here is that the component loader doesn't use an URL for the
script-file, then I'm tempted to just dup the non-necko-needing URL construction
code into mozJSComponentLoader rather than touching nsIFile again.

Comment 15

[Alec Flett]

yeah, I've thought about this more as well, and I've decided that I don't want
to see the notion of "url" in xpcom - there really isn't any other place that
doles out or accepts URL anywhere in XPCOM and I'd hate to see us start now.

Comment 16

[Robert Ginda]

sounds ok by me.

Comment 17

[Alec Flett]

ok, over to shaver then

Comment 18

[John Bandhauer]

Attachment: I checked this in to unbust XPCONNECT_STANDALONE.

But, please go ahead and fix it better.

Comment 19

[Mike Shaver (:shaver emeritus)]

Attachment: Fix: duplicated GetURLSpecFromFile

Fix I should have provided before jband had to wipe my chin.

Review me, I can take it!

Comment 20

[Robert Ginda]

Comment on attachment 72338 [details] [diff] [review]
Fix: duplicated GetURLSpecFromFile

We have 4 GetURLSpecFromFile()s, unix, mac, windows, and os/2.	Each deals with
platform specfic paths in  its own "special" way.  Looks like we;re going to
need some platform #ifdefs here.

+    if (displayPath.Empty())
+	 localFile->GetPath(getter_Copies(displayPath)); // better than nothing
+

If we fail to make a file: url, prepending the	native path with x-nativepath:
will at least let clients know that that this isn't a normal URL.  ie. It might
be possible to have a native path of "http://www.mozilla.org/foo.js",  I'd like
to see "x-nativepath:http://www.mozilla.org/foo.js" in this case.

Comment 21

[Mike Shaver (:shaver emeritus)]

So if you're going to write code to handle x-nativepath: anyway, why am I
bothering to try to convert it to an URL?  I'll just prepend "x-nativepath:" to
whatever path I already have, and save the cycles and code.

Comment 22

[Mike Shaver (:shaver emeritus)]

Attachment: Better fix: x-nativepath: über alles, and light reformatting

First patch stanza is just fixing some 80th-column violations (though I now
wonder why there's no spec-to-registry-location on nsIComponentRegistrar, given
what I assume is dougt's comment).

Comment 23

[Robert Ginda]

Comment on attachment 72439 [details] [diff] [review]
Better fix: x-nativepath: über alles, and light reformatting

OK, I can live with that.  r=rginda.

Comment 24

[Robert Ginda]

Attachment: teach venkman about the new urls

Shaver, I've tested this patch here and it seems to fit the bill.  Would you r=
and check it in when the loader changes?

Comment 25

[Mike Shaver (:shaver emeritus)]

Comment on attachment 72506 [details] [diff] [review]
teach venkman about the new urls

r=shaver, I'll tag it along.

Comment 26

[Alec Flett]

Comment on attachment 72506 [details] [diff] [review]
teach venkman about the new urls

sr=alecf

Comment 27

[Mike Shaver (:shaver emeritus)]

I'm not going to try to put this on the branch, but I'll get it on the trunk
shortly.

Comment 28

[Robert Ginda]

It occurred to me that if x-nativepath is going to be a reak URL, we should
NS_EscapeURLPath it, no?

Comment 29

[Mike Shaver (:shaver emeritus)]

I don't think we have to -- we don't with javascript:, do we?  (Regardless, it's
never intended to be used as an URL -- it's a protocol between the component
loader and consumers of the script path, x-nativepath: is just a prefix.  Gah,
do we have to teach the console as well?)

Comment 30

[Robert Ginda]

The protocol between the script path and consumers is (or should be, IMO) "the
script path will *always* be some kind of URL."

I'm not sure why the js console would have to know anything special about
x-nativepath:, as it doesn't try to dereference the url, AFAIK.

Mozilla has a number of unregistered schemes that don't follow the spec very
well, javascript: is just one of them.  See RFC 2718 "Guidelines for new URL
Schemes" <http://www.ietf.org/rfc/rfc2718.txt>, section 2.2.5 about character
encoding.  <http://www.w3.org/Addressing/schemes.html> enumerates registered and
unregistered url schemes found in the wild.

Comment 31

[David Bradley]

Just a note, mozJSComponentLoader.cpp is broken again when building
XPCONNECT_STANDALONE. I was just showing Phil how to build xpconnect standalone
and got an error complaining about passing a non-lvalue to a non-const ref.
Shaver's patch will fix it.

Comment 32

[Alec Flett]

I still say that #define XPCONNECT_STANDALONE is lame, and we should fix the
code to behave correctly under all circumstances. there should not be a binary
difference between normal and standalone XPCOM/XPConnect

Comment 33

[John Bandhauer]

alecf: If you have a plan on how to do that then say so. XPCONNECT_STANDALONE is
allowing people to use this code without pulling or building caps and necko.

dbradley: Please fix that. You can pretty much *always* land a fix that only
impacts XPCONNECT_STANDALONE - no matter how hacky.

Comment 34

[Alec Flett]

jband: workin' on it ... it will be a while though, so we should certainly land
this.. I just wanted to bitch ;)

Comment 35

[Robert Ginda]

So Mozilla 1.0 is going to hand me file: urls, and 1.0.1 is going to give me
something else?  lame.

Comment 36

[David Bradley]

Attachment: Patch to fix XPCONNECT_STANDALONE bustage

Comment 37

[David Bradley]

Sorry, should have noted this was checked into the trunk. Should this go on to
the branch?

Comment 38

[Scott S. McCoy]

Attachment: his patch gets xpconnect_standalone past atleast one point of not building in the current trunk (2006/03/26)

This patch gets xpconnect_standalone past atleast one point of not building, of the current trunk state.  Not sure it's the best way of doing things because I abandoned my motivation for even doing this due to other issues (the understanding that I cannot disentangle DOM, XSLT, etc from the UI)