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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Simon Fraser [no longer active]]

It seems that there is a problem with RDF persistence when the user has a / in 
their hard drive name (see dependent bugs).

Comment 1

[Tracy Walker [:tracy]]

Thanks Simon. 
I added that latest bug to the dependency list.

Comment 2

[David Hyatt]

--> waterson

Comment 3

[Mike Pinkerton (not reading bugmail)]

with all due respect, this is very common on mac. i don't see how you can future
this bug?

Comment 4

[Steve Dagley]

waterson is going off on sabbatical -> rjc

Comment 5

[Robert John Churchill]

Attachment: Patch

Clean up RDF a bit by new NS_NewFileURI() usage, and fix wanky encoding in
Necko of Mac file paths

Comment 6

[Chris Waterson]

Comment on attachment 64960 [details] [diff] [review]
Patch

sr=waterson

Comment 7

[Robert John Churchill]

Fixed.

Comment 8

[R.K.Aa.]

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 ?

Comment 9

[Robert John Churchill]

Looking...  (have an old Linux tree, have to update)

Comment 10

[Robert John Churchill]

Fix potential build bustage due to case issue (which the Mac is leniant about)

Update mozilla/rdf/datasource/src/nsLocalStore.cpp

Comment 11

[Simon Fraser [no longer active]]

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

Comment 12

[Robert John Churchill]

sfraser:  I caught that right before the checkin and tweaked it.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/rdf/datasource/src&command=DIFF_FRAMESET&file=nsLocalStore.cpp&rev1=1.39&rev2=1.40&root=/cvsroot

Comment 13

[Darin Fisher]

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

Comment 14

[Robert John Churchill]

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>

Comment 15

[Darin Fisher]

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

Comment 16

[Robert John Churchill]

Darin, I'll move your comments over to bug # 36972

Comment 17

[Robert John Churchill]

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]

Comment 18

[Robert John Churchill]

Targetting moz 0.9.9

Comment 19

[Robert John Churchill]

Attachment: Entire patch (includes patches from 36972 & 78013)

Entire patch (includes patches from 36972 & 78013, as well as two colon/slash
hack/fixes in Necko-land.

Comment 20

[Darin Fisher]

Comment on attachment 66215 [details] [diff] [review]
Entire patch (includes patches from 36972 & 78013)

r/sr=darin

Comment 21

[Robert John Churchill]

Fixed.

Comment 22

[John Morrison]

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?

Comment 23

[John Morrison]

filed bug http://bugzilla.mozilla.org/show_bug.cgi?id=121546 for the missing 
overlayinfo

Comment 24

[John Morrison]

these changes were backed out since dist/bin/chrome/overlayinfo/* was not
being created at browser startup (bug 121546).

Comment 25

[Robert John Churchill]

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.

Comment 26

[Robert John Churchill]

Attachment: Basically same patch as before, merged with darin's changes in nsIOServiceMac.cpp

Basically same patch as before, merged with darin's changes in
nsIOServiceMac.cpp which no longer seems to cause bug # 78013

Comment 27

[Robert John Churchill]

jrgm: Care to give this a spin as well?

Comment 28

[John Morrison]

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

Comment 29

[Robert John Churchill]

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?

Comment 30

[Darin Fisher]

dougt: nsIFile question for you... see comment #29.

Comment 31

[Doug Turner (:dougt)]

nsIFile::Create() did not carry this forward.  However, there is nothing in the
idl contract which would prevent an implementation from doing this.

Comment 32

[Brendan Eich [:brendan]]

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

Comment 33

[Doug Turner (:dougt)]

The Mac platform does not do this, unless FSpCreate does recursive direction
creation.

Comment 34

[Robert John Churchill]

Doug meant FSpDirCreate, BTW.

Comment 35

[Steve Dagley]

nsLocalFileMac will create all leaf nodes of a given path when told to create a 
file

Comment 36

[Robert John Churchill]

Attachment: Same patch as before but explicitly call nsIFile::Create() before trying to create an output stream

Sprinkly in a few nsIFile::Create()s for yucks.

Comment 37

[John Morrison]

this patch is creating the overlayinfo/* on win32 and linux (current trunk 
builds).

Comment 38

[Robert John Churchill]

Great.  (BTW, thanks for all the investigation/debugging/testing/etc re: this
bug, jrgm!)

Comment 39

[John Morrison]

No worries. (I might have learned some things about chrome registry that I 
never knew in the process :-)

Comment 40

[Robert John Churchill]

Darin, care to r= this bug one more time?  :)

Comment 41

[Darin Fisher]

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

Comment 42

[Robert John Churchill]

Fix checked in.