224305 - separate GRE registration from the installer scripts

Status: RESOLVED FIXED

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

Description

[Benjamin Smedberg]

Currently on win32 the GRE registration process occurs during the XPI install. I
intend to make a separate command-line app that can do GRE
registration/unregistration/finding. This app would be shipped with the GRE, and
would be something like this:

Usage:
grereg (--register-gre | --unregister-gre | --locate-gre | --gre-version)
  global options
    --verbose (print progress information)
    --user (Register/look in user-specific locations only.)
    --global (Register/look in global locations only.)
    --global-or-user (Register in a global location if security options permit,
                      otherwise in a user-specific location this is the
                      default.)
    --gre-version=string (Register/unregister a gre version string other than
                          the builtin default. For --locate-gre this can be set
                          to the special string "all" to find all installed
                          GREs.)

  --register-gre "registers" a GRE path in the registry/config files
  options:
    --gre-path=path (register a gre path other than the executable's directory)

  --unregister-gre "unregisters" a GRE from the registry/config files

  --locate-gre will print the registered path for the GRE to stdout

  --gre-version will print the builtin GRE version for the utility/GRE

This app would be called by the XPI installation process. It would also be very
useful for shipping tarball versions of the GRE or for testing GREs without
having to build an installer.

Comment 1

[Benjamin Smedberg]

blizzard, this will affect you, too, if you're thinking of GRE RPMs.

Comment 2

[Christopher Blizzard (:blizzard)]

That's what I want, yeah.  Of course, we kind of have this right now and it's
called regxpcom. :)

Comment 3

[Benjamin Smedberg]

blizzard: regxpcom does not do "GRE registration" in the sense of installing
files to /etc/gre.d or the win32 registry. I supposed I could integrate all this
functionality into the regxpcom program, although that seems like it would cause
confusion.

--BDS

Comment 4

[Benjamin Smedberg]

Darin, seems like I think about things, file bugs, and then forget. Shall we
resurrect this bug?

Comment 5

[Benjamin Smedberg]

Attachment: XPCOM glue changes, rev. 1

This changes the win32 registry reading a little bit so that you can add any
number of name->directory mappings under Software/Mozilla/GRE/<version>,
instead of just GreHome. It also allows you to search for any version of the
GRE, not just the version that is compiled into the glue. Darin, you're welcome
to comment, but I'm not going to ask for an official review until I can test
this code against the xulrunner launching code that will use it.

And it really needs some help for mac: I'm planning on hardcoding
/Library/Frameworks/XUL.framework/Versions/<version>/XUL and using NSAddImage
to actually get the glue working on mac (10.2+).

Comment 6

[Darin Fisher]

This looks good.  We might want to give some thought to supporting unicode file
paths on Windows, or at least think about how we might want to add that in the
future.

Comment 7

[Benjamin Smedberg]

Attachment: Register/unregister xulrunner in the windows registry using command-line flags, rev. 1

This patch implements registering/unregister a xulrunner as a GRE using
command-line flags. It changes somewhat how GREs are registered: formerly they
were registered at

HKLM|HKCU/Software/mozilla.org/GRE/<version>
  GreHome=<path>

Now they are registered at
HKLM|HKCU/Software/mozilla.org/GRE/<anykey>
  GreHome=<path>
  Version=<version>

however, <anykey> should be <version> if there is not already a GRE registered,
to maintain some compatibility with older glue libs.

This does not implement maxVersion/minVersion yet, I'm going to leave that for
a separate/future patch once mac and linux and done with this step.

Reminder to self: GRE_Locate(maxVersion, minVersion, features[]);

Comment 8

[Darin Fisher]

before uninstalling a key, we should validate that the path corresponds to the
GRE instance that we are uninstalling.  that way we avoid uninstalling some
other GRE that happened to install over our GRE's key.

Comment 9

[Darin Fisher]

Comment on attachment 184624 [details] [diff] [review]
Register/unregister xulrunner in the windows registry using command-line flags, rev. 1


>Index: xpcom/build/dlldeps.cpp

>+#include "nsXPCOMGlue.h"

historically, nsXPCOMGlue.h was only included if you were linking against
the full glue library.	perhaps this new method should be declared in some
other header file.

more comments later...

Comment 10

[Benjamin Smedberg]

Attachment: Register/unregister xulrunner in the windows registry using command-line flags, rev. 2

Adds path-existence checking to the glue, so that we don't select a GRE which
has been deleted. Also adds a check to xulrunner -unregister-global to check
that it is still pointing to this version of the GRE.

I think that GRE_GetGREPathForVersion should be declared in nsXPCOMGlue.h: it
shares many characteristics with the other functions declared in that file, in
that you may call it before XPCOMGlueStartup() and it is an "extension" of
GRE_GetGREPath.

Comment 11

[Darin Fisher]

Comment on attachment 184680 [details] [diff] [review]
Register/unregister xulrunner in the windows registry using command-line flags, rev. 2

>Index: xpcom/glue/nsGREGlue.cpp

>+nsresult
>+GRE_GetGREPathForVersion(const char *aVersion,
>+			 char *aBuffer, PRUint32 aBufLen)

indentation off by one

...
>+    if (!realpath(env, aBuffer))
>+      strncpy(aBuffer, env, aBufLen);
>+#elif XP_WIN32
>+    if (!_fullpath(aBuffer, env, aBufLen))
>+      strncpy(aBuffer, env, aBufLen);
>+#else
>+    // hope for the best
>+    // xxxbsmedberg: other platforms should have a "make absolute" function
>+    strncpy(aBuffer, env, aBufLen);
>+#endif
>+    return NS_OK;

strncpy can result in a buffer that is not null-terminated.  do
you protect against that case at the callsite?	perhaps this code
should stamp a null byte at the end of aBuffer.  or perhaps it
should fail if aBufLen is too small.

perhaps you should use PL_strncpyz.  (there are NSPR dependencies
below, so what's one more?)


>+PRBool
>+GRE_GetPathFromConfigDir(const char* version, const char* dirname,
>+			 char* buffer, PRUint32 buflen)

nit: indentation


>+    snprintf(fullPath, MAXPATHLEN, "%s" XPCOM_FILE_PATH_SEPARATOR "%s",

PR_snprintf?  there's a reason why glib has g_snprintf, right?	some
platforms don't have snprintf perhaps?


>+#ifdef XP_WIN
>+static PRBool
>+CopyWithEnvExpansion(char* aDest, const char* aSource, PRUint32 aBufLen,
>+                     DWORD aType)
>+{
>+  switch (aType) {
>+  case REG_SZ:
>+    strcpy(aDest, aSource);

how do we know that aDest is big enough to hold aSource?
looks like aBufLen tells us how big aDest is.  perhaps
we should use that.


>+    ExpandEnvironmentStrings(aSource, aDest, MAXPATHLEN);

s/MAXPATHLEN/aBufLen/ ?


do we need to handle errors if aDest is not big enough?



>+GRE_GetPathFromRegKey(const char* aVersion, HKEY aRegKey,
>+		      char* aBuffer, PRUint32 aBufLen)

nit: indentation

>+          access(aBuffer, 04) == 0) {

s/04/R_OK/ ?


>Index: xulrunner/app/nsRegisterGRE.h

>+NS_HIDDEN_(int)
>+RegisterXULRunner(PRBool aRegisterGlobally, nsIFile* aLocation);

document return value?


>Index: xulrunner/app/nsRegisterGREWin.cpp

I think you should consider using nsIWindowsRegKey in this code.  It is
designed
to be used prior to XPCOM initialization, and it already solves i18n problems
for
you.  We'll need to touch this code again anyways when we support Unicode file
i/o.
Moreover, I think this code would be simpler if it used nsIWindowsRegKey.  You
could use nsIFile::GetPath, and nsCOMPtr<nsIWindowsRegKey> (i.e., no need to
close
the key explicitly).


>Index: xulrunner/app/nsXULRunnerApp.cpp

>+           "Usage: " XULRUNNER_PROGNAME "[OPTIONS]\n"
>+	   "       " XULRUNNER_PROGNAME "APP-FILE [APP-OPTIONS...]\n"

nit: tabs


>+	   "  --register-global <path>   register this GRE in the machine registry\n"

path?  what is that for?  can't we use the same logic as nsAppRunner.cpp
to deduce the location of the running executable?  XRE_GetExecutablePath?


overall, patch looks good.

Comment 12

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

>+CopyWithEnvExpansion(char* aDest, const char* aSource, PRUint32 aBufLen,
>+    ExpandEnvironmentStrings(aSource, aDest, MAXPATHLEN);

have you considered making this a bit more unicode-aware? darin's new
nsIWindowsRegKey interfaces makes it easy to read unicode strings from the
registry! ;)

maybe the GRE_GetPath* functions should return an nsIFile*?

Comment 13

[Darin Fisher]

Comment on attachment 184680 [details] [diff] [review]
Register/unregister xulrunner in the windows registry using command-line flags, rev. 2

minusing to get attention and to clear my request queue.

Comment 14

[Benjamin Smedberg]

At the present time it doesn't make much sense to use unicode versions of the
functions on the "client" side, because we still have to end up with a
native-charset string to pass to PR_LoadLibraryWithFlags. If we end up with a
unicode version of that function in the future then using the windows unicode
registry functions makes sense. Until then, let's stick with the native charset
at both ends.

Comment 15

[Benjamin Smedberg]

Attachment: Register/unregister xulrunner in the windows registry using command-line flags, rev. 3

Comment 16

[Darin Fisher]

(In reply to comment #14)
> At the present time it doesn't make much sense to use unicode versions of the
> functions on the "client" side, because we still have to end up with a
> native-charset string to pass to PR_LoadLibraryWithFlags. If we end up with a
> unicode version of that function in the future then using the windows unicode
> registry functions makes sense. Until then, let's stick with the native charset
> at both ends.

Why does xulrunner startup need the PR_LoadLibraryWithFlags API?  You're
referring to the standalone xpcom glue, right?

Comment 17

[Benjamin Smedberg]

Yes, I'm referring to the standalone glue as "client", and the xulrunner
registration fu as "server". Maybe I should choose different terms glue/registrar.

Comment 18

[Darin Fisher]

Comment on attachment 185790 [details] [diff] [review]
Register/unregister xulrunner in the windows registry using command-line flags, rev. 3

>Index: xpcom/glue/nsGREGlue.cpp

>+    if (strlen(env) > aBufLen)
>+      return NS_ERROR_FILE_NAME_TOO_LONG;
>+
>+    strcpy(aBuffer, env);

I think you should test >= since you need room for the null byte
that strcpy will write.


>+PRBool
>+GRE_GetPathFromConfigDir(const char* version, const char* dirname,
>+             char* buffer, PRUint32 buflen)

nit: indentation is off.


>+    // Only look for files that end in .conf
>+    char *offset = PL_strrstr(entry->name, ".conf");
>+    if (!offset)
>+      continue;
>+
>+    if (offset != entry->name + strlen(entry->name) - 5)
>+      continue;

nit: maybe nice to do:

  const char kExt[] = ".conf";

and then use "sizeof(kExt)-1" in place of 5.


>+  if((cfg=fopen(filename,"r"))==nsnull) {
>+    return nsnull;
>+  }

nit: whitespace around operators.


>+  while (fgets(buffer, 1024, cfg) != nsnull) {

do we know that buffer size is always greater than 1024?  maybe make
1024 be a defined constant, and check that buffer is always big enough.


>+      if (!strcmp (buffer+1, version)) {

nit: remove whitespace between function and open paran


>+    if (foundHeader && !strncmp (buffer, "GRE_PATH=", 9)) {

nit: ditto


>+    if (strlen(aSource) > aBufLen)
>+      return PR_FALSE;
>+
>+    strcpy(aDest, aSource);

again, i think that ">=" is a better check.


>+    if (ExpandEnvironmentStrings(aSource, aDest, aBufLen) > aBufLen)
>+      return PR_FALSE;


>Index: xulrunner/app/nsRegisterGREWin.cpp

>+  nsCOMPtr<nsILocalFile> localSaved (do_QueryInterface(savedInfoFile));

nit: kill extra space between variable name and open paran


r=darin

Comment 19

[Philip K. Warren]

The checkin for this bug broke the AIX Tinderbox (multiple definitions of
MAXPATHLEN).

Comment 20

[Benjamin Smedberg]

Attachment: Wrap flag handling in argc > 1 check [checked in]

This patch caused a crash when xulrunner is called without any arguments.
Simple fix.

Comment 21

[Darin Fisher]

Comment on attachment 186144 [details] [diff] [review]
Wrap flag handling in argc > 1 check [checked in]

It almost feels like this block should be in a subroutine.  It's pretty long.

Comment 22

[Philip K. Warren]

Attachment: Fix for AIX bustage

Comment 23

[Darin Fisher]

Comment on attachment 186167 [details] [diff] [review]
Fix for AIX bustage

Fine by me.

Comment 24

[Benjamin Smedberg]

Comment on attachment 186144 [details] [diff] [review]
Wrap flag handling in argc > 1 check [checked in]

It's a long block, but it's fairly straightforward: if we add more stuff later
I'll move it to a separate "HandleFlags" or somesuch.

Comment 25

[Benjamin Smedberg]

Attachment: Switch to correct result code in fallback case

I'm not sure whether I just brain-farted when I wrote this originally, so I'm
asking for review to make sure.

Comment 26

[tqh]

Attachment 185790 [details] [diff] broke BeOS-builds:

/mozdev/mozilla/toolkit/xre/nsAppRunner.cpp: In function `nsresult
XRE_GetBinaryPath(const char *, nsILocalFile **)':
/mozdev/mozilla/toolkit/xre/nsAppRunner.cpp:1120: passing `nsILocalFile **' as
argument 1 of `PL_strncpy(char *, const char *, unsigned int)'

Comment 27

[tqh]

Attachment: Fixes BeOS bustage.

Just removing the PL_strncpy line should be ok I think.

Comment 28

[Benjamin Smedberg]

Attachment: Register/unregixter xulrunner in the unix, rev. 1

This is very similar to the windows code (no registry, of course, but it keeps
the same global.reginfo/user.reginfo model and inserts/removes files in
~/.gre.d/ or /etc/gre.d

Comment 29

[Darin Fisher]

Comment on attachment 186350 [details] [diff] [review]
Register/unregixter xulrunner in the unix, rev. 1

>Index: xpcom/glue/nsGREGlue.cpp

>     sprintf(buffer, "%s" XPCOM_FILE_PATH_SEPARATOR GRE_CONF_NAME, env);
>+    sprintf(buffer, "%s" XPCOM_FILE_PATH_SEPARATOR GRE_USER_CONF_DIR, env);

while you're here, please change this code to use snprintf or
PR_snprintf.


>+      if (buffer[versionlen + 1] != ']' ||
>+          strncmp(buffer + 1, version, versionlen) != 0) {
>+        foundHeader = PR_FALSE;
>+      } else {
>         foundHeader = PR_TRUE;
>       }

maybe write this as:

	foundHeader = (some expression);


>Index: xulrunner/app/nsRegisterGREUnix.cpp

>+static PRBool
>+MakeConfFile(const char *regfile, const nsCAutoString &greHome)

nit: in general, there is no benefit to passing a param as nsCAutoString.
pass as nsCString instead.  you don't avoid any virtual overhead by 
passing nsCAutoString, so might as well make the code more generic.


>+  struct stat dummy;
>+  if (stat(regfile, &dummy) == 0)
>+    return PR_FALSE;

Use access system call instead?


>+  PRFileDesc* fd = nsnull;
>+  rv = localSaved->OpenNSPRFileDesc(PR_CREATE_FILE | PR_RDWR, 0664, &fd);
...
>+  if (r < 0) {
>+    irv = PR_FALSE;
>+    goto reg_end;
>+  }

How about writing a simple AutoPRFileDesc class to use here
instead of the goto statements?


>+    struct stat dummy;
>+
>+    // There was already a .reginfo file, let's see if we are already
>+    // registered.
>+    if (stat(regfile, &dummy) == 0) {

Again, why not use "access" ?


>+  for (int i = 0; i < 1000; ++i) {

Create a #define for this magic number and document it?

Comment 30

[Benjamin Smedberg]

Attachment: Register/unregixter xulrunner in the unix, rev. 1.1 [checked in]

Nits fixed.

Comment 31

[Darin Fisher]

Comment on attachment 186391 [details] [diff] [review]
Register/unregixter xulrunner in the unix, rev. 1.1 [checked in]

>Index: xpcom/glue/nsGREGlue.cpp

>+    snprintf(buffer, MAXPATHLEN,
>+             "%s" XPCOM_FILE_PATH_SEPARATOR GRE_CONF_NAME, env);

nit: s/MAXPATHLEN/sizeof(buffer)/

same nit applies elsewhere


>+    if (foundHeader && !strncmp (buffer, kGreHome, sizeof(kGreHome) - 1)) {

nit: kill space between function name and opening paran

Comment 32

[Benjamin Smedberg]

Comment on attachment 186391 [details] [diff] [review]
Register/unregixter xulrunner in the unix, rev. 1.1 [checked in]

Unix checked in for 1.8b3

Comment 33

[Benjamin Smedberg]

I'm going to mark this FIXED and leave the (hard!) mac bits for bug 297923.

Comment 34

[tqh]

Sorry to reopen it, but the BeOS bustage patch needs to be checked in.

Comment 35

[Benjamin Smedberg]

File new bugs for non-tier1 platforms please.

Comment 36

[tqh]

The point was that you had gotten it reviewed and approved already and it just
needed commit. Oh well, why do it the easy way...

Comment 37

[Mike Kaply [:mkaply]]

BeOS build bustage fix checked in.

Comment 38

[tqh]

*** Bug 299985 has been marked as a duplicate of this bug. ***