219705 - Crash on loading Java plugin INTERNAL ERROR on Browser End

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[alexander j pierce]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030918 Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030918 Firebird/0.6.1+

Using MozillaFirebird built w/ gcc3.3.1, and the ns610-gcc plugin from Java(TM)
2 Runtime Environment, Standard Edition (build 1.4.2_01-b06), MozillaFirebird
crashes on loading a Java applet with the following error:

INTERNAL ERROR on Browser End: JavaPluginInstance5::GetValue - Not implemented
System error?:: Success

This setup (with the same compiler) worked as of the 20030915 dated build.
Only change is a new build of the browser.

Reproducible: Always

Steps to Reproduce:
1. visit a website with a Java applet
2.
3.

Actual Results:  
Browser exits with following error:

INTERNAL ERROR on Browser End: JavaPluginInstance5::GetValue - Not implemented
System error?:: Success

Expected Results:  
Loaded and displayed Java applet

Comment 1

[tor]

Works:  2003-09-15-10-trunk nightly
Broken: 2003-09-16-10-trunk nightly

Comment 2

[Bob Firth]

adding keyword: regression

fixing typo in URL field.

has anyone tried this on mozilla? I doubt this is a bug with Firebird. Suggest
move to product: Browser

Comment 3

[Andy Backa]

I'm running WinXP, but I've having issues also with the new JVM.  Simple applets
like games sites are the easiest (Yahoo! and Pogo).  Yahoo! tells me to install
a JVM, while Pogo tells me my JVM is corrupted.  Both work fine with IE and the
new JVM.

Comment 4

[Andrew Benton]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5b) Gecko/20030921 Firebird/0.7+
crashes for me when I visit
http://www.map24.com/map24/index.php3?map24_sid=4369fc2fec1db432d3396b39267be1a0

Comment 5

[Dai Toyama]

The Map24 site loads and functions with my own 2003-09-20 build, which is
compiled with GCC 3.2.3.  I use Java 1.4.2 (ns610-gcc32).

Comment 6

[tor]

Reverting the checkin for bug 189229 allows Java to run again.

Comment 7

[tor]

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

Comment 8

[Robin Lu]

It is a java plugin bug. When the browser calls GetValue with a key unknown to
java plugin, java plugin exits instead of returning an error. We have filed a
bug to java plugin team at Sun.

Comment 9

[Joshua Xia]

Attachment: check version of JPI before GetValue is invoked

Comment 10

[Joshua Xia]

Comment on attachment 131970 [details] [diff] [review]
check version of JPI before GetValue is invoked

Please +sr/r.

Thanks a lot!

Comment 11

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

Comment on attachment 131970 [details] [diff] [review]
check version of JPI before GetValue is invoked

>+nsresult nsPluginNativeWindowGtk2::CanGetValueFromPlugin(nsCOMPtr<nsIPluginInstance> &aPluginInstance)
>+{
>+  if(aPluginInstance) {
>+    nsresult rv;
>+    nsIPluginInstancePeer *aPeer;
>+    rv = aPluginInstance->GetPeer(&aPeer);
>+    if (NS_SUCCEEDED(rv) && aPeer) {
>+      const char *aMimeType = nsnull;
>+      aPeer->GetMIMEType((nsMIMEType*)&aMimeType);
>+      if (aMimeType &&
>+          (PL_strncasecmp(aMimeType, "application/x-java-vm", 21) == 0 ||
>+           PL_strncasecmp(aMimeType, "application/x-java-applet", 25) == 0)) {
>+#ifdef OJI

Why not wrap the whole function body in #ifdef OJI?  It doesn't appear to have
any effect otherwise.

>+        nsCOMPtr<nsIJVMManager> jvmManager = do_GetService(nsIJVMManager::GetCID(),
>+                                                           &rv);
>+        if (NS_SUCCEEDED(rv)) {
>+          const char* jpiDescription;
>+          jvmManager->GetJavaPluginDescription(&jpiDescription);
>+          // Java Plugin support Xembed from 1.5
>+          if (PL_strcasecmp(jpiDescription, "Java(TM) Plug-in 1.5") < 0)

If this is intended to be a version check, please make it more robust.	It
sounds like it should be a capability check instead, for other JVMs and OJI
implementations.  Can you try to use a check for actual XEmbed support?

If not, you should probably check instead that the prefix of the string is
"Java(TM) Plug-in ", and then compare the version numbers separately.  Another
vendor's plugin might not have this exit-on-error bug, and we shouldn't disable
this on them just because their description strcmps lower!

>+            return PR_FALSE;

The function is declared are returning nsresult, so you can't return PRBools. 
(For one thing, PR_FALSE == NS_OK!)

>+        }
>+#endif
>+      }
>+    }
>+  }
>+  
>+  return PR_TRUE;
> }

Comment 12

[Jack Lu]

Overall, the patch looks good. I didn't look into the detail. But here are some 
comments from me.
* In generall, when the interface got published, we are not supposed to change 
it. If we want, we can subclass it. So changing nsIJVMManager is not recommened 
since it will break the binary compatibility for any C++ object which implements 
those interfaces.

* We should be able to call nsIPlugin::GetValue easily without nsIJVMManager 
gets involved. I think we can do something like: 

   static NS_DEFINE_CID(kCPluginManagerCID, NS_PLUGINMANAGER_CID);
   nsIPlugin pluginService = do_GetService(NS_GET_CID(kCPluginManagerCID);
   if (pluginService)
      pluginService->GetValue(....);

Let me know if you have question regarding my comment.

Comment 13

[Brendan Eich [:brendan]]

Based on shaver's and xiaobin's comments, the patch is not generally good, alas.

I don't think nsIJVMManager should be extended, especially not by adding a
method in the middle.

Drivers need resolution for 1.5final fast -- we either need the patch for bug
189229 backed out, or a proper fix here, in the next few days.

/be

Comment 14

[Asa Dotzler [:asa]]

This is 1.6a and does not impact the 1.5 branch.

Comment 15

[Brendan Eich [:brendan]]

Er, I was confused into thinking that bug 189229 was fixed on the 1.5 branch. 
If not, then never mind my rush -- but please do come up with a better patch.

/be

Comment 16

[Joshua Xia]

Attachment: Better patch without modification any interface

Comment 17

[Joshua Xia]

Attachment: change return value to be PRBool

Comment 18

[Joshua Xia]

Comment on attachment 132057 [details] [diff] [review]
change return value to be PRBool

sr/r please.

Thanks!

Comment 19

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

Comment on attachment 132057 [details] [diff] [review]
change return value to be PRBool

What about the other issues from my and Xiaobin's review?

- version check needs to be improved

- why do we do anything but return PR_TRUE #ifndef OJI?

- (what about other, non-Java plugins?	If you're adding a method to the
generic gtk2 plugin code, shouldn't getvalue reflect the Xembed support for
everything, and not just OJI bits?)

Comment 20

[Joshua Xia]

In fact, We needn't #ifdef OJI.
We need only check Java Plugin 's version when meet applet.
this patch will don't infect other plugin.

Comment 21

[Pratik]

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

Comment 22

[Jack Lu]

Mike,

I think the reason of fixing this bug is due to an old bug inside JRE. When 
browser calls GetValue and passed an undefined nsPluginVariable, instead of 
returning some error code, it crashes. This is why we need to have this patch to 
do explicit checking whether it is JRE 1.5 which has the fix to the bug above.

So I think what Joshua tried to do is reasonable. He basically assumes other 
plugin's GetValue method can be safely called while only checking Java Plug-in.

If Mike agrees, I will give "GO" for review.

Comment 23

[Robin Lu]

> - (what about other, non-Java plugins?	If you're adding a method to the
> generic gtk2 plugin code, shouldn't getvalue reflect the Xembed support for
> everything, and not just OJI bits?)

There's already one for Xembed support for general query. This bug is caused by
java plugin being not able to response to this general query correctly, so we
have to do something especially for java plugin.

Comment 24

[Joshua Xia]

Hi Mike,

I must explain this patch for you:

Java Plugin (version < 1.5 without Xembed support) 's "GetValue" will exit
process if it meet a new key (for example: NPPVpluginNeedsXEmbed)  so that we
want to distinguish JPI 's version before "GetValue".

This is a critical workaround of JPI/Mozilla plugin 's Xembed supoort.
It only for Sun's Java Plugin and the version check also valid for Sun's JPI.

Comment 25

[walter sheets]

I've been looking for JRE 1.5 and can't find it.  Is there
really a 1.5?  Even an alpha version?

Comment 26

[Joshua Xia]

not external access yet.

Comment 27

[Joshua Xia]

Now there are three ways to deal with this bug:
1. Roll back the Xembed implement patch so that the crash don't happen again,
but Xembed won't work.
2. Let it be, Xembed will work but all Old version Java Plugin (< jre 1.5) will
crash.
3. Optimize patch for #219705 and checkin it, both Xembed and Java Plugin will work.

Please conside these three ways, we think the third is the best,
Of course this is a workaround just for Sun 's Java Plugin. 

Sun's Java Plugin (which version < 1.5 without Xembed support) 's "GetValue"
will exit
process if it meet a new key (for example: NPPVpluginNeedsXEmbed),  so that we
want to distinguish JPI 's version before "GetValue".

If we find Java Plugin 's version < 1.5, we can't invoke "GetValue".
This is special workaround for Sun 's JPI because it is difficult for Sun 's JPI
team
to fix all the old version JPI.

We expect that the other plugin works well when "GetValue", so I return TRUE if
it is not Java Plugin

This is a workaround of JPI/Mozilla plugin 's Xembed support.
It only for Sun's Java Plugin and the version check also valid for Sun's JPI,
and mozilla only link to Sun's JPI until now. 

Comment 28

[Boris Zbarsky [:bzbarsky]]

Joshua, shaver's point was that if you have a plug-in that:

1)  Supports the Java MIME types
2)  Has (PL_strcasecmp(jpiDescription, "Java(TM) Plug-in 1.5") < 0)
3)  Supports XEmbed

then this patch will fail, because we will in fact _not_ use XEmbed for it. 
Hence his suggestion that instead of just using PL_strcasecmp you check that the
jpeDescription starts with the string "Java(TM) Plug-in" and _if_ that is the
case, check the version number (eg via the same PL_strcasecmp you have now or by
some other method).

I think we all agree that option 3 in comment 27 is what we want....

Comment 29

[Joshua Xia]

Boris,

Thanks! I got his idea.

I am looking for a way to identify that the Java Plugin 's vendor is "Sun
Microsystem Inc.". It is necessary even though there is only Sun 's JPI for
mozilla until now.

I am waiting for Xiaobin (member of JPI team) 's idea for how to get vendor
information from JPI side. I scaned their code and have not found the way.

We will provide new patch ASAP.

Comment 30

[Marek Stępień [:marcoos, inactive]]

The same problem with Blackdown Java-Linux Java(TM) Plug-in1.4.1, Firebird 
20030928-trunk (gtk2+xft).

I'm not sure how the Blackdown's plugin is different from the Sun's one, so if
this comment is simply bugspam, then please ignore it ;)

Comment 31

[Joshua Xia]

Attachment: patch change version check and #ifdef OJI

1. change scope of #ifdef OJI to whole "CanGetValueFromPlugin" function.
2. stricter version checking:
   first, make sure this is Sun's Java Plugin which description start with
"Java(TM) Plug-in"."Java(TM) Plug-in" is Trademark of Sun 's JPI.
(Blackdown java plugin 's description start with "<a
href="http://www.blackdown.org/java-linux.html">Blackdown Java-Linux</a>")
   second, check version number.

If Blackdown JPI crash in this case, it's GetValue function shouold be fixed.

Comment 32

[Joshua Xia]

Comment on attachment 132391 [details] [diff] [review]
patch change version check and #ifdef OJI

sr/r?

Thanks a lot!

Comment 33

[Joshua Xia]

Comment on attachment 132391 [details] [diff] [review]
patch change version check and #ifdef OJI

sorry, my mistake

Comment 34

[Joshua Xia]

Attachment: This is the latest patch. change version check and #ifdef OJI

description see: Comment #31

Comment 35

[Joshua Xia]

Comment on attachment 132392 [details] [diff] [review]
This is the latest patch. change version check and #ifdef OJI

sr/r please.

Thanks a lot!

Comment 36

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

Comment on attachment 132392 [details] [diff] [review]
This is the latest patch. change version check and #ifdef OJI

Please make the method always defined and always called -- #ifing an if
conditional like that makes me nervous -- but put all but "return PR_TRUE" of
the method body inside #ifdef OJI.

With that nit picked, sr=shaver.

Comment 37

[Joshua Xia]

Ok, like this one?

+#ifdef OJI
+#include "plstr.h"
+#include "nsIPlugin.h"
+#include "nsIPluginHost.h"
+
+static NS_DEFINE_CID(kPluginManagerCID, NS_PLUGINMANAGER_CID);
+#endif
 
 class nsPluginNativeWindowGtk2 : public nsPluginNativeWindow {
 public: 
@@ -58,6 +65,9 @@
 private:
   GtkWidget*  mGtkSocket;
   nsresult  CreateXEmbedWindow();
+  PRBool    CanGetValueFromPlugin(nsCOMPtr<nsIPluginInstance> &aPluginInstance);
 };
 
 nsPluginNativeWindowGtk2::nsPluginNativeWindowGtk2() : nsPluginNativeWindow()
@@ -103,8 +113,11 @@
     nsresult rv;
     PRBool val = PR_FALSE;
     if(!mGtkSocket) {
-      rv = aPluginInstance->GetValue
-        ((nsPluginInstanceVariable)NPPVpluginNeedsXEmbed, &val);
+      if (CanGetValueFromPlugin(aPluginInstance))
+        rv = aPluginInstance->GetValue
+               ((nsPluginInstanceVariable)NPPVpluginNeedsXEmbed, &val);
     }
 #ifdef DEBUG
     printf("nsPluginNativeWindowGtk2: NPPVpluginNeedsXEmbed=%d\n", &val);
@@ -155,3 +168,47 @@
 
   return NS_OK;
 }
+
+PRBool
nsPluginNativeWindowGtk2::CanGetValueFromPlugin(nsCOMPtr<nsIPluginInstance>
&aPluginInstance)
+{
+#ifdef OJI
+  if(aPluginInstance) {
+    nsresult rv;
+    nsIPluginInstancePeer *aPeer;
+
+    rv = aPluginInstance->GetPeer(&aPeer);
+    if (NS_SUCCEEDED(rv) && aPeer) {
+      const char *aMimeType = nsnull;
+
+      aPeer->GetMIMEType((nsMIMEType*)&aMimeType);
+      if (aMimeType &&
+          (PL_strncasecmp(aMimeType, "application/x-java-vm", 21) == 0 ||
+           PL_strncasecmp(aMimeType, "application/x-java-applet", 25) == 0)) {
+        nsCOMPtr<nsIPluginHost> pluginHost = do_GetService(kPluginManagerCID, &rv);
+        if (NS_SUCCEEDED(rv) && pluginHost) {
+          nsIPlugin* pluginFactory = NULL;
+
+          rv = pluginHost->GetPluginFactory("application/x-java-vm",
&pluginFactory);
+          if (NS_SUCCEEDED(rv) && pluginFactory) {
+            const char * jpiDescription;
+
+            pluginFactory->GetValue(nsPluginVariable_DescriptionString,
(void*)&jpiDescription);
+            /** 
+             * "Java(TM) Plug-in" is Sun's Java Plugin Trademark,
+             * so we are sure that this is Sun 's Java Plugin if 
+             * the description start with "Java(TM) Plug-in"
+             **/
+            if (PL_strncasecmp(jpiDescription, "Java(TM) Plug-in", 16) == 0) {
+              // Java Plugin support Xembed from JRE 1.5
+              if (PL_strcasecmp(jpiDescription + 17, "1.5") < 0)
+                return PR_FALSE;
+            }
+          }
+        }
+      }
+    }
+  }
+#endif
+  
+  return PR_TRUE;
+}

Comment 38

[Jack Lu]

Looks good to me.

Comment 39

[Max Kanat-Alexander]

Xiaobin --

Does that mean review+?

-M

Comment 40

[Jack Lu]

Yes, should I update it somewhere?

Comment 41

[Joshua Xia]

checked in -> fixed

Comment 42

[tor]

Using the Blackdown java plugin, java applets still cause the browser to exit
in the same manner.

Comment 43

[Joshua Xia]

That is the Blackdown java plugin 's bug, it must be fixed on Blackdown java
plugin side. my workaround only avoid Sun 's Java Plugin crash mozilla.

Comment 44

[tor]

To a first approximation, Blackdown is a recompile of the Sun java source.
As such, it will tend to exhibit the same bugs, and can probably be made
to work in the same way.

Comment 45

[tor]

Attachment: add check for Blackdown java plugin

Not pretty, but it gets java running again with Blackdown.

Comment 46

[Christopher Blizzard (:blizzard)]

Comment on attachment 132802 [details] [diff] [review]
add check for Blackdown java plugin

Geez.

Comment 47

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

Comment on attachment 132802 [details] [diff] [review]
add check for Blackdown java plugin

Aye.

Comment 48

[tor]

Blackdown JVM detection patch checked in.

Comment 49

[Joshua Xia]

Are you sure that the Blackdown java plugin support Xembed from version > 1.5?

Thanks!

Comment 50

[tor]

Since Blackdown is a third-party port/compile of the licensed Sun Java source
tree, I think that's a reasonable assumption.

Comment 51

[Christopher Blizzard (:blizzard)]

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

Comment 52

[Max Kanat-Alexander]

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

Comment 53

[Philip K. Warren]

We are seeing this same problem on AIX GTK2 builds using the IBM Java plugin. It
looks like we will need to add another check for the IBM plugin.

Comment 54

[tor]

Attachment: ... and again for the IBM plugin

Comment 55

[Philip K. Warren]

I have tested tor's latest patch with the Java 1.3.1 and 1.4.1 plugins for AIX
and it does seem to fix the crash.

Comment 56

[Roland Mainz]

Comment on attachment 137518 [details] [diff] [review]
... and again for the IBM plugin

What about implementing this using extended regular expressions (and a pref to
set the eregex) instead of hardcoding the value for each single JRE ?

Comment 57

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

Comment on attachment 137518 [details] [diff] [review]
... and again for the IBM plugin

I don't understand why we need a case-insensitive comparison against "1.5", but
you're probably just following their lead.  sr=shaver.

Comment 58

[Christopher Blizzard (:blizzard)]

Comment on attachment 137518 [details] [diff] [review]
... and again for the IBM plugin

r=blizzard

Comment 59

[Daniel (Leaf) Nunes]

Comment on attachment 137518 [details] [diff] [review]
... and again for the IBM plugin

a=leaf on behalf of drivers for 1.6

Comment 60

[tor]

Checked in.