Closed Bug 241384 Opened 20 years ago Closed 20 years ago

nsProfileDirServiceProvider should be GRE friendly

Categories

(Core Graveyard :: Embedding: GRE Core, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mpgritti, Assigned: mpgritti)

References

Details

Attachments

(2 files, 9 obsolete files)

Problems are usage of nsString and nsIAtom. They both seem easily fixable.
Attached patch proposed fix (obsolete) — Splinter Review
Attached patch port also nsProfileLock (obsolete) — Splinter Review
Attachment #146857 - Attachment is obsolete: true
Attachment #146866 - Flags: review?(ccarlen)
I'm fine with replacing nsIAtom usage with strcmp, but the replacement for
NS_NAMED_LITERAL_STRING seems a bit too ugly. Since creating Unicode string
literals seems to be such a basic nescesity, can't that be added to nsEmbedString?
why is this code, which is part of core gecko, using nsEmbedString?  that seems
very wrong indeed.  what am i missing?
ok, the answer to my question appears to be that embedders take this static lib
and use it too.  that's unfortunate since we also link it into profile.dll.  in
that case, there might be symbol conflicts between the nsAC?String defined in
nsStringAPI.h and the nsAC?String defined in nsAC?String.h ... my concern is
that other parts of profile.dll uses the internal definition of nsAC?String.

i'd recommend using #ifdefs to compile profdirserviceprovider conditionally
using either nsEmbedString or nsString.  then compile it twice, and link the
nsString version into profile.dll and let the other version be used by embedders.
Attachment #146866 - Flags: review?(ccarlen)
Finally I dont think this is the right way to go. I dont think we should bind
embedding to the old toolkit profile stuff. For one, the per user compreg.dat
would be useful to embedding too.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → WONTFIX
No longer blocks: 205425
Reopening per discussion with brendan and bsmedberg on bug 205425. Create a
standalone version of the profile code seem to be preferable over duplicating
the code right away
Blocks: 205425
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
I tried to avoid NS_NAMED_LITERAL_STRING mess where possible.
Attachment #146866 - Attachment is obsolete: true
Attachment #147159 - Flags: review?(ccarlen)
Comment on attachment 147159 [details] [diff] [review]
create a standalone version of the static library

>Index: dirserviceprovider/src/Makefile.in
>===================================================================

> VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
>+include $(srcdir)/objs.mk

objs.mk isn't in the diff, but I guess it just sets up
MODULES_PROFILEDIRSERVICE_SRC_LCSRCS?

> 
> MODULE		= profdirserviceprovider
> LIBRARY_NAME	= profdirserviceprovider_s
>@@ -53,11 +54,7 @@
> 		  $(NULL)
> endif
> 
>-CPPSRCS		= nsProfileDirServiceProvider.cpp
>-
>-ifdef MOZ_PROFILELOCKING
>-CPPSRCS		+= nsProfileLock.cpp
>-endif
>+CPPSRCS		= $(MODULES_PROFILEDIRSERVICE_SRC_LCSRCS)
> 
> # we don't want the shared lib
> FORCE_STATIC_LIB = 1
>Index: dirserviceprovider/src/nsProfileDirServiceProvider.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/profile/dirserviceprovider/src/nsProfileDirServiceProvider.cpp,v
>retrieving revision 1.14
>diff -u -r1.14 nsProfileDirServiceProvider.cpp
>--- dirserviceprovider/src/nsProfileDirServiceProvider.cpp	27 Feb 2004 20:11:01 -0000	1.14
>+++ dirserviceprovider/src/nsProfileDirServiceProvider.cpp	27 Apr 2004 18:14:38 -0000
>@@ -38,8 +38,6 @@
> 
> #include "nsProfileDirServiceProvider.h"
> #include "nsProfileLock.h"
>-#include "nsIAtom.h"
>-#include "nsStaticAtom.h"
> #include "nsILocalFile.h"
> #include "nsDirectoryServiceDefs.h"
> #include "nsAppDirectoryServiceDefs.h"
>@@ -54,6 +52,21 @@
> 
> // File Name Defines
> 
>+#ifdef STANDALONE_PROFILEDIRSERVICE
>+#define PREFS_FILE_50_NAME           nsEmbedCString("prefs.js")
>+#define USER_CHROME_DIR_50_NAME      nsEmbedCString("chrome")
>+#define LOCAL_STORE_FILE_50_NAME     nsEmbedCString("localstore.rdf")
>+#define HISTORY_FILE_50_NAME         nsEmbedCString("history.dat")
>+#define PANELS_FILE_50_NAME          nsEmbedCString("panels.rdf")
>+#define MIME_TYPES_FILE_50_NAME      nsEmbedCString("mimeTypes.rdf")
>+#define BOOKMARKS_FILE_50_NAME       nsEmbedCString("bookmarks.html")
>+#define DOWNLOADS_FILE_50_NAME       nsEmbedCString("downloads.rdf")
>+#define SEARCH_FILE_50_NAME          nsEmbedCString("search.rdf" )
>+#define MAIL_DIR_50_NAME             nsEmbedCString("Mail")
>+#define IMAP_MAIL_DIR_50_NAME        nsEmbedCString("ImapMail")
>+#define NEWS_DIR_50_NAME             nsEmbedCString("News")
>+#define MSG_FOLDER_CACHE_DIR_50_NAME nsEmbedCString("panacea.dat")
>+#else
> #define PREFS_FILE_50_NAME           NS_LITERAL_CSTRING("prefs.js")
> #define USER_CHROME_DIR_50_NAME      NS_LITERAL_CSTRING("chrome")
> #define LOCAL_STORE_FILE_50_NAME     NS_LITERAL_CSTRING("localstore.rdf")
>@@ -67,25 +80,7 @@
> #define IMAP_MAIL_DIR_50_NAME        NS_LITERAL_CSTRING("ImapMail")
> #define NEWS_DIR_50_NAME             NS_LITERAL_CSTRING("News")
> #define MSG_FOLDER_CACHE_DIR_50_NAME NS_LITERAL_CSTRING("panacea.dat")

Some typedefs could save a number of #ifdefs throughout this code.

#if STANDALONE_PROFILEDIRSERVICE
typedef nsEmbedString nsUniversalString;
#else
typedef nsString nsUniversalString;
#endif

If you can come up with a better name than nsUniversalString ;-)

>Index: dirserviceprovider/src/nsProfileLock.cpp
>===================================================================
>-nsresult nsProfileLock::Lock(nsILocalFile* aFile)
>-{
>-#if defined (XP_MACOSX)
>-    NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, ".parentlock");
>-    NS_NAMED_LITERAL_STRING(OLD_LOCKFILE_NAME, "parent.lock");
>-#elif defined (XP_UNIX)
>-    NS_NAMED_LITERAL_STRING(OLD_LOCKFILE_NAME, "lock");
>-    NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, ".parentlock");
>+#ifdef STANDALONE_PROFILEDIRSERVICE
>+
>+#if defined (XP_UNIX)
>+    #define OLD_LOCKFILE_NAME nsEmbedCString("lock")
>+    #define LOCKFILE_NAME     nsEmbedCString(".parentlock")
> #else
>-    NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, "parent.lock");
>+    #define LOCKFILE_NAME     nsEmbedCString("parent.lock")
> #endif
> 
>+#else
>+
>+#if defined (XP_UNIX)
>+    #define OLD_LOCKFILE_NAME NS_LITERAL_CSTRING("lock")
>+    #define LOCKFILE_NAME     NS_LITERAL_CSTRING(".parentlock")
>+#else
>+    #define LOCKFILE_NAME     NS_LITERAL_CSTRING("parent.lock")
>+#endif
>+
>+#endif

This changes the definition of OLD_LOCKFILE_NAME for XP_MACOSX. With your
change, OSX gets the UNIX def, which is different.

>--- /dev/null	2004-02-23 22:02:56.000000000 +0100
>+++ dirserviceprovider/standalone/Makefile.in	2004-04-27 12:46:14.000000000 +0200
>@@ -0,0 +1,69 @@
>+# ***** BEGIN LICENSE BLOCK *****
<snip>
>+# The Original Code is mozilla.org code.
>+#
>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.

Netscape? I don't think so ;-)

>+# Portions created by the Initial Developer are Copyright (C) 2002

The date is wrong. Check this on all added files.


Minusing for the >-#if defined (XP_MACOSX) and the license problems. See if you
can clean things up with some #defines and typedefs but I won't require that.
Attachment #147159 - Flags: review?(ccarlen) → review-
>This changes the definition of OLD_LOCKFILE_NAME for XP_MACOSX. With your
>change, OSX gets the UNIX def, which is different.

Yeah, but my code is using OLD_LOCKFILE_NAME only for UNIX. We need to special
case that code already for the conversion in macos code...

See:

+#ifdef STANDALONE_PROFILEDIRSERVICE
+        nsEmbedString oldLockFileName;
+        NS_CStringToUTF16(nsEmbedCString("parent.lock"),
+                          NS_CSTRING_ENCODING_ASCII, oldLockFileName);
+#else
+        NS_NAMED_LITERAL_STRING(oldLockFileName, "parent.lock");
+#endif

The only alternative I can think of is a monster like this:

#ifdef STANDALONE_PROFILEDIRSERVICE
 #if defined (XP_MACOSX)
    NS_CStringToUTF16(nsEmbedCString(".parentlock"),
                      NS_CSTRING_ENCODING_ASCII, LOCKFILE_NAME);
    NS_CStringToUTF16(nsEmbedCString("parent.lock"),
                      NS_CSTRING_ENCODING_ASCII, OLD_LOCKFILE_NAME);
 #elif defined (XP_UNIX)
    NS_CStringToUTF16(nsEmbedCString(".parentlock"),
                      NS_CSTRING_ENCODING_ASCII, LOCKFILE_NAME);
    NS_CStringToUTF16(nsEmbedCString("lock"),
                      NS_CSTRING_ENCODING_ASCII, OLD_LOCKFILE_NAME);
 #else
    NS_CStringToUTF16(nsEmbedCString("parent.lock"),
                      NS_CSTRING_ENCODING_ASCII, LOCKFILE_NAME);
 #endif
#else
 #if defined (XP_MACOSX)
    NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, ".parentlock");
    NS_NAMED_LITERAL_STRING(OLD_LOCKFILE_NAME, "parent.lock");
 #elif defined (XP_UNIX)
    NS_NAMED_LITERAL_STRING(OLD_LOCKFILE_NAME, "lock");
    NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, ".parentlock");
 #else
    NS_NAMED_LITERAL_STRING(LOCKFILE_NAME, "parent.lock");
 #endif
#endif

Do you want me to make it this way ? Or do you see a better way ?
Attachment #147159 - Attachment is obsolete: true
Comment on attachment 147402 [details] [diff] [review]
fix license problems and add obj.mk

I did not make changes to the profile lock file stuff, see my previous comment.
The code works correctly I think, though if you think it's nicer using the big
block of defines or yet another way let me know and I'll happily fix it.
Attachment #147402 - Flags: review?(ccarlen)
Comment on attachment 147648 [details] [diff] [review]
Alternative. More verbose but maybe more clear.

Feel free to review the one you like better. I dont have a strong preference.
No one of them is particularly nice but it's either this or duplicating profile
code :/ (bug 205425)
Attachment #147648 - Flags: review?(ccarlen)
I forgot to mention that the only actual difference between the two patches is
in nsProfileLock::Lock. Attachment 147402 [details] [diff] uses CString when possible (so that we
dont need conversion) and special case mac old lock. Attachment 147648 [details] [diff] just port
the string definition at the top to embed string for the standalone case.
Attachment #147653 - Flags: review?(ccarlen)
Attached patch Third alternative (update) (obsolete) — Splinter Review
Attachment #147653 - Attachment is obsolete: true
Attachment #147653 - Flags: review?(ccarlen)
Comment on attachment 147656 [details] [diff] [review]
Third alternative (update)

Hopefully the last one, sorry for the spam. FWIW this is my favourite.
Attachment #147656 - Flags: review?(ccarlen)
Sorry for the delay, was away for a few days.

Looks better. Two things:

+#ifdef STANDALONE_PROFILEDIRSERVICE
+  const char *lockPath = nsEmbedCString(lockFilePath).get();
+#else
+  const char *lockPath = PromiseFlatCString(lockFilePath).get();
+#endif
+ 
+  mLockFileDesc = open(lockPath, O_WRONLY | O_CREAT | O_TRUNC, 0666);

Take a look at
http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTPromiseFlatString.h#60
According to that, in the !STANDALONE_PROFILEDIRSERVICE case, lockPath is a
dangling pointer. There are two instances of this in the patch.

This is good:

+#ifdef STANDALONE_PROFILEDIRSERVICE
+    #define NAMED_LITERAL_STRING(stringname, filename) \
+        nsEmbedString stringname; \
+        NS_CStringToUTF16(nsEmbedCString(filename), \
+                          NS_CSTRING_ENCODING_ASCII, stringname);
+#else
+    #define NAMED_LITERAL_STRING NS_NAMED_LITERAL_STRING
+#endif

I wish there was a corresponding CSTRING version and and was used in
nsProfileDirServiceProvider.cpp.

Check out the dangling pointer issue (Darin, is that comment still true?) and
r=ccarlen.
> +#ifdef STANDALONE_PROFILEDIRSERVICE
> +  const char *lockPath = nsEmbedCString(lockFilePath).get();
> +#else
> +  const char *lockPath = PromiseFlatCString(lockFilePath).get();
> +#endif
> + 
> +  mLockFileDesc = open(lockPath, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> 
> Take a look at
>
http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsTPromiseFlatString.h#60
> According to that, in the !STANDALONE_PROFILEDIRSERVICE case, lockPath is a
> dangling pointer. There are two instances of this in the patch.

Ouch, good catch. In the other case I've just put back the old code (but
defined). For this I had to add an intermediate const char. I'm going to ask sr
to darin so he can check this is ok.
/me needs a clue about strings

> I wish there was a corresponding CSTRING version and and was used in
> nsProfileDirServiceProvider.cpp.

Done.
Attachment #147648 - Flags: review?(ccarlen)
Attachment #147402 - Attachment is obsolete: true
Attachment #147402 - Flags: review?(ccarlen)
Attached patch Fix dangling pointer (obsolete) — Splinter Review
Attachment #147648 - Attachment is obsolete: true
Attachment #147656 - Attachment is obsolete: true
Comment on attachment 147817 [details] [diff] [review]
Fix dangling pointer

Darin, could you please also check that the changes at nsProfileLock.cpp @@
-217,9 +221,16 @@ are correct ?
Attachment #147817 - Flags: superreview?(darin)
+  const char *lockPath = nsEmbedCString(lockFilePath).get();

this looks like a dangling pointer as well (the nsEmbedCString object will be
destroyed after this line)
Attachment #147817 - Flags: superreview?(darin)
Attachment #147817 - Attachment is obsolete: true
Attachment #147920 - Flags: superreview?(darin)
Comment on attachment 147920 [details] [diff] [review]
Really fix all dangling pointers

please do something like

typedef nsEmbedString nsString;

in the STANDALONE_PROFILEDIRSERVICE case.  that way you avoid extra #ifdef's. 
sound good?  same goes for nsCAutoString I think.

also, it looks like LITERAL_CSTRING doesn't need to take an argument in the
nsEmbedCString case.

ugh.. nsEmbedCString should have an IsEmpty() method.  bad darin!

use the EmptyString() function instead of nsString().

And maybe "#define EmptyString nsEmbedString" in the
STANDALONE_PROFILEDIRSERVICE case.  the fewer #ifdefs scattered throughout the
code the beter, right? ;-)

i would also recommend making PromiseFlatCString equate to nsEmbedCString ctor,
and typedef nsPromiseFlatCString to nsEmbedCString.  again, this eliminates
more #ifdefs.

seems like it would be good to put all of these #defines and typedefs in one of
the profile header files.

i have no problem with you #define'ing NS_NAMED_LITERAL_STRING in terms of the
embedding string API.  there's no need to invent custom macros.  just use the
standard macro names so you don't have to touch the body of the code.
Attachment #147920 - Flags: superreview?(darin) → superreview-
> typedef nsEmbedString nsString;
> 
> in the STANDALONE_PROFILEDIRSERVICE case.  that way you avoid extra #ifdef's. 
> sound good?  same goes for nsCAutoString I think.

Frankly I dont like that approach. It hides the fact that you can use only the
nsString/nsEmbedString api intersection. If someone tries to modify
nsProfileLock.cpp using nsString only api I think he will get really confused.
IHMO it's much better to have some additional ugly defines than obscure it this way.

Anyway, it's up to you. I'm going to attach a patch that does it in the way you
suggested. I had to add nsProfileCore.h. nsProfileLock.h doesnt sounds like the
right place to put the defines to me. nsProfileDirServiceProvider.h is public
and the typedefs can cause conflicts: for example I was getting a conflict
between nsColor.h and nsProfileDirServiceProvider.h definition of nsString in
gtkmozembed.
Attachment #147974 - Flags: superreview?(darin)
Comment on attachment 147974 [details] [diff] [review]
Different approach suggested by darin

I understand your concerns about changing the meaning of well-known string
types, but I think that can be solved by ample documentation.  Even a
README.txt file might be good :-)

At the very least, I think you should comment what's going on in
nsProfileCore.h.

Also for the record, we do a lot of this typedef'ing and #define'ing sort of
thing in modules/libjar so that we can support two builds of that module: 1 for
the core gecko and 1 for use by the installer (which does not has xpcom
support).

I'd also like to get feedback/MOA= from Conrad on this change.
Comment on attachment 147974 [details] [diff] [review]
Different approach suggested by darin

I like this approach. I asked for typedefs a few reviews ago, so I'm happy.

I think it's OK to hide this stuff behind #defines and typedefs because the
STANDALONE_PROFILEDIRSERVICE case is part of the default build. If somebody
uses an nsString method not supported by nsEmbedString, they will have no
choice but to know about it. But, please file a bug about the lack of IsEmpty()
on nsEmbedString.

Please rename nsProfileCore.h -> nsProfileStringTypes.h, put a README in that
file, and r=ccarlen.
Attachment #147974 - Flags: review+
Attachment #147974 - Flags: superreview?(darin)
Attachment #147974 - Attachment is obsolete: true
Attachment #148131 - Flags: superreview?(darin)
Attachment #148131 - Flags: superreview?(darin) → superreview+
Damn, there is a collision between this and 232691 :(
there don'T seem to be any conflicts here - checked in
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.