Closed Bug 54097 Opened 24 years ago Closed 24 years ago

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

Categories

(Core Graveyard :: Skinability, defect, P2)

Product:
Core Graveyard
Component:
Skinability
Platform:
PowerPC
All
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Future

People

(Reporter: mikepinkerton, Assigned: ccarlen)

Details

(Keywords: crash, helpwanted, Whiteboard: [rtm++])

Attachments

(3 files)

makes chrome registry use nsIFile (and necko for url parsing.)
5.70 KB, patch
Details | Diff | Splinter Review
Uses nsIFileURL to build chrome URL's #2
1.67 KB, patch
Details | Diff | Splinter Review
Patch for user "chrome" folder name
715 bytes, patch
Details | Diff | Splinter Review 
- 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.
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
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.

Actually, I think the chrome RDF files may be generated correctly, but the code 
that uses them has bad slash habits.

rtm nomination.
Keywords: rtm
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
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.

->dougt, need to implement additional nsiFile methods & give bug back to hyatt.
Assignee: hyatt → dougt
warren, were you doing something like this?
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.
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
marking helpwanted, as we are unlikely to be able to get to this in time.
Keywords: helpwanted
need a good relnote here...anyone?
Keywords: relnoteRTM
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
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.
Status: NEW → ASSIGNED
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]
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?
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?
The diffs are not really that big, and look pretty safe to me. hyatt?

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.

Why the changes from "aFileURL" to "outFileURL"? When in Rome, and all. Other
than that, a=waterson.
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.

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
cc'ing jrgm
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.

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.

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
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
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.


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

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.

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

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.

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?

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
Closed: 24 years ago
Resolution: --- → FIXED
so this is on both the trunk and the branch?
Yes.  It accidentally came along for the ride on the branch when I checked in my
other chrome registry rtm++ bug.

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.
not fixed, at least on branch. reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Or rather, the code was identical to dougt's patch with the extra bulletproofing
to solve the absent directory trailing slash problem.

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

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.

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.

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

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.

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

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. :-]


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.

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.

I filed bug 56486 on the disk cache.

a=hyatt on the new patch.  Thanks for producing this, conrad!  Any takers for an r=?
Whiteboard: [rtm need info] landed on trunk → [rtm need info]
I am reassigning to ccarlen, so he can use my a=.
Assignee: hyatt → ccarlen
Status: ASSIGNED → NEW
Thanks, I'll try and get it into the trunk today.
Status: NEW → ASSIGNED
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.

I was just getting ready to land this patch on the trunk. I need rtm+ to get
this on the branch.

Conrad: you need a=/r= on the final attached patch to land anywhere.  You need
rtm++ to land on branch.
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.

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+]
I already a= the new profile patch, since it was checked in on the trunk 
already.

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+]
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]
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. 
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?
[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.]


[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].
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+?  
cc neeti, for the "cache only on 2nd run" issue.

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
Filed bug 57463 for the separate issue of "cache only on 2nd run"
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.
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!
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++]
Removing one '+' to get a different kind of attention from PDT.
Whiteboard: [rtm++] → [rtm+]
rtm++
Whiteboard: [rtm+] → [rtm++]
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
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
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Whiteboard: [rtm++] relnote-user → [rtm++]
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
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: