Problem in mozJSComponentLoader.cpp when building with XPCONNECT_STANDALONE

RESOLVED FIXED in mozilla1.0

Status

()

Core
XPConnect
P1
normal
RESOLVED FIXED
17 years ago
12 years ago

People

(Reporter: Tobias Oberstein, Assigned: shaver)

Tracking

Trunk
mozilla1.0
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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

17 years ago
Formally confirming bug for consideration -
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

17 years ago
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

17 years ago
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

17 years ago
Created attachment 66311 [details] [diff] [review]
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

17 years ago
over to me.. 
Assignee: dbradley → alecf

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: fix in hand
Target Milestone: --- → mozilla0.9.9
Comment on attachment 66311 [details] [diff] [review]
remove the dependency, switch to GetPath()

That's the stuff. r=shaver.
Attachment #66311 - Flags: review+

Comment 7

17 years ago
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

17 years ago
Ya man, I *need* a URL, not a platform dependant pathname.  See bug 85968.
Can't the JS debugger call into necko and convert from nsIFile to nsIFileURL or
whatever?

Comment 10

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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*
...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

17 years ago
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

17 years ago
sounds ok by me.

Comment 17

17 years ago
ok, over to shaver then
Assignee: alecf → shaver
Status: ASSIGNED → NEW
Whiteboard: fix in hand

Updated

17 years ago
Attachment #66311 - Attachment is obsolete: true

Comment 18

17 years ago
Created attachment 67664 [details] [diff] [review]
I checked this in to unbust XPCONNECT_STANDALONE.

But, please go ahead and fix it better.
Created attachment 72338 [details] [diff] [review]
Fix: duplicated GetURLSpecFromFile

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

Review me, I can take it!
Attachment #67664 - Attachment is obsolete: true

Comment 20

17 years ago
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.
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.
Created attachment 72439 [details] [diff] [review]
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).
Attachment #72338 - Attachment is obsolete: true

Comment 23

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

OK, I can live with that.  r=rginda.
Attachment #72439 - Flags: review+

Comment 24

17 years ago
Created attachment 72506 [details] [diff] [review]
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 on attachment 72506 [details] [diff] [review]
teach venkman about the new urls

r=shaver, I'll tag it along.
Attachment #72506 - Flags: review+

Comment 26

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

sr=alecf
Attachment #72506 - Flags: superreview+
I'm not going to try to put this on the branch, but I'll get it on the trunk
shortly.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.9 → mozilla1.0

Comment 28

16 years ago
It occurred to me that if x-nativepath is going to be a reak URL, we should
NS_EscapeURLPath it, no?
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

16 years ago
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

16 years ago
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

16 years ago
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

16 years ago
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

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

Comment 35

16 years ago
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

16 years ago
Created attachment 85758 [details] [diff] [review]
Patch to fix XPCONNECT_STANDALONE bustage

Comment 37

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

Updated

15 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 38

12 years ago
Created attachment 216359 [details] [diff] [review]
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)
You need to log in before you can comment on or make changes to this bug.