202363 - Cannot start up Mozilla 1.4a by clicking on a URL in AOL Communicator Mail

Status: VERIFIED FIXED

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

Description

[Wan-Teh Chang]

To reproduce this bug, make Mozilla 1.4a the default web
browser on your Windows machine.

Make sure Mozilla 1.4a is not running.

Use AOL Communicator Mail to view an email message containing
a URL, for example, a bug mail from Bugzilla.

Click on the URL in the email message.  Windows pops up this
error message dialog four times:
The procedure entry point ?Init@nsStaticCaseInsensitiveNameTable@@QAEHQAPBDH@Z
could not be located in the dynamic link library xpcom.dll.

I believe this bug was introduced when Mozilla started to use
the GRE.

Comment 1

[Wan-Teh Chang]

If I view the email message in Outlook Express and
click on the URL, Mozilla 1.4a can be started up
with no problem.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

This resets the current working directory to the application's process
directory after the application is started up.	This should fix the problem
that some applications sees if it starts up Mozilla using ShellExecute().

Comment 3

[Doug Turner (:dougt)]

i tested the above patch and it seams to fix the problem.  I was using the
following test program that wtc sent me:

#include <windows.h>
#include <shellapi.h>
#include <stdio.h>

int main()
{
    int rv;

    rv = (int)ShellExecute(NULL, "open", "http://bugzilla.mozilla.org/", NULL, NULL,
        SW_SHOWNORMAL);
    printf("ShellExecute returned %d\n", rv);
    return 0;
}


Note, i rebuilt mozilla so that the GRE and app directory were disjoint to
simulate the offical release.

Comment 4

[Wan-Teh Chang]

Here is a simpler way to reproduce the bug.  It only
requires that you have both Netscape 7 and Mozilla 1.4a
installed.

1. Start up a Command Prompt window.

2. In the Command Prompt window, change to Netscape 7's
installation directory.  For example,

   C:
   cd \Program Files\Netscape\Netscape

3. Invoke Mozilla 1.4a from that directory.  For example,

   C:\"Program Files"\mozilla.org\Mozilla\mozilla.exe

Comment 5

[Chak Nanga]

Comment on attachment 120860 [details] [diff] [review]
patch v.1

r=chak

Comment 6

[Darin Fisher]

Comment on attachment 120860 [details] [diff] [review]
patch v.1

>Index: nsGREDirServiceProvider.cpp

>-    if (PR_GetEnv("MOZILLA_FIVE_HOME") == nsnull)
>+    if (getenv("MOZILLA_FIVE_HOME") == nsnull)

ok, why the change to getenv?  there's a reason for PR_GetEnv, no?
iirc it has something to do with getenv not necessarily being 
thread safe.  wtc?


>+char* 
>+nsGREDirServiceProvider::GetNSPRPath()
>+{
>+  char* grePath = GetGREDirectoryPath();
>+  if (!grePath) {
>+    char* greEnv = getenv("MOZILLA_FIVE_HOME");
>+    if (!greEnv) {
>+      return nsnull;
>+    }
>+    grePath = strdup(greEnv);

nsCRT::strdup or PL_strdup since strdup is not portable.


>+#define NSPR_DLL "libnspr"MOZ_DLL_SUFFIX
>+
>+  int len = strlen(grePath);
>+  char* nsprPath = (char*) malloc(len + sizeof(NSPR_DLL) + sizeof(XPCOM_FILE_PATH_SEPARATOR) + 1);
>+
>+  sprintf(nsprPath, "%s" XPCOM_FILE_PATH_SEPARATOR NSPR_DLL, grePath);

sizeof("foo") returns 4 not 3, so really:

  len + sizeof(...) + sizeof(...) - 1

would be sufficient.


>Index: nsXPCOMGlue.cpp

>+    char* nsprPath = nsGREDirServiceProvider::GetNSPRPath();
>+
>+#if defined(WIN32)
>+    HINSTANCE h = LoadLibrary(nsprPath);
...
>+#elif defined(XP_UNIX)
>+    void * h = dlopen(nsprPath, RTLD_LAZY | RTLD_GLOBAL);
...
>+#endif
>+    if (nsprPath)
>+        free(nsprPath);

hmm.. maybe you should null check |nsprPath| up above where it
is first used.	how about a NS_NOTREACHED("not implemented")
for other platforms?

general comment: does GetNSPRPath really need to return a 
dynamically allocated string?  couldn't it just populate a
global string buffer and then return a const reference to it
each time?  that would simplify consumer code a lot don't you
think?

Comment 7

[Wan-Teh Chang]

Comment on attachment 120860 [details] [diff] [review]
patch v.1

I was wondering why Doug changed PR_GetEnv() to getenv() too.
Is it because we will be running this piece of code before
NSPR is loaded?

>+#define NSPR_DLL "libnspr"MOZ_DLL_SUFFIX

Is this code for Unix only?  NSPR DLL doesn't have the "lib"
prefix on Windows.

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 120860 [details] [diff] [review]
patch v.1

OH my.	I am sorry about that.	this isn't the patch that I wanted to add here.
:-/

Comment 9

[Doug Turner (:dougt)]

Attachment: patch v.1.1

Comment 10

[Doug Turner (:dougt)]

sorry about that.  the initial patch was from a different tree where I am trying
to remove the nspr dependencies -- clearly that patch isn't ready :-)

Comment 11

[Chak Nanga]

>> sorry about that.  the initial patch was from a different tree 

And i should've done a better job r='ing :-)

Comment 12

[Wan-Teh Chang]

Comment on attachment 120964 [details] [diff] [review]
patch v.1.1

r=wtc.

Nit: you might want to use the Win32
::SetCurrentDirectory() function instead of the
libc chdir() function, then you don't need to
include <direct.h>.

Comment 13

[Darin Fisher]

Comment on attachment 120964 [details] [diff] [review]
patch v.1.1

>Index: nsGREDirServiceProvider.cpp

>+  // On windows, the current directory is searched before the 
>+  // library search environment variable.  This is a very bad

do you mean the "PATH" environment variable?  if so, then just
say so ;-)


>+  // thing since libraries in the cwd will be picked up before
>+  // any that in either the application or GRE directory.

any that _are_ in either...

i agree with wtc on using SetCurrentDirectory.

sr=darin

Comment 14

[Wan-Teh Chang]

Build 2003050610 still has this problem.  Doug, you
haven't checked in your patch, right?

Comment 15

[Doug Turner (:dougt)]

no, this patch was not checked in.  

asa/seth, this is a very low risk / high gain patch.  it should have been
checked days agos.

Comment 16

[Brendan Eich [:brendan]]

dougt, how about attaching the patch you intend to checkin?

/be

Comment 17

[Doug Turner (:dougt)]

Attachment: updated patch

Comment 18

[Wan-Teh Chang]

Comment on attachment 122710 [details] [diff] [review]
updated patch

r=wtc.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 122710 [details] [diff] [review]
updated patch

a=asa (on behalf of drivers) for checkin to 1.4.

Comment 20

[Doug Turner (:dougt)]

Checked into the trunk for 1.4 final.

Checking in nsGREDirServiceProvider.cpp;
/cvsroot/mozilla/xpcom/glue/standalone/nsGREDirServiceProvider.cpp,v  <-- 
nsGREDirServiceProvider.cpp
new revision: 1.18; previous revision: 1.17
done

Comment 21

[Wan-Teh Chang]

Verified with nightly build ID 2003050908 on Windows XP.