If '/' in path, chrome files generated incorrectly, app will never restart

VERIFIED FIXED in Future

Status

Core Graveyard
Skinability
P2
critical
VERIFIED FIXED
18 years ago
10 years ago

People

(Reporter: Mike Pinkerton (not reading bugmail), Assigned: Conrad Carlen (not reading bugmail))

Tracking

({crash, helpwanted})

Trunk
Future
PowerPC
All
crash, helpwanted

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm++])

Attachments

(3 attachments)

(Reporter)

Description

18 years ago
- build mozilla
- put a '/' in the path anywhere along the way from the HD to the bin dir
- run mozilla (it will work_
- run it again

mozilla stalls at startup. Deleting the generated chrome registry files 
("chrome:overlayinfo" folder, chrome:*.rdf)  will get it to start again, but then 
it will fail again. deleting the chrome registry files and then removing the 
slash will cause the app to work a-ok.
(Reporter)

Comment 1

18 years ago
this is very serious, and needs to be fixed for beta3. Lots of people use slashes 
in folder/hardDrive names, especially on mac (hence nsmac1). 

if this is not fixed, the app will not restart for those unlucky people and they 
won't know how to ever get it to work again.
Keywords: crash, nsbeta3, nsmac1
(Reporter)

Comment 2

18 years ago
another way to put this is as such: if you have a / in your path, we will only 
run when the chrome files need to be generated (first time). After that, we'll 
always fail.

Comment 3

18 years ago
Actually, I think the chrome RDF files may be generated correctly, but the code 
that uses them has bad slash habits.
(Reporter)

Comment 4

18 years ago
rtm nomination.
Keywords: rtm

Comment 5

18 years ago
Hmn, having the app stop working entirely would be bad, right?  rtm+ need info.
High profile, very obscure workaround (for Mac folks).  P2  Should be worth a
simple fix for rtm, but no heroic efforts.
Priority: P3 → P2
Whiteboard: [rtm+ need info]
Target Milestone: --- → M19
(Assignee)

Comment 6

18 years ago
Looked at it and found this: nsChromeRegistry builds URLs from file specs using
nsFileURL. This uses MacFileHelpers::EncodeMacPath to build the URL string. When
it comes time to load the chrome and the URL has to be turned back into a file,
nsStdURL::GetFile is used. Either EncodeMacPath is wrong (likely) when paths
have slashes, or it doesn't work (no round trip conversion) when it is decoded
by nsStdURL. Best to not use nsFileSpec and nsFileURL. Better would be to
implement GetURL/SetURL on nsIFile (declared but not implemented right now),
taking the logic from nsStdURL.

Comment 7

18 years ago
->dougt, need to implement additional nsiFile methods & give bug back to hyatt.
Assignee: hyatt → dougt

Comment 8

18 years ago
warren, were you doing something like this?

Comment 9

18 years ago
I added attribute string url to nsIFile, but it really isn't in use yet. Once we 
get it going, we should remove GetFile/SetFile from nsStdURL.

Comment 10

18 years ago
Dave,

Okay here is a quick how to convert URL's to nsIFile's and vise versa:


given a nsIFile call GetURL to get that file's URL  (cake)

Given a URL and you want an nsIFIle, call SetURL. (DAMN it is not implemented...
we have to do some work)  ->

    nsCOMPtr<nsIURI> aIURI;
    if (NS_FAILED(rv = NS_NewURI(getter_AddRefs(aIURI), uri)))
	    return(PR_FALSE);
    if (!aIURI) return(PR_FALSE);

    nsCOMPtr<nsIFileURL>    fileURL = do_QueryInterface(aIURI);
    if (!fileURL)   return(PR_FALSE);

    nsCOMPtr<nsIFile> aDir;
    rv = fileURL->GetFile(getter_AddRefs(aDir));
    if (NS_FAILED(rv))  return(PR_FALSE);
Assignee: dougt → hyatt

Comment 11

18 years ago
marking helpwanted, as we are unlikely to be able to get to this in time.
Keywords: helpwanted

Comment 12

18 years ago
need a good relnote here...anyone?
Keywords: relnoteRTM

Comment 13

18 years ago
Time to cut this, since we are quickly ramping down, and no simple fix has
emerged. rtm-/future
Whiteboard: [rtm+ need info] → [rtm-]
Target Milestone: M19 → Future

Comment 14

18 years ago
Whaaaaaa. This is a very evil bug. It cost pinkerton and I several hours of 
debugging, and prevented him from running builds for a long time. It's evil 
because it's very non-obvious. I strongly advocate that we fix this for RTM.

Updated

18 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 15

18 years ago
putting back on need info list. As this cost _me_, a nscp engineer who knows
what is going on, one full week of productivity, imagine what this is going to
do to users.

We're already running into people in the field with hard drives named
"Files/Apps" or the like which won't be able to run our product and won't know
why. Users don't read release notes, and even when we posted info about this bug
to mac news sites, people would comment directly below the posting about
"unexplainable" stalls at startup.

doug? your last comment indicated you have to do some work on this. Well, now is
the time. I know you're busy with embedding, but this is a critical bug (think
of it as a crash) for releasing NS6. What will entice you? Beer?  Do you need help?
Assignee: hyatt → dougt
Status: ASSIGNED → NEW
Whiteboard: [rtm-] → [rtm need info]
(Reporter)

Comment 16

18 years ago
forgot to mention before...

There has got to be a simpler fix than converting to use nsIFile, which is
significant and probably not acceptable for RTM. Someone (conrad?) suggested
that there's a routine that encodes mac paths that is probably doing the wrong
thing. Can we just patch that routine?

Comment 17

18 years ago
Created attachment 16841 [details] [diff] [review]
makes chrome registry use nsIFile (and necko for url parsing.)
(Reporter)

Comment 18

18 years ago
doug: thanks for the patch, but I really don't think pdt will thumbs up a change
of this magnitude this close to the ship date. any other ideas?

Comment 19

18 years ago
The diffs are not really that big, and look pretty safe to me. hyatt?
(Reporter)

Comment 20

18 years ago
ok, i applied the patch and it appears to work (yay dougt! i owe you booze!).
What sorts of things should I be testing (besides the obvious components). I
tried switching skins, restarting the app multiple times, adding slashes at
different levels of the path...

what else? hyatt, we need a review of this.

Comment 21

18 years ago
Why the changes from "aFileURL" to "outFileURL"? When in Rome, and all. Other
than that, a=waterson.

Comment 22

18 years ago
Patch looks good. My only suggestion would be to put the 

+//----------------------------------------------- hack until nsLocalFile::GetURL 
is implemented  
+  nsCOMPtr<nsIURI> url;
+  NS_NewURI(getter_AddRefs(url), "file:///");
+  if (!url) return NS_ERROR_FAILURE;
+
+  nsCOMPtr<nsIFileURL> fileUrl = do_QueryInterface(url);
+  rv = fileUrl->SetFile(file);
+  if (NS_FAILED(rv)) return rv;
+
+  nsXPIDLCString urlStr;    
+  rv = fileUrl->GetSpec(getter_Copies(urlStr));
+  if (NS_FAILED(rv)) return rv;
+//---------------------------------------------- end hack  

into a local function, for cleanliness, since it's repeated twice. This would 
also scope the temporary nsCOMPtrs there better.

Even without that change, sr=sfraser.
(Reporter)

Comment 23

18 years ago
we have patch, r= and a=. landing on the trunk (with sfraser's suggestion to
factor the duplicated code).

we need some qa lovin' on this to help PDT feel warm and fuzzy about this.
anyone up to the task?
Whiteboard: [rtm need info] → [rtm need info] landed on trunk
(Reporter)

Comment 24

18 years ago
cc'ing jrgm

Comment 25

18 years ago
You do not have module owner review for this code.  Do not land it on the trunk
until I have had a chance to review it.

Comment 26

18 years ago
Ok, I see you landed it anyway, despite not having module owner review.  I'm not
going to be an asshole about this, and won't back it out (even though I'd be
well within my rights to), but please keep in mind that module owner review is
also a requirement.  You can't just pick a random r= and a= and then check a bug
fix into someone else's module.

Comment 27

18 years ago
This patch caused a regression and busted user stylesheets on UNIX.  Given that 
it wasn't approved by the module owner and given that the bug wasn't even +, I 
have now backed the patch out of the trunk.

This was an uncool rush job, and you should not have checked this in.  Come on, 
guys.  2 hours from the time the patch is attached to a trunk checkin, without 
module owner review or + or thorough testing?  Uncool.

I'm taking this bug back now.
Assignee: dougt → hyatt

Comment 28

18 years ago
dougt, question.  The case of the chrome dir is wrong in one of your special 
directory locations or something.  The bustage caused by this patch is that 
"chrome" became "Chrome".  This breaks user stylesheets and also causes the 
loss of the user's selected skin/locale (once the user has switched once).  

I need to know where you defined the special location so that we can fix 
"Chrome" so that it is "chrome" (like it's supposed to be).
Status: NEW → ASSIGNED

Comment 30

18 years ago
That files defines it as "chrome" lower case on anything other than a Mac, but
somehow when your patch was applied on Unix it became "Chrome" instead.

Comment 31

18 years ago
"Chrome" is a Mac-ism, but becuase the Mac FS is case insensitive, "chrome" will 
work fine.

Comment 32

18 years ago
Ok, it gets even worse.  When the chrome directory doesn't exist in the profile 
(which is the case for everybody), this new code leaves off the trailing slash.  
It then performs writes with the slash omitted, so your skin data is saved at

chromeuser-skins.rdf

instead of

chrome/user-skins.rdf

In addition it seems to periodically pick the wrong case (this is not always 
reproducible).

It seems like there's a bug with the directory service.  Any nsIFiles it hands 
back are (by the very definition of the service) directories, and even if they 
don't exist should have the trailing slash.

Comment 33

18 years ago
To Simon: Problem is it switched from "chrome" to "Chrome" on UNIX, which is 
case-sensitive. :(

Comment 34

18 years ago
I have an idea that will probably be safe.  Why don't we go back to the world 
where I manually append the "chrome" subdir onto the app install and app profile 
dirs.  That would solve all these bugs.
(Assignee)

Comment 35

18 years ago
I checked something in this morning which was similar to the above patch. I had
already been through the "Chrome" vs. "chrome" problem, the directory not
existing, and so on and took care of that. I should have added myself on the cc
list and we could have combined efforts. My patch was just to use directory
services in chrome.
I've looked at implementing Get/SetURL on nsIFile. This needs to have all the
sophistication of nsIStdURL. It needs to do exactly the same thing (using the
same code). I ran into one problem though. There is a file needed
nsURLHelpers.h/.cpp. It's in Necko though. Can we put this file into xpcom? Is
that too much at this point?

Comment 36

18 years ago
Conrad's patch fixed the problems I described (by ensuring that the profile
chrome dir gets created properly) and fixed this bug as well.  Thanks ccarlen!

Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Reporter)

Comment 37

18 years ago
so this is on both the trunk and the branch?

Comment 38

18 years ago
Yes.  It accidentally came along for the ride on the branch when I checked in my
other chrome registry rtm++ bug.

Comment 39

18 years ago
Conrad: You're exactly right. To we need to move the escape/unescape code from 
nsURLHelper.cpp to xpcom/io (and get rid of nsEscape.cpp that's there) in order 
to move this functionality to nsIFile.

(Note that the reason we want to move it to nsIFile is because only the 
implementation of nsIFile really knows how to convert from a platform path to a 
file: url. The problem is evident in the fact that the code in nsStdURL is 
ifdef'd for different platforms.)

I filed bug 56354 on this.
(Reporter)

Comment 40

18 years ago
not fixed, at least on branch. reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 41

18 years ago
The code seems more or less identical to what dougt checked in.  Is there
something additional that has been patched with filelocations on the trunk that
needs to be moved to the branch?

Pink or sfraser: could you see if the problem has been fixed on the trunk?
Status: REOPENED → ASSIGNED
OS: All

Comment 42

18 years ago
Or rather, the code was identical to dougt's patch with the extra bulletproofing
to solve the absent directory trailing slash problem.
(Assignee)

Comment 43

18 years ago
The other part of the change was a change to nsProfile.cpp so that the chrome
dir was lower-case.

Comment 44

18 years ago
Ack! We need to get that on the branch then, either that or I should back out 
the nsChromeRegistry.cpp change on the branch.

Easier would be to get this rtm++ so we could check in the nsProfile.cpp change 
onto the branch.
(Assignee)

Comment 45

18 years ago
There's still more to be done. My change was not aimed at this problem and it
does not fix it. We actually need the combination of my changes (nsprofile.cpp
included) and dougt's fix. The difference is that his built the URL from the
file using nsIFileURL which is what needs to happen to fix this. My changes are
needed so it gets the right directory.

Comment 46

18 years ago
dougt or conrad... wanna take a stab at producing a revised patch now that
conrad's changes have landed?  Then we can get that checked into the branch and
trunk (and fix the case of "chrome" on the branch at the same time).
(Assignee)

Comment 47

18 years ago
I've got a patch which I'm testing now. Program starts, chrome is right, can
read mail, see ftp sites, but no web pages draw. In other words, i think it
fixes the problem WRT chrome but that is not the entire problem.

Comment 48

18 years ago
But it fixes this particular bug? It makes sure that the chrome registry is ok?
 If so, we want it! :)

Comment 49

18 years ago
Conrad: what happens when you restart a second time after installing into a 
folder that contains a '/'. Crash or runs?

> but no web pages draw

Ooh, that's doesn't sound good. :-]

(Assignee)

Comment 50

18 years ago
It will take a few more minutes to know what happens when installing. What I did
was having a running debug build which was already installed in a path with no
'/'. Then I added the '/', realaunched the browser and got what I described
before. Ran but didn't draw web pages. Before making this change and doing the
same, it just froze at the splash screen.
(Assignee)

Comment 51

18 years ago
Figured it out. It's the disk cache. When I said all was working with chrome but
no web pages drew, I then went to a page which I had not visited before, all was
well, so I tossed the cache. Like I said, what I did fixes chrome but there are
many other problems with '/' in the code. I just identified one. I'll put my
chrome patch up in a few minutes.
(Assignee)

Comment 52

18 years ago
Created attachment 16967 [details] [diff] [review]
Uses nsIFileURL to build chrome URL's #2

Comment 53

18 years ago
I filed bug 56486 on the disk cache.

Comment 54

18 years ago
a=hyatt on the new patch.  Thanks for producing this, conrad!  Any takers for an r=?

Updated

18 years ago
Whiteboard: [rtm need info] landed on trunk → [rtm need info]

Comment 55

18 years ago
I am reassigning to ccarlen, so he can use my a=.
Assignee: hyatt → ccarlen
Status: ASSIGNED → NEW
(Assignee)

Comment 56

18 years ago
Thanks, I'll try and get it into the trunk today.
Status: NEW → ASSIGNED

Comment 57

18 years ago
Please include on the branch the patch to change "Chrome" to "chrome" in 
nsProfile.cpp.  I broke user stylesheets when I landed your other changes on 
the branch, since I forgot this file.
(Assignee)

Comment 58

18 years ago
I was just getting ready to land this patch on the trunk. I need rtm+ to get
this on the branch.
(Assignee)

Comment 59

18 years ago
Created attachment 17411 [details] [diff] [review]
Patch for user "chrome" folder name

Comment 60

18 years ago
Conrad: you need a=/r= on the final attached patch to land anywhere.  You need
rtm++ to land on branch.
(Assignee)

Comment 61

18 years ago
I have r=dougt and a=hyatt for the second patch here. It's late here so that
will go in the trunk tomorrow. Then rtm++ can be decided on the rest.

Comment 62

18 years ago
There are three patches attached, which are needed to resolve this bug?  Do you
have a=/r= for each?  When ready to submit to PDT, please change [rtm need info]
to [rtm+]

Comment 63

18 years ago
I already a= the new profile patch, since it was checked in on the trunk 
already.
(Assignee)

Comment 64

18 years ago
The patch from 10/12/00 was checked into the trunk this morning. That fixes this
for good on the trunk. The branch needs that same patch (which already has
r=/a=) as well as the one from 10/17/00. As David pointed out, he already gave
a= on that as well.
Whiteboard: [rtm need info] → [rtm+]

Comment 65

18 years ago
PDT says need info: let this bake until tomorrow on the trunk, and we'll look at
it again. Please test the failure case on several machines, and let us know the
results, renominate as appropriate.
Whiteboard: [rtm+] → [rtm need info]
(Assignee)

Comment 66

18 years ago
Here are my results with builds 2000101808 from sweetlou. Trunk which has this
patch vs. branch which does not. I started by adding a '/' to the end of the
startup disk.
Trunk - Ran first time, quit, ran again, started up fine, cached pages were blank.
Branch - Ran first time, quit, tried to run again and it froze at start screen. 

Comment 67

18 years ago
With the branch bits 2000101908 MN6, installing into 
  "Macintosh HD:foobar:baz/bar"
on a second and all subsequent restarts, the browser is completely hung and
must be force quit (nothing new, just stating the branch state of affairs).

With the trunk bits 2000101908 MTrunk, installing into 
  "Macintosh HD:foo:foo/test"
I could start the browser, perform the pre-checkin tests, visited some pages,
quit, restarted ... and on restarting I could go back to those pages I had
previously visited and view their contents.

I thought I was not supposed to be able to revisit web pages with the trunk 
build. Something changed?

Comment 68

18 years ago
[By the way, I then retried on a brand new profile, and found that the cache
is not created until the second run of the browser (that is with either the
trunk or branch 10/19 builds). I'll go find out if this is a known problem.]

Comment 69

18 years ago
[Sorry, just want to be clear ... the "cache-only-on-second-run" happens for
an install into a "slash-less" folder name, as well as installation into 
'...:foo/test'. This problem is independent of having a slash in the folder 
name].
(Assignee)

Comment 70

18 years ago
John, you're right about the cache not getting made until 2nd run. I installed
2000101908 MTrunk also into "HD:foo:foo/test" and did not get a cache until the
second run. That cache problem is unrelated to this bug for certain. Although
WRT this bug, I was able to relaunch again and again with a '/' in my path. Any
more testers? How much more info is needed to go to rtm+?  

Comment 71

18 years ago
cc neeti, for the "cache only on 2nd run" issue.

Comment 72

18 years ago
On windows and linux, I find that the cache is made on the first run of the 
browser with a new profile. I do not have access to a Mac currently.

Gordon,
Can you investigate why we do not create the cache on the first run for  a new 
profile on the Mac?

Thanks,
Neeti

Comment 73

18 years ago
Filed bug 57463 for the separate issue of "cache only on 2nd run"
(Reporter)

Comment 74

18 years ago
why has this not landed on the branch or the trunk? are we still waiting for
reviews? It is still a problem with the 10/23/00 branch build.

Comment 75

18 years ago
I thought it had landed on the trunk.  At any rate, the trunk is working fine
for me on linux, while user*.css still doesn't work on the branch on linux (I
don't see the mac-specific part of the bug).  Sure would be nice to see this
land on the branch!
(Assignee)

Comment 76

18 years ago
This has been checked into the trunk. As you can see from John's comments on
2000-10-19, it's fixed on the trunk. Nominating (again) for rtm++.
Whiteboard: [rtm need info] → [rtm++]

Comment 77

18 years ago
Removing one '+' to get a different kind of attention from PDT.
Whiteboard: [rtm++] → [rtm+]

Comment 78

18 years ago
rtm++
Whiteboard: [rtm+] → [rtm++]
(Assignee)

Comment 79

18 years ago
Thanks. Now that it's rtm++, I'll get it checked into the branch as soon as I
can build and test on Linux.
Nominating relnote-user in the unlikely event that someone forgets to check this 
patch in ;-)

Gerv
Whiteboard: [rtm++] → [rtm++] relnote-user
(Assignee)

Comment 81

18 years ago
The patch was checked into the branch yesterday. The part in nsProfile.cpp which
should have been there all along was also checked in which should fix the
"Chrome" vs. "chrome" folder name problem on Unix.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
Whiteboard: [rtm++] relnote-user → [rtm++]

Comment 82

18 years ago
I don't have a mac to test this on.  Asa, could you please take QA for this?  
It wfm in my new windows trunk and branch builds with paths of C:\WINDOWS/ and 
C:/WINDOWS/
QA Contact: blakeross → asa

Comment 83

18 years ago
verified fixed on branch -- 2000102908 MN6 build, marking vtrunk, removing 
relnoteRTM (what's notable now). 

I installed into 'foo:bar/test:Netscape Folder' and could run initially. After 
a restart I was fine, and could send and receive email, browse to previously 
cached/visited pages, change preferences, edit bookmarks, customize the 
sidebar, submit a HTTP POST, start composer and addressbook, login to AIM ...
Keywords: relnoteRTM → vtrunk

Comment 84

17 years ago
Verified Fixed on Mozilla trunk build
mac 110608 Mac OS9
Setting bug to Verified and removing vtrunk keyword
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.