On Windows, when nsPluginHostImpl::LoadPlugins() is called, include the installation path of the JRE in plugin scan. If the user does not have the OJI plugin in the "plugins" folder, the Default Plugin redirects the user to download the JRE from Sun. Currently, their plugin installer is not aware how to detect Gecko installation. This will eventually change, but until such time their installer is updated and released, it would be nice if Mozilla could detect the installation path of the JRE from enumerating Windows registry path: [HKEY_LOCAL_MACHINE\SOFTWARE\JavaSoft\Java Plug-in\1.3.0_01] "Java Home" = "C:\Program Files\JavaSoft\JRE\1.3.0_01" ..for "Java Home" keys that point to the installlation directory. The JRE installer creates these when it installs.

cc:ing a bunch of people to find out if this would solve their Java installation problems at least for beta and Sun folks to find out if this is even an option for us to search for the Java OJI DLL's in their default installed place.

I agree that Mozilla should look for the registry key of Java Plugin and get the value of the "Java Home",go to where "Java Home" points to and get the DLL file and put it into the plugins folder of Mozilla. Even in the future, it is better that Mozilla(or Netscape) should take this responsiblity because JRE has no idea about which version of JRE works with which version of Netscape, but Netscape people know it. Another solution is before Mozilla launching the jre.exe, it first get the DLL file and put it into the plugins folder and we don't need to care about the rest.

Okay, here's my first patch. Be sure to set the pref in all.js: pref("plugin.enabled_JRE_Plugin_Scan", true); Would it be possible if as many people could pounce on this as possible. Especially at Sun. Also, would it be possible to get older installs of the JRE so we can test what happens if there is a pre Netscape 6 JRE installed.

Xiaobin, can you look at Peter's patch?

peterl wrote: > + char newestPath[2000]; // should path be 255? This is Windows specific file. We can probably use _MAX_PATH , which is defined on Windows as 255 if I am not mistaken.

Can we do that for "path" too? (one line above newpath)

Certainly. I am just thinking that we might have something similar in our xp module. If not, Windows _MAX_PATH should be fine in this file.

Peter: Thanks for the patch! However, this patch still can not garantee that the java version user installed works with mozilla if the java version is old. It might be better to check the java version also.

Good idea. It's not a problem. I think it's simple to add. Starting with what version of the JRE is verified to work with Mozilla? It appears that each key is a version. szKey is currently "1.3.0_01" in my build. If older JRE's follow this pattern (I hope they do), we can screen them out as long as we know the base version that works. I can use 1.3 if you think it's reasonable. Oh, another thing that Arun brought up: This assumes that the JRE one would currently download from Sun.COM with any client is the same and compatible with the one that comes with Mozilla. In othe words, there is only ONE JRE, it's current version is 1.3, and it's compatible with Mozilla (and other browsers).

oh...in the above, I mean JRE = "Java Plug-in"

I think at least now, it is reasonalbe to assume 1.3 is the right one. However, it might be adaptable to change with newer release of Java Plugin.

When was 1.3 released?

Stanley: Can you tell Peter when jre 1.3.0_01 was released?

Okay, patch #3 addresses: 1) On by default. Use pref("plugin.skip_JRE_Plugin_Scan", true); to disable. 2) Only if the key name is version 1.3 or above will it scan 3) Cut down memory a bit by using _MAX_PATH (it's win32 only code). Marking P1 per meeting this morning. Adding [Seeking Review].

Ed and other Sun engineers: I'm seeing a weird problem here. I downloaded the latest JDK for Win32 1.3.0_02 which also installed the JRE, to another path. First bug I noticed with the installer is that it did not update the "Java Plug-in" registry key that I am using in this patch with a newer version. It should have, I think, what does the last number signify on the version? No that big a deal because it's the JDK. If you install the JDK, you know what you are doing anyway and should copy DLL's. However, the JRE stand-alone installer downloaded from Sun.com needs to be tested. If an installer can't write it's location to the registry reliably, how on earth would one locate it without searching the whole drive? What convention are you using here for registry keys and versions, I think I'm just trying to guess? Putting that aside....I figured copying the DLL's from the JDK/JRE/BIN folder to my plugins folder would work since the ones with the JRE I was presented to download are (older?) version 1.3.0_01 and these were (newer?) 1.3.0_02 as read in Explorer properties of each DLL. They registered fine in about:plugins but visiting a page that required Java brought up the Default Plugin!!!! It shouldn't do that, especially if it's registered in about:plugins. I remember seing a bug like this for Linux (not Win32) but perhaps there is some mix up in the bits? Am I missing something here? Environment variable? There is no point in doing the work in the patch if it will break in future versions.

Peter: First I don't think the registry key has problem. I downloaded the JRE1.3.0_02 and it successfully updated my registry keys. Second I can't see any problem of using the plugin even I have two different versions of JRE. The only thing I need to do is to configure the Java control panel to point to use the right version of JRE you want.

Okay, ignore my last rant and rave about the JDK and version _02. I just installed JRE _02 and it installed AND updated my registry correctly. There is no bug with the JRE _02. My patch works as intended. I now have version 1.3.0_01 and 1.3.0_02 installed at the SAME time on my system. However, my patch ensures that the newer, 1.3.0_02 is scanned and not the older one. Having said that, I'm asking for a super review. Marc? Xiaobin Lu game me an r= via e-mail as long as update to get the pref service at the top of the method.

Ok, prepare to be henpecked! :-) In general the patch looks great. Couple of nits, take 'em or leave 'em. sr=waterson + char szKey[512] = "Software\\JavaSoft\\Java Plug-in"; You should either size this to _MAX_PATH, no? Also, why is this the only variable that gets a hungarian name? Wouldn't ``key'' be ok? + pathlen = sizeof(path); Why do you initialize this way out here? Heck, why even declare it out here? It's used only in a nested scope. + long result = ::RegOpenKeyEx(HKEY_LOCAL_MACHINE, szKey, 0, KEY_READ, &baseloc); Should result be a LONG since we're using MS types n' stuff? + numChars = pathlen = _MAX_PATH; numChars should probable get |sizeof(szKey)|, er ``key''. And pathlen can be moved in a few scopes. + if (CompareFileTime(&modTime,&curVer) > 0 && atof(szKey) >= 1.3) Maybe qualify |CompareFileTime()| at global scope to be consistent; i.e., |::CompareFileTime(...)|. + if (newestPath[0] != 0) + PL_strcat(newestPath,"\\bin"); Possible buffer overrun? + *(nsFileSpec*)this = allocPath; This is a bit weird; probably the best thing to do here is call |operator=()| directly; e.g., |nsFileSpec::operator=(allocPath)|. Or maybe not. God, I hate nsFileSpec.

This was checked in before the weekend bug left disabled because of an ASSERTION that the service manager was being held past XPCOM shutdown. I'm pretty sure this is Java as I tested with all plugin dirs empty, my patch off, and only the OJI plugin in my plugins folder. Xiaobin, do you know the bug # for that ASSERTION or the eta on a fix if it's Sun internal? Anyway, embedding vendors should be able to simply add: pref("plugin.enabled_JRE_Plugin_Scan", true); to the all.js file in their prefs to make this work.

oops...changed the pref name at the last second: pref("plugin.do_JRE_Plugin_Scan",true); That should ensure that the Java plugin is always picked up if it's been ever installed. The master Java enabled pref can override. To verify, remove the 5 Java DLL's from your Netscape 6 plugins folder. Be sure they are NOT in your Netscape 4.x plugins folder either. Turn the pref on by adding that line to all.js, Visit a site that needs Java. Please let me know if this does not work.

Peter, I am not sure what you mentioned about " Service Manager being past XPCOM shutdown" is java related bug. I don't know what's the bug number. This problem seems to exsits for a long time.

It's Java related in that if the OJI plugin is picked up and I simply hit CANCEL to exit the profile manager (not even the browser), I get an ASSERTION. If I remove the OJI plugin, this does not happen. I do not see this behavior in other 4.x or XPCOM plugins so I assume it's a probelm with Java? I'd like to be in on the tracking of that issue so I'm wondering if there is already a bug filed since it's been around for a while?

Awesome! works nicely on today's windows trunk (0515).I tried both cases and the expected behaviour is seen. When the pref is set to 'true' (with java plugin not present in 6.x folder), doing 'about:plugins" lists the java plugin from the ' c:\prog\javasoft\...\bin\' path. Setting the pref to 'false' does not list any java plugin (as expected) I do not see any crash on exit either.

Okay, now that Java doesn't crash or ASSERT on exit and plugins looks to be in better shape, maybe I can turn this on by default? Phil, what kind of approval do I need for that? Planning to flip the variables in he code to make it default on.

Peter, the only approval you need is from drivers@mozilla.org, once you have an r= and sr=. I'm wondering how the extra refcount disappeared, though. Did Java change?

As desireable as this feature may be for beta, until we are sure we are not leaking a JVM at shutdown, I would suggest not checking it in. For NT this may be fine, but for a 9x based OS, it's not fun. I don't have purify installed on my system, but if somebody with it installed could possibly run with Java and verify that we don't leak at shutdown, I'd feel a lot better about checking this in. PDT: this is still mozilla0.9.1. I think the orriginal reviews in this bug are still valid because they were to leave it on by default. I can check this in at any time. What would you like me to do?

> What would you like me to do? You could verify the status of the leak using a Purify buddy (I know there are licensed seats in SD) or the GC leak diff tool.

Since Java works in mozilla and NS commercial (but not embedded) I'm moving this off 0.9.1. Peter, who's on the hook to investigate the memory leak which prevents us from turning this on?

In an e-mail exchange between Stanley and Xiaobin, I think I picked up on the hint that the Release for the held service manager is taken care of by the Java plugin in ::Shutdown(). However, nsIPlugin::Shutdown() won't ever be called if no instance of a Java plugin is ever created. I get the ASSERT even if I exit the App from the profile selection dialog. Also, if I recall correctly, this seemed to start happening around the same time that lazy JVM loading began. This is just all speculation, perhaps with access to the source, Stanley or Xiaobin can fill in the gaps.

Peter: I would like to say the Assertion Box poped up when shut down the browser have been exists for a long time not only after lazy loading fix. So, I don't think lazy loading cause that. I looked at the code when the browser starts up. And this is the stack trace. CPluginServiceProvider::CPluginServiceProvider(nsISupports * 0x009558c0) line 123 CJavaPluginFactory::CJavaPluginFactory(CJavaPluginApp * 0x025b3f50 class CJavaPluginApp g_App, nsISupports * 0x009558c0) line 119 + 38 bytes CPluginModule::NSGetFactory(CPluginModule * const 0x025d1c40, const nsID & {...}, nsISupports * 0x009558c0, nsIFactory * * 0x01043084) line 73 + 46 bytes NSGetFactory(nsISupports * 0x009558c0, const nsID & {...}, const char * 0x00000000, const char * 0x00000000, nsIFactory * * 0x01043084) line 70 + 31 bytes nsPluginHostImpl::GetPluginFactory(nsPluginHostImpl * const 0x0103fe34, const char * 0x02333738, nsIPlugin * * 0x0012f9cc) line 3561 + 29 bytes nsJVMManager::StartupJVM() line 602 + 32 bytes nsJVMManager::MaybeStartupLiveConnect() line 783 + 20 bytes nsJVMManager::StartupLiveConnect(nsJVMManager * const 0x0103e6b8, JSRuntime * 0x00ce5e20, int & 0x00000000) line 128 + 11 bytes nsJSEnvironment::nsJSEnvironment() line 1496 + 49 bytes nsJSEnvironment::GetScriptingEnvironment() line 1437 + 27 bytes NS_CreateScriptContext(nsIScriptGlobalObject * 0x00ffdc50, nsIScriptContext * * 0x0100d130) line 1543 + 5 bytes nsDOMSOFactory::NewScriptContext(nsDOMSOFactory * const 0x00fff0c0, nsIScriptGlobalObject * 0x00ffdc50, nsIScriptContext * * 0x0100d130) line 123 + 13 bytes nsDocShell::EnsureScriptEnvironment(nsDocShell * const 0x0100d080) line 5704 + 67 bytes nsWebShell::GetInterface(nsWebShell * const 0x0100d0a8, const nsID & {...}, void * * 0x0012fbf0) line 331 + 19 bytes nsGetInterface::operator()(const nsID & {...}, void * * 0x0012fbf0) line 37 + 31 bytes nsCOMPtr<nsIDOMWindowInternal>::assign_from_helper(const nsCOMPtr_helper & {...}, const nsID & {...}) line 971 + 18 bytes nsCOMPtr<nsIDOMWindowInternal>::nsCOMPtr<nsIDOMWindowInternal>(const nsCOMPtr_helper & {...}) line 553 nsAppShellService::GetHiddenWindowAndJSContext(nsAppShellService * const 0x01013e60, nsIDOMWindowInternal * * 0x0012fc98, JSContext * * 0x0012fca0) line 730 + 32 bytes nsAppShellService::SetXPConnectSafeContext() line 181 + 40 bytes nsAppShellService::CreateHiddenWindow(nsAppShellService * const 0x01013e60) line 258 main1(int 0x00000001, char * * 0x00954050, nsISupports * 0x00000000) line 1111 main(int 0x00000001, char * * 0x00954050) line 1426 + 37 bytes mainCRTStartup() line 338 + 17 bytes KERNEL32! 77f1ba06() Actually the ServiceManager used in this context is a GlobalServiceManager. Basedon the comment there, it is NOT A LEAK ( see GetPluginFactory function in nsPluginHostImpl.cpp). And the assertion box always poped up even when we don't visit an applet page. So, I suspect that the ServiceManager is held by some other components other than OJI component. How do you think about this?

Thank, that method looks very interesting, especially the call to plugin->Initialize(); I'll take a closer look. Correct me if I'm wrong, but I thought that if an application didn't properly release all it's resources when it quit on Windows 95/98/ME, that memory won't get reclaimed as it does on NT.

Either I'm doing something wrong in my leak detection (I just set the environment variables as noted next to the attachments) or we seem to be leaking a lot more when stuff when the Java DLL's are swept. This test was to simply bring up the profile choice window and exit from there, NO WEB BROWSING!!! This is on Win32. Chofmann? I'm not quite sure if this is important, but it seems to me that with Java, the report indicates that a nsServiceManagerImpl leaked along with a nsPluginHostImpl and a nsJVMManager. Am I reading these reports right? Does it matter? I've attached a patch to revert this to be default ON, however, I feel queasy about what these numbers mean.

Xiaobin ask me to duplicate this information here, so here it is: All, I have been looking at the Browser code for Netscape 6 on Solaris and there seems to be an extra Addref in: http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginHostImpl.cpp at line 3553 My understanding is that when ever you get an interface pointer back from a function call you should assume it has been AddRef'ed already. In fact in the Java Plugin we do this for the interface pointer returned by NSGetFactory() (That is to say we AddRef the pointer before we return it). As such the Addref at line 2552 would seem to be an extra since all this code is doing is passing the interface pointer up the call stack. My understanding of the browser code is limited, so if someone could tell me what I'm overlooking here I would appreciate it. Thanks, stevek

Andrei, do you know why that extra Addref is there? Perhaps it should only be there for 4.x style plugins.

I don't know. Looks like the right place for it would be a couple of lines higher, so it addrefs only for ns4xPlugin::CreatePlugin case.

It could be the ADDREF but, moving that up for just 4.x plugin so it doesn't even get called for the Java case doesn't fix the problem. However, looking up a bit, I found that after the nsGetFactory in nsPluginHostImpl::GetPluginFactory, we call plugin->Initilize() and I recall we don't ever call Shutdown().

Okay, I think I found the problem. Using an nsCOMPtr seemed to fix the ASSERTION of the holding the global service manager. The plugin gests shutdown. I'm seeking reviews for this patch and can you try it out.

I love this kind of things.

Woohoo! Peter, I'm happy to report this works for me on win32 when applied to today's trunk using JPI 1.3.1. r=edburns. Check that bad boy in!

Wow, in just starting up and quiting at the profile box, leaks are WAY down. In fact, we only seem to be leaking a nsPluginHostImpl of 176 bytes (for some reason).

Um, what about this? http://lxr.mozilla.org/mozilla/source/modules/plugin/nglsrc/nsPluginHostImpl.cpp#3554 // No, this is not a leak. GetGlobalServiceManager() doesn't // addref the pointer on the way out. It probably should. If you look at: http://lxr.mozilla.org/mozilla/source/xpcom/components/nsServiceManager.cpp#521 you'll see that nsServiceManager::GetGlobalServiceManager() is _not_ addref'ing the pointer it returns. So, this patch adds a random |Release()| that just happens to match some other unbalanced |AddRef()| somewhere else, no?

I know nsServiceManager::GetGlobalServiceManager() isn't addrefing but just a few lines down is nsGetFactory that is doing it, as observed in the debugger. > // XXX fix ClassName/ContractID > rv = nsGetFactory(serviceManager, kPluginCID, nsnull, nsnull, > (nsIFactory**)&pluginTag->mEntryPoint); I thought using an nsCOMPtr would be the cleanest approach. Also, the nsCOMPtr manual says the Don't_AddRef macro is no longer used so I can see how the getter_AddRefs macro after the comment makes it sound weird.

But |nsGetFactory()| is the plugin's entry point, right? If we load a plugin that _doesn't_ |AddRef()| the service manager, we'll crash soon after leaving this method, because we've just done an extra |Release()|. As much as it sucks, it seems like the real bug here is that the plugin is doing an unbalanced |AddRef()|, and as such, is screwing the proverbial XPCOM pooch. There's not a lot we can do about this on our end. Or am I missing something?

Hm...that's what I thought too, I just didn't have any evidence till now. Could someone working with the Java Plugin source take a look at NSGetFactory() and let me know where when the corresponding Release is called for the service manager? cc:ing skatz@east.sun.com

Peter, I am in the process of getting rid of NSGetFactory() all together from the plugin, and switching it over to use the nsIModule scheme. I'm looking at the plugin source code for NSGetFactory () and it appears to me that we do a QueryInterface on the pointer to get the nsIServiceManager and the only release it when the browser release the factory (which never seems to happen). Is this the information you are looking for?

Yes, this is sort of what I'm looking for. Basically, I'd like to check in my patch to make scanning for Java default, however, I don't feel I should when the XPCOM_MEM_LEAK_LOG (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=37134) reveals all these leaks. I know NSGetFactory is leaving, but don't know when so I'm not quite sure how to fix this. I guess I should try to find where the plugin releases the factory and find out what to do with it. What about moving the release for the service manager to Shutdown() or just holding a weak ref to it (it's not like it's going to go away during the lfetime of the plugin)?

Ahh!!! I think I found the problem. The addref of the plugin after nsGetFactory was 2 instead of 1. This means that somebody else already created this factory and the minute I comented out the whole nsGetFactory block, all was good! Now, the hard task, who else is addrefing the plugin? Could someone with access to the Java Plugin source set a breakpoint in AddRef and see who else calls during startup besides nsPluginHostImpl::GetPluginFactory?

Peter, I think I have an answer for you. I found that our JavaConsole object (which is aggregated to the factory) was addref'ing the factory. I believe this to be a mistake and have removed that line from the code. stevek

Filed bug 88311 about XPCOM shutdown with the Java plugin present. Move to mozilla0.9.3.

no longer rtm stopper.

I do not want to turn this on untill bug 88311 is fixed. Moving to mozilla 0.9.4. It may be a moot issue as the JRE installer soon will be able to find the mozilla plugins directory as long as the installer was used.

This is an old bug that the mozilla community really wants fixed plus our embedding partners arleady use this, so why shouldn't Seamonkey? Attaching patch, please review.

is this bug also covering the fact that Java2 v1.4 beta 3 isn't found by plugin.do_JRE_Plugin_Scan

Since I think JRE 1.4 is scheduled to be an XPCOM component and will need to run regexpcom at install time, this approach won't work. I think instead of using this pref and scan, we should try to fix the XPI install scripts for Java that are hosted on Netscape.com which is where Mozilla users are taken when Java isn't detected.

Although the JS is not in, this feature is done so marking this bug FIXED. It probably won't be activated by default. See bug 123124 for detecting Java if it's already installed.

v