Closed Bug 121438 Opened 23 years ago Closed 21 years ago

Problem in mozJSComponentLoader.cpp when building with XPCONNECT_STANDALONE

Categories

(Core :: XPConnect, defect, P1)

Product:
Core
Component:
XPConnect
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.0

People

(Reporter: tobias.oberstein, Assigned: shaver)

Details

Attachments

(4 files, 3 obsolete files)

Better fix: x-nativepath: über alles, and light reformatting
2.05 KB, patch
rginda
: review+
Details | Diff | Splinter Review
teach venkman about the new urls
1.17 KB, patch
shaver
: review+
alecf
: superreview+
Details | Diff | Splinter Review
Patch to fix XPCONNECT_STANDALONE bustage
1.04 KB, patch
Details | Diff | Splinter Review
his patch gets xpconnect_standalone past atleast one point of not building in the current trunk (2006/03/26)
4.23 KB, patch
Details | Diff | Splinter Review 
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
Formally confirming bug for consideration -
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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....
and update to the right string API, I don't know why the old cast should even
work!

reviewers?
over to me..
Assignee: dbradley → alecf
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+
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?
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?
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?
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.
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.


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.
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.
sounds ok by me.
ok, over to shaver then
Assignee: alecf → shaver
Status: ASSIGNED → NEW
Whiteboard: fix in hand
Attachment #66311 - Attachment is obsolete: true
But, please go ahead and fix it better.
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 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.

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 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+
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 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
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?)
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.

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.
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
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.
jband: workin' on it ... it will be a while though, so we should certainly land
this.. I just wanted to bitch ;)
So Mozilla 1.0 is going to hand me file: urls, and 1.0.1 is going to give me
something else?  lame.
Sorry, should have noted this was checked into the trunk. Should this go on to
the branch?
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: