241384 - nsProfileDirServiceProvider should be GRE friendly

Status: RESOLVED FIXED

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

Description

[Marco Pesenti Gritti]

Problems are usage of nsString and nsIAtom. They both seem easily fixable.

Comment 1

[Marco Pesenti Gritti]

Attachment: proposed fix

Comment 2

[Marco Pesenti Gritti]

Attachment: port also nsProfileLock

Comment 3

[Conrad Carlen (not reading bugmail)]

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?

Comment 4

[Darin Fisher]

why is this code, which is part of core gecko, using nsEmbedString?  that seems
very wrong indeed.  what am i missing?

Comment 5

[Darin Fisher]

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.

Comment 6

[Marco Pesenti Gritti]

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.

Comment 7

[Marco Pesenti Gritti]

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

Comment 8

[Marco Pesenti Gritti]

Attachment: create a standalone version of the static library

I tried to avoid NS_NAMED_LITERAL_STRING mess where possible.

Comment 9

[Conrad Carlen (not reading bugmail)]

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.

Comment 10

[Marco Pesenti Gritti]

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

Comment 11

[Marco Pesenti Gritti]

Attachment: fix license problems and add obj.mk

Comment 12

[Marco Pesenti Gritti]

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.

Comment 13

[Marco Pesenti Gritti]

Attachment: Alternative. More verbose but maybe more clear.

Comment 14

[Marco Pesenti Gritti]

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)

Comment 15

[Marco Pesenti Gritti]

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.

Comment 16

[Marco Pesenti Gritti]

Attachment: Third alternative. Like 147648 but use a NAMED_LITERAL_STRING macro.

Comment 17

[Marco Pesenti Gritti]

Attachment: Third alternative (update)

Comment 18

[Marco Pesenti Gritti]

Comment on attachment 147656 [details] [diff] [review]
Third alternative (update)

Hopefully the last one, sorry for the spam. FWIW this is my favourite.

Comment 19

[Conrad Carlen (not reading bugmail)]

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.

Comment 20

[Marco Pesenti Gritti]

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

Comment 21

[Marco Pesenti Gritti]

Attachment: Fix dangling pointer

Comment 22

[Marco Pesenti Gritti]

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 ?

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

+  const char *lockPath = nsEmbedCString(lockFilePath).get();

this looks like a dangling pointer as well (the nsEmbedCString object will be
destroyed after this line)

Comment 24

[Marco Pesenti Gritti]

Attachment: Really fix all dangling pointers

Comment 25

[Darin Fisher]

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.

Comment 26

[Marco Pesenti Gritti]

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

Comment 27

[Marco Pesenti Gritti]

Attachment: Different approach suggested by darin

Comment 28

[Darin Fisher]

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 29

[Conrad Carlen (not reading bugmail)]

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.

Comment 30

[Marco Pesenti Gritti]

Attachment: add the comment and rename the header

Comment 31

[Marco Pesenti Gritti]

Damn, there is a collision between this and 232691 :(

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

there don'T seem to be any conflicts here - checked in