355077 - xulrunner-stub does not work on OS/2

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Andy Willis (abwillis)]

Xulrunner-stub.exe does not work on OS/2.  There are actually several issues going on.  I have a partial patch that I will be attaching but it is not yet functional.  In order to work at all, currently gre_home has to be set to the xulrunner location.  This may be something we will want to change by adding an ini or registry entry.

Comment 1

[Andy Willis (abwillis)]

Attachment: partial patch

This gets to the point where     retval = XRE_main(argc, argv, appData); is called and dies somewhere after that that I have not tracked down fully.
The debugger though makes me think that the problem may stem though from:
http://lxr.mozilla.org/seamonkey/source/xulrunner/stub/nsXULStub.cpp#312
http://lxr.mozilla.org/seamonkey/source/xulrunner/stub/nsXULStub.cpp#321
as there are messages about incomplete structure but it proceeds trhough there with no errors.

Comment 2

[Andy Willis (abwillis)]

Comment on attachment 240868 [details] [diff] [review]
partial patch

OK, in the newsgroups it was reported that this patch works for them.  My tree has some high-mem changes that were affecting it.  I first removed OBJ_ANY to no affect.  After a report in the high mem bug I readded the OBJ_ANY and removed -Zhigh-mem and it does indeed now work.
In the newsgroup it was reported that the xulrunner bin directory had to be under the application directory but I do not see that here. 
set gre_home=path-to-xulrunner-bin-directory
and it now works.
I am going to request review on this patch and persue any high-mem issues in the high-mem bug.

Comment 3

[Andy Willis (abwillis)]

Comment on attachment 240868 [details] [diff] [review]
partial patch

>Index: xpcom/glue/nsGREGlue.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/glue/nsGREGlue.cpp,v
>retrieving revision 1.20
>diff -u -1 -0 -r1.20 nsGREGlue.cpp
>--- xpcom/glue/nsGREGlue.cpp	27 Sep 2006 16:07:06 -0000	1.20
>+++ xpcom/glue/nsGREGlue.cpp	1 Oct 2006 23:32:56 -0000
>@@ -159,20 +159,25 @@
>     char p[MAXPATHLEN];
>     snprintf(p, sizeof(p), "%s" XPCOM_FILE_PATH_SEPARATOR XPCOM_DLL, env);
>     p[sizeof(p) - 1] = '\0';
> 
> #if XP_UNIX
>     if (realpath(p, aBuffer))
>       return NS_OK;
> #elif XP_WIN
>     if (_fullpath(aBuffer, p, aBufLen))
>       return NS_OK;
>+#elif XP_OS2
>+     // realpath on OS/2 returns a unix-ized path, so re-native-ize
>+    if (realpath(p, aBuffer))
>+     for (char* ptr = strchr(aBuffer, '/'); ptr; ptr = strchr(ptr, '/')) *ptr = '\\';
>+      return NS_OK;
> #elif XP_BEOS
>     BPath path;
>     status_t result;
>     result = path.SetTo(p,0,true);
>     if (result == B_OK)
>     {
>       sprintf(aBuffer, path.Path());
>       return NS_OK;
>     }
> #else
>Index: xpcom/glue/standalone/nsGlueLinkingOS2.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/glue/standalone/nsGlueLinkingOS2.cpp,v
>retrieving revision 1.5
>diff -u -1 -0 -r1.5 nsGlueLinkingOS2.cpp
>--- xpcom/glue/standalone/nsGlueLinkingOS2.cpp	1 Feb 2006 16:51:12 -0000	1.5
>+++ xpcom/glue/standalone/nsGlueLinkingOS2.cpp	1 Oct 2006 23:32:56 -0000
>@@ -128,21 +128,21 @@
> 
>     ulrc = DosLoadModule(pszError, _MAX_PATH, xpcomFile, &h);
> 
>     if (ulrc != NO_ERROR)
>         return nsnull;
> 
>     AppendDependentLib(h);
> 
>     GetFrozenFunctionsFunc sym;
> 
>-    ulrc = DosQueryProcAddr(h, 0, "NS_GetFrozenFunctions", (PFN*)&sym);
>+    ulrc = DosQueryProcAddr(h, 0, "_NS_GetFrozenFunctions", (PFN*)&sym);
> 
>     if (ulrc != NO_ERROR)
>         XPCOMGlueUnload();
> 
>     return sym;
> }
> 
> void
> XPCOMGlueUnload()
> {
>@@ -164,20 +164,23 @@
> nsresult
> XPCOMGlueLoadXULFunctions(const nsDynamicFunctionLoad *symbols)
> {
>     ULONG ulrc = NO_ERROR;
> 
>     if (!sXULLibrary)
>         return NS_ERROR_NOT_INITIALIZED;
> 
>     nsresult rv = NS_OK;
>     while (symbols->functionName) {
>-        ulrc = DosQueryProcAddr(sXULLibrary, 0, symbols->functionName, (PFN*)symbols->function);
>+        char buffer[512];
>+        snprintf(buffer, sizeof(buffer),
>+                 "_" "%s", symbols->functionName);
>+        ulrc = DosQueryProcAddr(sXULLibrary, 0, buffer, (PFN*)symbols->function);
> 
>         if (ulrc != NO_ERROR)
>             rv = NS_ERROR_LOSS_OF_SIGNIFICANT_DATA;
> 
>         ++symbols;
>     }
> 
>     return rv;
> }
>Index: xulrunner/stub/nsXULStub.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xulrunner/stub/nsXULStub.cpp,v
>retrieving revision 1.7
>diff -u -1 -0 -r1.7 nsXULStub.cpp
>--- xulrunner/stub/nsXULStub.cpp	25 Aug 2006 20:33:34 -0000	1.7
>+++ xulrunner/stub/nsXULStub.cpp	1 Oct 2006 23:33:01 -0000
>@@ -48,20 +48,29 @@
> #ifdef XP_WIN
> #include <windows.h>
> #include <io.h>
> #define snprintf _snprintf
> #define PATH_SEPARATOR_CHAR '\\'
> #include "nsWindowsRestart.cpp"
> #define R_OK 04
> #elif defined(XP_MACOSX)
> #include <CFBundle.h>
> #define PATH_SEPARATOR_CHAR '/'
>+#elif defined (XP_OS2)
>+#define INCL_DOS
>+#define INCL_DOSMISC
>+#define INCL_DOSERRORS
>+#include <os2.h>
>+#include <unistd.h>
>+#include <sys/types.h>
>+#include <sys/stat.h>
>+#define PATH_SEPARATOR_CHAR '\\'
> #else
> #include <unistd.h>
> #include <sys/types.h>
> #include <sys/stat.h>
> #define PATH_SEPARATOR_CHAR '/'
> #endif
> 
> #define VERSION_MAXLEN 128
> 
> static void Output(PRBool isError, const char *fmt, ... )
>@@ -154,20 +163,27 @@
>   CFStringGetCString(iniPathStr, iniPath, sizeof(iniPath),
>                      kCFStringEncodingUTF8);
>   CFRelease(iniPathStr);
> 
> #else
> 
> #ifdef XP_WIN
>   if (!::GetModuleFileName(NULL, iniPath, sizeof(iniPath)))
>     return 1;
> 
>+#elif defined(XP_OS2)
>+   PPIB ppib;
>+   PTIB ptib;
>+ 
>+   DosGetInfoBlocks( &ptib, &ppib);
>+   DosQueryModuleName( ppib->pib_hmte, sizeof(iniPath), iniPath);;
>+
> #else
>   // on unix, there is no official way to get the path of the current binary.
>   // instead of using the MOZILLA_FIVE_HOME hack, which doesn't scale to
>   // multiple applications, we will try a series of techniques:
>   //
>   // 1) use realpath() on argv[0], which works unless we're loaded from the
>   //    PATH
>   // 2) manually walk through the PATH and look for ourself
>   // 3) give up
> 
>@@ -258,20 +274,28 @@
>     if (NS_FAILED(rv)) {
>       // XXXbsmedberg: Do something much smarter here: notify the
>       // user/offer to download/?
> 
>       Output(PR_FALSE,
>              "Could not find compatible GRE between version %s and %s.\n", 
>              range.lower, range.upper);
>       return 1;
>     }
>   }
>+#ifdef XP_OS2
>+  sprintf(tmpPath,greDir);
>+  lastSlash = strrchr(tmpPath, PATH_SEPARATOR_CHAR);
>+  *(lastSlash) = '\0';
>+  // DosSetExtLIBPATH("T", LIBPATHSTRICT);
>+  DosSetExtLIBPATH(tmpPath, BEGIN_LIBPATH);
>+  // DosSetCurrentDir(tmpPath);
>+#endif
> 
>   rv = XPCOMGlueStartup(greDir);
>   if (NS_FAILED(rv)) {
>     Output(PR_TRUE, "Couldn't load XPCOM.\n");
>     return 1;
>   }
> 
>   static const nsDynamicFunctionLoad kXULFuncs[] = {
>     { "XRE_CreateAppData", (NSFuncPtr*) &XRE_CreateAppData },
>     { "XRE_FreeAppData", (NSFuncPtr*) &XRE_FreeAppData },

Comment 4

[Andy Willis (abwillis)]

Sorry that didn't do what I was expecting/wanting.  I wanted to change one thing in the patch to comment out the libpathstrict stuff I had for testing.

Comment 5

[Andy Willis (abwillis)]

Attachment: remove libpathstrict

This is what I had wanted to do previously.  The libpathstrict was put in for testing to make sure that the running moz was not causing an issue.

Comment 6

[Mike Kaply [:mkaply]]

Comment on attachment 241778 [details] [diff] [review]
remove libpathstrict

r=mkaply

Comment 7

[Andy Willis (abwillis)]

Adding Peter to cc, asking that he check this in.

Comment 8

[Peter Weilbacher]

Comment on attachment 241778 [details] [diff] [review]
remove libpathstrict

I guess I could have looked at this some time ago. Didn't even try it out yet... Let me ask a few questions before I check in, because if I check something in I want to make sure that it's OK.

>--- xpcom/glue/nsGREGlue.cpp	27 Sep 2006 16:07:06 -0000	1.20
>+++ xpcom/glue/nsGREGlue.cpp	1 Oct 2006 23:32:56 -0000
[...]
> #elif XP_WIN
>     if (_fullpath(aBuffer, p, aBufLen))
>       return NS_OK;
>+#elif XP_OS2
>+     // realpath on OS/2 returns a unix-ized path, so re-native-ize
>+    if (realpath(p, aBuffer))
>+     for (char* ptr = strchr(aBuffer, '/'); ptr; ptr = strchr(ptr, '/')) *ptr = '\\';
>+      return NS_OK;

The spacing here looks weird, I could rectify that on checkin.


>--- xpcom/glue/standalone/nsGlueLinkingOS2.cpp	1 Feb 2006 16:51:12 -0000	1.5
>+++ xpcom/glue/standalone/nsGlueLinkingOS2.cpp	1 Oct 2006 23:32:56 -0000
[...]
>@@ -164,20 +164,23 @@
> nsresult
> XPCOMGlueLoadXULFunctions(const nsDynamicFunctionLoad *symbols)
> {
>     ULONG ulrc = NO_ERROR;
> 
>     if (!sXULLibrary)
>         return NS_ERROR_NOT_INITIALIZED;
> 
>     nsresult rv = NS_OK;
>     while (symbols->functionName) {
>-        ulrc = DosQueryProcAddr(sXULLibrary, 0, symbols->functionName, (PFN*)symbols->function);
>+        char buffer[512];
>+        snprintf(buffer, sizeof(buffer),
>+                 "_" "%s", symbols->functionName);
>+        ulrc = DosQueryProcAddr(sXULLibrary, 0, buffer, (PFN*)symbols->function);

Is there a reason for the space between "_" and "%s", why not "_%s"? Are symbols always below 511 chars in length? Otherwise perhaps better dynamically allocate the buffer?


>--- xulrunner/stub/nsXULStub.cpp	25 Aug 2006 20:33:34 -0000	1.7
>+++ xulrunner/stub/nsXULStub.cpp	1 Oct 2006 23:33:01 -0000
[...]
>@@ -154,20 +163,27 @@
>   CFStringGetCString(iniPathStr, iniPath, sizeof(iniPath),
>                      kCFStringEncodingUTF8);
>   CFRelease(iniPathStr);
> 
> #else
> 
> #ifdef XP_WIN
>   if (!::GetModuleFileName(NULL, iniPath, sizeof(iniPath)))
>     return 1;
> 
>+#elif defined(XP_OS2)
>+   PPIB ppib;
>+   PTIB ptib;
>+ 
>+   DosGetInfoBlocks( &ptib, &ppib);
>+   DosQueryModuleName( ppib->pib_hmte, sizeof(iniPath), iniPath);;
>+

Will remove the double ;; and the extra spacing on checkin.

>@@ -258,20 +274,28 @@
>     if (NS_FAILED(rv)) {
>       // XXXbsmedberg: Do something much smarter here: notify the
>       // user/offer to download/?
> 
>       Output(PR_FALSE,
>              "Could not find compatible GRE between version %s and %s.\n", 
>              range.lower, range.upper);
>       return 1;
>     }
>   }
>+#ifdef XP_OS2
>+  sprintf(tmpPath,greDir);
>+  lastSlash = strrchr(tmpPath, PATH_SEPARATOR_CHAR);
>+  *(lastSlash) = '\0';
>+  // DosSetExtLIBPATH("T", LIBPATHSTRICT);
>+  DosSetExtLIBPATH(tmpPath, BEGIN_LIBPATH);
>+  // DosSetCurrentDir(tmpPath);
>+#endif

Not sure I understand why this is necessary, but then I have never had the time to try XULRunner... How about a comment? (I added one below that might or might not make sense.) Should you not be using strcpy() instead of sprintf (it's a bit weird to see sprintf without any format string)? Is greDir guaranteed to contain a path seperator here? Otherwise strrchr() will return NULL and it will crash when assigning '\0'. Also, the brackets around lastSlash are not necessary. Do you want to keep the commented lines? How about

  #ifdef XP_OS2
    // On OS/2 we need to set BEGINLIBPATH to be able to find subsequent DLLs
    strcpy(tmpPath, greDir);
    lastSlash = strrchr(tmpPath, PATH_SEPARATOR_CHAR);
    if (lastSlash) {
      *lastSlash = '\0';
    }
    DosSetExtLIBPATH(tmpPath, BEGIN_LIBPATH);
  #endif

Comment 9

[Andy Willis (abwillis)]

In the first 2 cases I copied and pasted from other parts of the codebase.
In the last case the beginlibpath has to be set so that we can install xulrunner dlls.  The comment should be fine, or instead of subsequent you could put xulrunner dll's to be more explicit (either way).  
The readme should probably also be updated... install xulrunner (which will probably unzipping the files to desired directory) and set gre_home to that directory (either in config.sys or a command script, multiple instances can coexist as long as the gre_home points to the desired version -- note if the xulrunner directory exists under an application (such as Chatzilla) that is using xulrunner that version takes precedence over the gre_home environment variable).  
Copy xulrunner-stub.exe to the application directory (this is where application.ini resides) and give it the application name (name doesn't have to be changed but makes it easier to track).
If Chatzilla is in e:\chatzilla copy xulrunner-stub.exe to e:\chatzilla\cz.exe (or chatzilla.exe).  
For clarification, normally cz.exe would use the libxul that is located at gre_home.  If d:\chatzilla\xulrunner exits it will use that version and ignore gre_home (which could be useful if you are wanting a specific version for a specific app... maybe an older version, without having to change the gre_home).

Comment 10

[Andy Willis (abwillis)]

Ok, I remember why I separated "_" "%s".  I copied it from BeOS code and it had LEADING_UNDERSCORE defined as "_" so I separated it, there is no reason to do so... I see Mac has  "_%s" so that would be fine.

Comment 11

[Walter Meinl]

Peter I built with your suggested changes Firefox on top of XULRunner. No problems during build. I'm posting in the moment with this build :)
Just to mention, there was a check-in after r+ for Andy's patch
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xulrunner/stub&command=DIFF_FRAMESET&file=nsXULStub.cpp&rev2=1.8&rev1=1.7
preventing the first hunk of then nsXULStub.cpp patch to apply cleanly. I'll attach the trivial change concerning only this part of nsXULStub.cpp in a second.

Comment 12

[Walter Meinl]

Attachment: cleanly applying 1st hunk of nsXULStub.cpp

Comment 13

[Peter Weilbacher]

Comment on attachment 241778 [details] [diff] [review]
remove libpathstrict

Walter, thanks for testing. I am trying to build xulrunner myself just now.

>--- xpcom/glue/nsGREGlue.cpp	27 Sep 2006 16:07:06 -0000	1.20
>+++ xpcom/glue/nsGREGlue.cpp	1 Oct 2006 23:32:56 -0000
[...]
> #elif XP_WIN
>     if (_fullpath(aBuffer, p, aBufLen))
>       return NS_OK;
>+#elif XP_OS2
>+     // realpath on OS/2 returns a unix-ized path, so re-native-ize
>+    if (realpath(p, aBuffer))
>+     for (char* ptr = strchr(aBuffer, '/'); ptr; ptr = strchr(ptr, '/')) *ptr = '\\';
>+      return NS_OK;

Hmm, looking at stuff more closely, are there not curly brackets missing?

Comment 14

[Peter Weilbacher]

Attachment: Nits fixed

This is what I changed Andy's patch to. It addresses all my nits and compiles fine and everything seems to work. Mike's review carried over. I will check this in soon if nobody protests.

Comment 15

[Walter Meinl]

It rocks! bla_check2.diff works fine for ff on xr. Moreover, the patch did not affect building "traditional" minefield :)

Comment 16

[Peter Weilbacher]

Walter, thanks for confirming again.

Fixed checked into trunk.