Closed Bug 506814 Opened 10 years ago Closed 10 years ago

Get rid of/Change GetPersistentDescriptor/SetPersistentDescriptor

Categories

(Core :: XPCOM, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: vlad, Assigned: adw)

References

Details

(Whiteboard: [ts])

Attachments

(1 file, 1 obsolete file)

We use Get/SetPersistentDescriptor in a bunch of prefs, xpti, etc. code to obtain a "persistent descriptor" for a particular file.  On all platforms other than OSX, it's just a plain pathname.  On OSX, it's some magic OS Alias reference, which is able to track files across moves, renames, whatever; it's also the cause of base64-encoded filename-blobs in our OSX profile dirs.

The other platforms get away with using native pathnames, and nothing blows up if the user moves or renames one of those files; given that they're generally in the profile dir, they probably won't.

So we should:

- change nsLocalFileOSX's Get/SetPersistent* to just use the pathname (keeping the parsing in Set for backwards compat, since it already handles pathnames that start with /)

and/or

- stop using the PersistentDescriptor stuff and just use pathnames everywhere (Windows does some stuff with UTF8 vs non UTF8 though, so maybe that's harder).

Going through this stuff makes us hit Carbon file URLs and end up super slow.
-> adw for investigation
Assignee: nobody → adw
Vlad, is this comment relevant?  A reason aliases are used on OS X rather than paths?
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#231
Attached patch patch (obsolete) — Splinter Review
This is too simplistic, right?  Who can review this?
Attachment #406075 - Flags: review?(vladimir)
Comment on attachment 406075 [details] [diff] [review]
patch

You can't change those methods. Their behavior is part of the API, with your modifications those methods do not create persistent descriptors. You should change the consumers to use path methods.
Attachment #406075 - Flags: review-
Attachment #406075 - Flags: review?(vladimir)
Thanks Josh.

Recording my thoughts:  Existing prefs (e.g., complex prefs of type nsILocalFile [1]), xpti.dat entries, etc. already contain base64-encoded aliases.  If we change over to paths, we don't know whether a given pref value, xpti.dat entry, etc. is an alias or path -- whether to set persistentDescriptor or call initWithPath.  A simple solution is to first assume it's an alias, and if SetPersistentDescriptor fails because the data is not base64-encoded, then it must be a path.  (OS X's InitWithPath could do this check, or a new method InitWithPathOrPersistentDescriptor, or worst rewriting all consumers to do it themselves.)  Unix and Windows won't feel the difference, but there's extra cost at runtime on OS X.  Or, is there some migration path to hook into, and if so, can the locations on disk of all existing base64-encoded aliases be determined during migration?  Is this even worth worrying about?

[1] http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#281
(In reply to comment #2)
> Vlad, is this comment relevant?  A reason aliases are used on OS X rather than
> paths?
> http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#231

That was true back in MacOS pre-X days; I honestly don't think we care any more, and shouldn't treat OSX any different than we do any other Unix system.
I agree with vlad: paths should *be* the persistent descriptors we use. The only issue I have is whether we need to provide any upgrade path, in case these persistent descriptors are stored anywhere in prefs that persists between versions. It's not an issue for fastload or the other caches.
Are prefs really the only place persistent descriptors are persistently stored?  Looking through mxr they are also potentially stored in at least:

* extensions.ini
* download manager DB: mime type => path of preferred application
* profiles.ini
* nsNetscapeProfileMigratorBase, nsProfileMigrator get descriptors from some NSPR registry

1) Are these all caches?
2) If descriptors are persistently stored, and it appears they certainly can be, then there's no choice but to do a migration (or check at every Set per comment 5).  Where's the best place to do it?  Is it possible to hook into .autoreg/compatibility.ini invalidation, or can the updater do stuff like this?
I don't think it matters. A path-style persistent descriptor will always start with /. The old style will always start with an alphanumeric. Just detect which kind you have in ReadPersistentDescriptor, keep the code to load the old kind, but only write the new kind.
Attached patch patch v2Splinter Review
This should be it then...
Attachment #406075 - Attachment is obsolete: true
Attachment #406364 - Flags: review?(joshmoz)
Attachment #406364 - Flags: review?(joshmoz) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/5372b453a52e
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Blocks: 574952
You need to log in before you can comment on or make changes to this bug.