299992 - Allow glue-finding code to specify additional features such as toolkit=gtk2 and xulrunner=true

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect, P1)

Description

[Benjamin Smedberg]

We are probably going to have seamonkey and xulrunner 1.8 GREs out in the wild
:-(, and there are situations in which we may have GTK2 and QT xulrunners
installed on the same machine :-(. Finally, it should be possible to look for a
range of GRE versions, not just one particular version (this unfortunately
requires GRE IDs to be something other than opaque strings, something I haven't
figured out yet).

We need a function like GRE_FindGREPathForVersion that will find a GRE based on
these multiple characteristics.

Comment 1

[Benjamin Smedberg]

Attachment: Part JS, rev. 1 - Alter jsdhash and the plify script to use NS_COM_GLUE and avoid NSPR symbols

Comment 2

[Benjamin Smedberg]

Attachment: Part XPCOMGlue, rev. 1 - Move pldhash and the templatized hashtables to the glue

Comment 3

[Benjamin Smedberg]

Attachment: Part XPCOMGlue (cvsmoves)

Comment 4

[Darin Fisher]

Comment on attachment 191385 [details] [diff] [review]
Part XPCOMGlue, rev. 1 - Move pldhash and the templatized hashtables to the glue

>Index: xpcom/ds/nsHashKeys.h

>+#ifdef MOZILLA_INTERNAL_API
>+#include "nsAString.h"
>+#include "nsString.h"
>+#else
>+#include "nsStringAPI.h"
>+#endif

Since we are trying to present nearly the same API for the
frozen strings as the internal strings, it might be nice to
create a header file that code can #include to get the 
proper string headers.


>Index: xpcom/ds/nsTHashtable.cpp

>+NS_COM PRUint32 HashString( const nsAString& aStr )
>+{
>+  PRUint32 code = 0;
>+
>+#ifdef MOZILLA_INTERNAL_API
>+  nsAString::const_iterator begin, end;
>+  aStr.BeginReading(begin);
>+  aStr.EndReading(end);
>+#else
>+  const PRUnichar *begin, *end;
>+  PRUint32 len = NS_StringGetData(aStr, &begin);
>+  end = begin + len;
>+#endif

Doesn't nsStringAPI.h define BeginReading/EndReading?
Why not use those here to avoid #ifdef?


>Index: xpcom/string/public/nsStringAPI.h

>+  NS_HIDDEN_(PRBool) Equals( const self_type &other ) const {
>+    const char_type *cself;
>+    const char_type *cother;
>+    PRUint32 selflen = NS_StringGetData(*this, &cself);
>+    PRUint32 otherlen = NS_StringGetData(other, &cother);
>+
>+    if (selflen != otherlen)
>+      return PR_FALSE;
>+
>+    for (const char_type *end = cself + selflen;
>+         cself < end;
>+         ++cself, ++cother) {
>+      if (*cself != *cother)
>+        return PR_FALSE;
>+    }
>+
>+    return PR_TRUE;
>+  }

Ugh.  I think this should be non-inline, and implemented in the
glue library.  Also, why not use |memcmp|?  I think you should
be able to use that function in both implementations.


Otherwise, this patch looks good to me.  r=darin

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 191380 [details] [diff] [review]
Part JS, rev. 1 - Alter jsdhash and the plify script to use NS_COM_GLUE and avoid NSPR symbols

Looks good, assuming sed works and the xpcom/ds output files diff well (against
their previous revs, and null diff against the js ones when sed'ed).  r=me.

/be

Comment 6

[Benjamin Smedberg]

Comment on attachment 191385 [details] [diff] [review]
Part XPCOMGlue, rev. 1 - Move pldhash and the templatized hashtables to the glue

This should be low risk, it's just changing a little of the linking strategy
and is almost necessary to fix the xulrunner startup process.

Comment 7

[Benjamin Smedberg]

Oh, and I did things the way I did for a reason:

1) The stringapi versions of beginreading/endreading would end up calling
NS_GetStringData twice, and this is a critical codepath.

2) the stringapi header currently does not require that you link against any
glue. If we want to require this, we have to think through which versions of the
glue this function gets defined in (we probably do not want it in the
"xpcom_core.dll" glue, only the xpcomglue_s.lib and xpcomglue.lib versions... we
currently don't have the link-magic to make this happen.

Comment 8

[Benjamin Smedberg]

Attachment: Consolidate several of our various iniparsers into the XPCOM glue, rev. 1

This consolidates several of our existing variations on nsINIParser into the
xpcom glue... nsGREGlue.cpp will use this implementation instead of the
custom-hacked INI parser as the last patch here. I'm still building various
pieces of this, so I may attach minor build-fixes if there are problems, but I
think this should be good enough for review.

Comment 9

[Darin Fisher]

Comment on attachment 191720 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 1

>Index: xpcom/glue/nsINIParser.cpp

>+    rv = aFile->OpenANSIFileDesc("r", &fd);
...
>+    if (!fb) {
>+        rv = NS_ERROR_OUT_OF_MEMORY;
>+        goto bail;
>+    }
...
>+bail:
>+    if (fd)
>+        fclose(fd);
>+
>+    free(fb);

In the updater code, I created a AutoFILE class to avoid messy
goto statements.  You could do the same here, and use operator
new[] and delete[] with nsAutoArrayPtr.  Then, the code would
be a lot cleaner.


>+// copied from toolkit/mozapps/updater/src/updater/updater.cpp
>+// we could use nsCRT::strtok except that nsCRT isn't part of the glue,
>+// and may never be due to NSPR dependencies

Maybe we should add this function to the glue at some point.
I think we basically want to add a set of string helper functions
to the glue.  These would include nice functions for working
with nsAString/nsACString, but it might also include stuff like
this for working with raw char buffers.


>Index: toolkit/profile/src/nsToolkitProfileService.cpp

>+    rv = parser.GetString(NS_LITERAL_CSTRING("General"),
>+                          NS_LITERAL_CSTRING("StartWithLastProfile"),

I wonder.. is it really better for the first two
parameters to be nsACString here?  The result should
definitely be returned via nsACString, but i'm not so
sure about the input.  Maybe consistency is best :-/
RBool aValue)


>+      rv = parser.GetString(nsDependentCString(lastSectionName),
>+                                     nsDependentCString(transform->keyName),
>+                                     val);

fix indentation (this is in the opera profile migrator)


>+    nsCAutoSTring proxyServer;

compile error.


>+    rv = aParser.GetStringAlloc(NS_LITERAL_CSTRING("Proxy"),
>+                                 nsDependentCString(serverBuf),
>+                                 proxyServer);

indentation... what is GetStringAlloc?


>-#if defined(XP_WIN) || (defined(XP_UNIX) && !defined(XP_MACOSX))
>+// #if defined(XP_WIN) || (defined(XP_UNIX) && !defined(XP_MACOSX))
>+#if 1

why add the "#if 1" business?

Comment 10

[Benjamin Smedberg]

Attachment: Consolidate several of our various iniparsers into the XPCOM glue, rev. 1.1

This one actually builds, and I've added the auto-closing classes with early
returns. The #if changes were just local changes to test-build that code since
this happens to be a mac tree.

I thought about keeping the signature "const char*, const char*, nsACString&
aResult", but I intend to write an XPCOM interface wrapper around this code
which I would prefer used ACString instead of string. The
NS_LITERAL_CSTRING/dependentstring wrappers around the current code aren't that
big a deal.

Comment 11

[Chase Phillips]

Comment on attachment 191380 [details] [diff] [review]
Part JS, rev. 1 - Alter jsdhash and the plify script to use NS_COM_GLUE and avoid NSPR symbols

a=[e-mail]

Comment 12

[Chase Phillips]

Comment on attachment 191385 [details] [diff] [review]
Part XPCOMGlue, rev. 1 - Move pldhash and the templatized hashtables to the glue

a=[e-mail]

Comment 13

[Benjamin Smedberg]

The cvsmoves should be performed here before 1.8 branches, as they will be much
more difficult to manage afterwards.

Comment 14

[Benjamin Smedberg]

Comment on attachment 191768 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 1.1

I need another "init" method which I can use from the standalone glue.

Comment 15

[Benjamin Smedberg]

In fact, this can't use ACStrings for my purposes because you can't use
ACStrings until the XPCOM glue is started. Back to the char* drawing board.

Comment 16

[Benjamin Smedberg]

Attachment: Consolidate several of our various iniparsers into the XPCOM glue, rev. 2

This uses const char* instead of nsACString to satisfy the glue-init
chicken-and-egg problem.

Comment 17

[Darin Fisher]

Comment on attachment 192007 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 2

Do we need to worry about InitFromFILE being called twice on the same
nsINIParser instance?  What if memory allocation (of mFileContents) succeeds,
but the subsequent call to fseek fails?  Then, imagine InitFromFILE being
called again.  Won't we leak mFileContents in the process?  Do we need to worry
about mFileContents being non-nullterminated after an fread error?

aResult[aResultLen] writes off the end of the buffer no?  or should aResultLen
be one less than the actual length of aResult?	I think you should use
aResult[aResultLen-1] instead.

I think you should also just compute the length of val->value before trying to
copy it into aResult.  If its length is greater than aResultLen-1, then just
don't bother copying anything and return NS_ERROR_LOSS_OF...

Comment 18

[Darin Fisher]

Comment on attachment 192007 [details] [diff] [review]
Consolidate several of our various iniparsers into the XPCOM glue, rev. 2

r=darin w/ fix for aResultLen-1 and documentation for the other things.

Comment 19

[Benjamin Smedberg]

Attachment: Consolidate several of our various iniparsers into the XPCOM glue, rev. 2.1

Comments and array access updated.

Comment 20

[Dave Miller [:justdave]]

someone pointed out to me that there were cvs moves on this bug...  are they
still waiting to happen? and if so, which attachments have the scripts in
question? (there appears to be more than one).  Assign the bug to me when you're
ready for it to happen.

Comment 21

[Benjamin Smedberg]

justdave: https://intranet.mozilla.org/SCM:20050810-bug299992

Comment 22

[Chase Phillips]

CVS copy completed 12:45 Mountain View local time.

Comment 23

[Chase Phillips]

For future reference cc'ing me on this bug would have let me know much sooner
that a CVS move needed to happen since I only found out it was needed two days
ago via a concall.  If I missed a previous ping on IRC about this then I apologize.

Comment 24

[Benjamin Smedberg]

glue and iniparser patches landed... this no longer blocks branching

Comment 25

[Benjamin Smedberg]

backed out due to various OS bustages... still does not blokck the branch

Comment 26

[Benjamin Smedberg]

Attachment: Remove unneeded -D__STDC__ from intl makefiles, rev. 1

the intl makefiles contain odd windows-specific -D__STDC__ which are leftover
from ftang's original landing in 1999. I can't see what they're there for, and
they broke the nsHashKeys usage of strdup. Removing them.

Comment 27

[Benjamin Smedberg]

Attachment: Open in binary mode, and ignore semicolon comments

Comment 28

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 192764 [details] [diff] [review]
Open in binary mode, and ignore semicolon comments

r=shaver

Comment 29

[Benjamin Smedberg]

I have landed the JS, xpcomglue, iniparser, intl makefiles, and binary mode patches.

Comment 30

[Samuel Sidler (old account; do not CC)]

Checkin's broke Camino's tinderboxen. Kreeger's working on a patch to fix this
(ObjC treats "id" special, needs to be changed in nsHashKeys.h).

CCing kreeger.

Comment 31

[Nick Kreeger]

Attachment: Fixes bustage on Camino tboxes

Gets rid of the use of the keyword "id", fixes bustage in the Camino tboxes

Comment 32

[Samuel Sidler (old account; do not CC)]

(In reply to comment #31)
> Created an attachment (id=192792) [edit]
> Fixes bustage on Camino tboxes
> 
> Gets rid of the use of the keyword "id", fixes bustage in the Camino tboxes

Since apparently the original patches haven't been checked into the 1.8 branch
yet, bsmedberg can just update them instead of applying this patch. In addition,
pink checked in the fix for the trunk and tboxen are no longer red.

Comment 33

[[e-mail]]

mingw gcc doesn't like the NS_COM declarations in nsHashKeys.h so I'm trying
s/\<NS_COM\>/NS_COM_GLUE/ but I don't know if it's the right fix.

Comment 34

[[e-mail]]

Attachment: Fix mingw bustage

Comment 35

[Benjamin Smedberg]

Comment on attachment 193302 [details] [diff] [review]
Fix mingw bustage

I'm not sure why these are NS_COM *or* NS_COM_GLUE... shouldn't they be inlined
directly? What happens if you remove the NS_COM altogether?

Comment 36

[Benjamin Smedberg]

Attachment: Search for GREs with version ranges and properties, rev. 1

I got darin to do an API review before he left... just needs a code review.

Comment 37

[Benjamin Smedberg]

Comment on attachment 193846 [details] [diff] [review]
Search for GREs with version ranges and properties, rev. 1

>Index: xpcom/glue/nsGREGlue.cpp

>+GRE_GetPathFromRegKey(HKEY aRegKey,

>     if (::RegQueryValueEx(subKey, "Version", NULL, NULL,
>                           (BYTE*) version, &versionlen) == ERROR_SUCCESS &&
>-        strncmp(aVersion, version, versionlen) == 0) {
>-      if (::RegQueryValueEx(subKey, "GreHome", NULL, &pathtype,
>-                            (BYTE*) pathbuf, &pathlen) == ERROR_SUCCESS &&
>-          *pathbuf &&
>-          CopyWithEnvExpansion(aBuffer, pathbuf, aBufLen, pathtype) &&
>-          access(aBuffer, R_OK) == 0) {
>-        RegCloseKey(subKey);
>-        return PR_TRUE;
>+        CheckVersion(version, versions, versionsLength) == 0) {

The == 0 is wrong here, since CheckVersion returns PRBool... using the result
directly is what I wanted.

--BDS

Comment 38

[Darin Fisher]

Comment on attachment 193846 [details] [diff] [review]
Search for GREs with version ranges and properties, rev. 1

>Index: xpcom/glue/nsGREGlue.cpp

>+#include "nsXPCOMPrivate.h"
>+
>+
> #include <stdio.h>

nit: bogus extra newline ?


>+GRE_FindGREFramework(const char* rootPath,
>+                     const GREVersionRange *versions,
...
>+  DIR *dir = opendir(buffer);
>+  if (dir) {
>+    struct dirent *entry;
>+    while (!found && (entry = readdir(dir))) {
>+      if (CheckVersion(entry->d_name, versions, versionsLength)) {
>+        snprintf(buffer, buflen,
>+                 "%s/Library/Frameworks/" GRE_FRAMEWORK_NAME
>+                 "/Versions/%s/" XPCOM_DLL, rootPath, entry->d_name);
>+        if (access(buffer, R_OK | X_OK) == 0)
>+          found = PR_TRUE;

I'm curious... so, it seems like we just return the first
GRE that satisfies the version requirements.  We don't 
necessarily find the most recent GRE, which would seem
like a desirable behavior to me.  This would probably
require sorting the list of directory names, and I can
imagine that being a bit of extra code.  You should at
least comment somewhere (in the API docs) that the GRE
is chosen in no particular order.


>Index: xpcom/glue/standalone/nsGREDirServiceProvider.cpp

>+  static const GREVersionRange version[] = {
>+    GRE_BUILD_ID, PR_TRUE,
>+    GRE_BUILD_ID, PR_TRUE
>+  };

The array notation seems a bit screwy here.  I know that GCC and MSVC
are happy with this, and perhaps it is part of the C++ standard, but
I would expect to see one of the following forms instead:

  static const GREVersionRange version = {
    GRE_BUILD_ID, PR_TRUE,
    GRE_BUILD_ID, PR_TRUE
  };

  static const GREVersionRange version[] = {
    { GRE_BUILD_ID, PR_TRUE,
      GRE_BUILD_ID, PR_TRUE }
  };

I think either of these would be more clear to the reader.


>Index: xpcom/glue/standalone/nsXPCOMGlue.h

>+ * @param properties       A null-terminated list of GRE property/value pairs
>+ *                         which must all be satisfied.

How does one learn about the set of properties that may be passed
in this parameter?  Maybe we should have some #define's in this
file for the property names (and values).


>Index: xulrunner/stub/nsXULStubOSX.cpp

>+  char maxVersion[VERSION_MAXLEN] = "2";
...
>+  rv = parser.GetString("Gecko", "MaxVersion", maxVersion, sizeof(maxVersion));
>+  if (NS_SUCCEEDED(rv))
>+    range.upperInclusive = PR_TRUE;

So, if the XUL app does not specify a max version, then you're
going to restrict it to Gecko 2 or earlier?  Some comments would
be good to explain this so that we update this properly when we
ever start shipping a Gecko 2.x.


r=darin

Comment 39

[Benjamin Smedberg]

> necessarily find the most recent GRE, which would seem
> like a desirable behavior to me.  This would probably

Filed bug 306688 on this (1.9 fodder).

> How does one learn about the set of properties that may be passed
> in this parameter?  Maybe we should have some #define's in this

I'll do this with the xulrunner stub (bug 306689).


> So, if the XUL app does not specify a max version, then you're
> going to restrict it to Gecko 2 or earlier?  Some comments would
> be good to explain this so that we update this properly when we
> ever start shipping a Gecko 2.x.

I don't think we should update it; I'll add a basic explanatory comment. If you
don't specify a maxversion I'm going to assume that you mean that your app only
uses frozen APIs, and our frozen APIs are only guaranteed for the toolkit 1.x
lifetime.

Comment 40

[Benjamin Smedberg]

Fixed on trunk.

Comment 41

[Asa Dotzler [:asa]]

ben, can you tell us a bit more about why we need this and what is the risk?

Comment 42

[Benjamin Smedberg]

This is needed to make xulrunner embedding functional on linux in the presence
of mixed XULRunner and seamonkey GREs. It should have no impact on the aviary
apps, which do not use GRE lookups.

Comment 43

[Benjamin Smedberg]

I introduced an API inconsistency here: sometimes this function returns the path
with the DLLname, and sometimes not.

Comment 44

[Benjamin Smedberg]

Attachment: Always return the XPCOM DLL path, rev. 1

Comment 45

[Darin Fisher]

Comment on attachment 195106 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1

>Index: xpcom/glue/nsGREGlue.cpp

>+  strncat(c->pathBuffer, "/" XPCOM_DLL, c->buflen - strlen(c->pathBuffer));
>+  if (access(c->pathBuffer, R_OK))
>+    return PR_TRUE;

Hmm... |strncat| does not promise to null terminate its first argument.  I
think you should be careful not to pass garbage to |access|.


>+        strncat(aBuffer, "\\" XPCOM_DLL, aBufLen - strlen(aBuffer));
>+        if (access(aBuffer, R_OK)) {

ditto


How about writing a subroutine that appends a string to a buffer?

Comment 46

[Benjamin Smedberg]

Attachment: Always return the XPCOM DLL path, rev. 1.1

Comment 47

[Darin Fisher]

Comment on attachment 196150 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1.1

>Index: xpcom/glue/nsGREGlue.cpp

>+    ++dest, ++append;

cute


>+  if (*append)
>+    return PR_FALSE;
>+
>+  return PR_TRUE;

    return *append == '\0';


>+      else {
>+        if (!safe_strncat(aBuffer, "\\" XPCOM_DLL, aBufLen) ||
>+            access(aBuffer, R_OK)) {
>+          ok = PR_FALSE;
>+        }
>+      }

looks like you could reduce one level of indenting here by making
it an "else if" instead.


r=darin

Comment 48

[Benjamin Smedberg]

Attachment: Always return the XPCOM DLL path, rev. 1.2

Final DLL-fix patch as checked in on trunk.

Comment 49

[Benjamin Smedberg]

Fixed on trunk, this needs to make branch also.

Comment 50

[[e-mail]]

Attachment: Alternate mingw bustage fix (checked in trunk and branch)

Comment 51

[Asa Dotzler [:asa]]

BSmedberg, can you tell us more about the risk here, especially any risk to
Firefox or Thunderbird?

Comment 52

[Stephen Donner [:stephend] Not actively reading bugmail]

I think this caused bug 308838.

Comment 53

[Asa Dotzler [:asa]]

Comment on attachment 196204 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1.2

please re-request approvals when all regressions have been resolved.

Comment 54

[Darin Fisher]

Comment on attachment 196216 [details] [diff] [review]
Alternate mingw bustage fix (checked in trunk and branch)

r=darin

Comment 55

[[e-mail]]

CCing myself to watch for approval on attachment 196216 [details] [diff] [review] (checked in) :-/

Comment 56

[Benjamin Smedberg]

Don't know why this got minused, but it's essential for embedding.

Comment 57

[Benjamin Smedberg]

Comment on attachment 196204 [details] [diff] [review]
Always return the XPCOM DLL path, rev. 1.2

regression in bug 308838 needs to land with this (neither will affect toolkit
apps)

Comment 58

[Benjamin Smedberg]

All fixed, with bug 308838.

Comment 59

[Brian Ryner (not reading)]

The files under mozilla/xulrunner were missed when this was landed on the 1.8
branch.  I just checked them in.

Comment 60

[Iain MacDonnell]

Hmmpphhh .. fixes from here seem to break compile with Sun Studio:

"/export/stuff/mozilla/ff15b2/mozilla/xpcom/glue/nsGREGlue.cpp", line 472:
Error: A class with a reference member must have a user-defined constructor.
"/export/stuff/mozilla/ff15b2/mozilla/xpcom/glue/nsGREGlue.cpp", line 523:
Error: Expected an expression.
"/export/stuff/mozilla/ff15b2/mozilla/xpcom/glue/nsGREGlue.cpp", line 529:
Error: Use ";" to terminate statements.3 Error(s) detected.
gmake[4]: *** [nsGREGlue.o] Error 3