Closed Bug 113894 Opened 19 years ago Closed 18 years ago

RDF persistence breaks when there is a / in the file path

Categories

(Core :: XUL, defect, major)

PowerPC
Mac System 9.x
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla0.9.9

People

(Reporter: sfraser_bugs, Assigned: mozilla)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss)

Attachments

(1 file, 3 obsolete files)

It seems that there is a problem with RDF persistence when the user has a / in 
their hard drive name (see dependent bugs).
Blocks: 97141
No longer blocks: 97141
Blocks: 97143
Blocks: 97141
Blocks: 97639
Thanks Simon. 
I added that latest bug to the dependency list.
Blocks: 110632
--> waterson
Assignee: hyatt → waterson
Target Milestone: --- → Future
with all due respect, this is very common on mac. i don't see how you can future
this bug?
Severity: normal → major
Keywords: dataloss, nsbeta1
waterson is going off on sabbatical -> rjc
Assignee: waterson → rjc
Target Milestone: Future → ---
Attached patch Patch (obsolete) — Splinter Review
Clean up RDF a bit by new NS_NewFileURI() usage, and fix wanky encoding in
Necko of Mac file paths
Status: NEW → ASSIGNED
Comment on attachment 64960 [details] [diff] [review]
Patch

sr=waterson
Attachment #64960 - Flags: superreview+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
getting this now when trying to build on Linux:

nsLocalStore.cpp:47:26: nsIIoService.h: No such file or directory
gmake[5]: *** [nsLocalStore.o] Error 1

Typo for nsIOService.h ?
Looking...  (have an old Linux tree, have to update)
Fix potential build bustage due to case issue (which the Mac is leniant about)

Update mozilla/rdf/datasource/src/nsLocalStore.cpp
Ugh. This went in already, but I object to this line:

+    if (! aFile->Exists(&fileExistsFlag) && (!fileExistsFlag)) {

! aFile->Exists(&fileExistsFlag) needs to be changed to 
NS_SUCCEEDED(aFile->Exists(&fileExistsFlag))
rjc: what is the reasoning behind the change in nsIOServiceMac.cpp?  from
section 3.3 of RFC2396, URL paths are allowed to contain ':' characters.  does
this change have something to do with '/' being a valid character in Mac file
paths?  if so, then we should really be escaping '/' chars before calling
SwapSlashColon.

also, i don't understand why you are using the URL spec to initialize a
nsFileSpec... why not use nsIURI::path instead?

moreove, isn't the conversion from nsIFile -> filePath -> nsFileSpec potentially
lossy on the Mac?  what if two volumes share the same name?  it seems like it
would be better to just use the given nsIFile directly, but perhaps there is too
much legacy code in nsLocalStore.cpp that uses nsFileSpec )-:
Darin: The change was made inside of GetURLSpecFromFile().  A Macintosh file can 
contain slashes but can not contain a colon (the OS doesn't allow it) as its the 
separator.  [We'll leave Mac OS X out of the equation; although technically at 
the BSD layer its Unix and is using slashes, it does a conversion when going up 
to the Mac layer(s).]

When converting from a file, the code was erroneously just swapping colons and 
slashes.  So, if you had a volume name such as "Mac/Disk", it became "Mac:Disk" 
... thus, this bug.

Since you'd never actually find a file with a colon in it, after the above wanky 
conversion happened, it was a simple one line hack to convert all the colons 
(really original slashes) to "%2F", i.e. an encoded slash.

If you want to rewrite that function to make the encoding process more clear and 
logical, I certainly wouldn't stand in your way.  :)  However, again, you'll 
never encounter a Mac file path containing a colon, thus the one-liner I added 
works.

As for nsFileSpec usage in nsLocalStore, yes, it should all go away... its very 
"legacy".  There's a bug on it somewhere;  however, it isn't this one.  <grin>
take a look at NS_NewLocalFileInputStream and NS_NewLocalFileOutputStream in
nsIFileStreams.idl.  you might also want to wrap the output stream in a buffered
output stream (see NS_NewBufferedOutputStream).
Darin, I'll move your comments over to bug # 36972
Reopen (due to bug # 120617).

Short story
-----------
Basically, to make this fully work, we need to:

1 get all usages of nsFileSpec out of RDF (this bug and # 78013 and # 36972)
2 fix slash/colon encoding in Necko

at the same time.

Regarding #2, there are two spots in nsIOServiceMac.cpp where SwapSlashColon()
is used. Both spots need to be fixed.


Long story
----------
nsFileSpec, like Necko at the moment, is broken regarding its handling of
slashes/colons.  Basically, both use the wanky SwapSlashColon() routine which is
bogus. [And, nsFileSpec is deprecated, it'll no one's ever going to fix it.]

After the fixes for this bug went in, here's what was happening: given a
volume/folder with a slash in it on Mac, RDF would get a file:// out of an
nsIFile which would have the slash encoded as a %2F which was good.  However,
due to bug # 78013, nsRDFXMLDataSource would take that URL and try and stuff it
into an nsFileSpec which would do the wrong thing [that is, decode the "%2F" to
a slash, and then use SwapSlashColon() which would convert a valid slash into a
colon, causing bug # 120617]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 36972, 78013
Targetting moz 0.9.9
Status: REOPENED → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Entire patch (includes patches from 36972 & 78013, as well as two colon/slash
hack/fixes in Necko-land.
Attachment #64960 - Attachment is obsolete: true
Comment on attachment 66215 [details] [diff] [review]
Entire patch (includes patches from 36972 & 78013)

r/sr=darin
Attachment #66215 - Flags: review+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
the build on 'comet' does not have a directory dist/bin/chrome/overlayinfo
and so, no overlays. rjc: can you have a look at this?
filed bug http://bugzilla.mozilla.org/show_bug.cgi?id=121546 for the missing 
overlayinfo
these changes were backed out since dist/bin/chrome/overlayinfo/* was not
being created at browser startup (bug 121546).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It looks like we are hitting the same weirdness as in bug # 78013. Chris tried
to check in similiar fixes a while ago and they had to be backed out... but then
tingley couldn't reproduce.  Funky.
Basically same patch as before, merged with darin's changes in
nsIOServiceMac.cpp which no longer seems to cause bug # 78013
Attachment #66215 - Attachment is obsolete: true
jrgm: Care to give this a spin as well?
I tried this on win32, and, for the situation where bin/chrome/overlayinfo/*
does not already exist (e.g., a clobber build), then this patch still fails 
to create this part of the chrome registry, and the build will be "overlay
challenged" after a restart.

I stepped through the code on win32 and the problem is that the code wants to
call Flush() for '...\overlayinfo\navigator\content\overlays.rdf' where the
directory tree '...\overlayinfo\navigator\content\' doesn't exist
yet. "NS_NewLocalFileOutputStream(getter_AddRefs(out), file);" will not (on
win32, at least) create the required path if it does not yet exist.

[I confirmed that the current code-plus-patch would work if the desired 
path already exists, by breaking and then hand creating the path (except 
for the actual file 'overlays.rdf') before calling
NS_NewLocalFileOutputStream and descendants; in this situation, the file 
will be created and serialized into].
One could learn to really dislike nsFileSpec.  Good detective work, jrgm!

Perhaps this is the magical reason --  the code used to do:
    nsFileURL url(mOriginalURLSpec, PR_TRUE);

where the PR_TRUE looks to be "inCreateDirs" (which creates all the intermediate
directories, apparently)

I wonder if nsLocalFile::Create() will create multiple levels of intermediate
directories, or only a leaf directory.  Darin?
dougt: nsIFile question for you... see comment #29.
nsIFile::Create() did not carry this forward.  However, there is nothing in the
idl contract which would prevent an implementation from doing this.
dougt, I see code in nsLocalFile::Create for Unix to do the right thing:
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsLocalFileUnix.cpp#401 -- it
looks to my untrained-on-Win32-API eyes as though
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsLocalFileWin.cpp#760 and nearby
does the same.

/be
The Mac platform does not do this, unless FSpCreate does recursive direction
creation.
Doug meant FSpDirCreate, BTW.
nsLocalFileMac will create all leaf nodes of a given path when told to create a 
file
Sprinkly in a few nsIFile::Create()s for yucks.
Attachment #67055 - Attachment is obsolete: true
this patch is creating the overlayinfo/* on win32 and linux (current trunk 
builds).
Great.  (BTW, thanks for all the investigation/debugging/testing/etc re: this
bug, jrgm!)
No worries. (I might have learned some things about chrome registry that I 
never knew in the process :-)
Darin, care to r= this bug one more time?  :)
Comment on attachment 67177 [details] [diff] [review]
Same patch as before but explicitly call nsIFile::Create() before trying to create an output stream

r/sr=darin
Attachment #67177 - Flags: superreview+
Fix checked in.
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.