Crash on loading Java plugin INTERNAL ERROR on Browser End

RESOLVED FIXED

Status

()

Core
Plug-ins
--
critical
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: alexander j pierce, Assigned: Joshua Xia)

Tracking

({crash, regression})

Trunk
x86
Linux
crash, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

15 years ago
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

Updated

15 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

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

Comment 2

15 years ago
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

15 years ago
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

15 years ago
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

15 years ago
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).

Updated

15 years ago
QA Contact: asa

Comment 6

15 years ago
Reverting the checkin for bug 189229 allows Java to run again.
Assignee: blake → robin.lu

Comment 7

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

Comment 8

15 years ago
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.
Assignee: robin.lu → joshua.xia
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

15 years ago
Created attachment 131970 [details] [diff] [review]
check version of JPI before GetValue is invoked
(Assignee)

Comment 10

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

Please +sr/r.

Thanks a lot!
Attachment #131970 - Flags: superreview?(blizzard)
Attachment #131970 - Flags: review?(Xiaobin.Lu)
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;
> }
Attachment #131970 - Flags: superreview?(blizzard) → superreview-

Comment 12

15 years ago
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.
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

15 years ago
This is 1.6a and does not impact the 1.5 branch.
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
(Assignee)

Comment 16

15 years ago
Created attachment 132055 [details] [diff] [review]
Better patch without modification any interface
(Assignee)

Updated

15 years ago
Attachment #132055 - Flags: review?(Xiaobin.Lu)
(Assignee)

Comment 17

15 years ago
Created attachment 132057 [details] [diff] [review]
change return value to be PRBool
(Assignee)

Comment 18

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

sr/r please.

Thanks!
Attachment #132057 - Flags: superreview?(shaver)
Attachment #132057 - Flags: review?(Xiaobin.Lu)
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?)
Attachment #132057 - Flags: superreview?(shaver) → superreview-
(Assignee)

Comment 20

15 years ago
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

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

Updated

15 years ago
Keywords: crash

Comment 22

15 years ago
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

15 years ago
> - (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.

(Assignee)

Comment 24

15 years ago
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

15 years ago
I've been looking for JRE 1.5 and can't find it.  Is there
really a 1.5?  Even an alpha version?
(Assignee)

Comment 26

15 years ago
not external access yet.
(Assignee)

Comment 27

15 years ago
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. 
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....
(Assignee)

Comment 29

15 years ago
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.
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 ;)

(Assignee)

Comment 31

15 years ago
Created attachment 132391 [details] [diff] [review]
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.
(Assignee)

Comment 32

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

sr/r?

Thanks a lot!
Attachment #132391 - Flags: superreview?(shaver)
Attachment #132391 - Flags: review?(Xiaobin.Lu)
(Assignee)

Comment 33

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

sorry, my mistake
Attachment #132391 - Attachment is obsolete: true
(Assignee)

Comment 34

15 years ago
Created attachment 132392 [details] [diff] [review]
This is the latest patch. change version check and #ifdef OJI

description see: Comment #31
(Assignee)

Comment 35

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

sr/r please.

Thanks a lot!
Attachment #132392 - Flags: superreview?(shaver)
Attachment #132392 - Flags: review?(Xiaobin.Lu)
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.
Attachment #132392 - Flags: superreview?(shaver) → superreview+
(Assignee)

Comment 37

15 years ago
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

15 years ago
Looks good to me.

Comment 39

15 years ago
Xiaobin --

Does that mean review+?

-M

Comment 40

15 years ago
Yes, should I update it somewhere?

Updated

15 years ago
Attachment #132392 - Flags: review?(Xiaobin.Lu) → review+
(Assignee)

Comment 41

15 years ago
checked in -> fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 42

15 years ago
Using the Blackdown java plugin, java applets still cause the browser to exit
in the same manner.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 43

15 years ago
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

15 years ago
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

15 years ago
Created attachment 132802 [details] [diff] [review]
add check for Blackdown java plugin

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

Updated

15 years ago
Attachment #132802 - Flags: review?(blizzard)
Comment on attachment 132802 [details] [diff] [review]
add check for Blackdown java plugin

Geez.
Attachment #132802 - Flags: review?(blizzard) → review+

Updated

15 years ago
Attachment #132802 - Flags: superreview?(shaver)
Comment on attachment 132802 [details] [diff] [review]
add check for Blackdown java plugin

Aye.
Attachment #132802 - Flags: superreview?(shaver) → superreview+

Comment 48

15 years ago
Blackdown JVM detection patch checked in.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 49

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

Thanks!

Comment 50

15 years ago
Since Blackdown is a third-party port/compile of the licensed Sun Java source
tree, I think that's a reasonable assumption.
*** Bug 222172 has been marked as a duplicate of this bug. ***

Comment 52

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

Updated

15 years ago
Attachment #131970 - Flags: review?(Xiaobin.Lu)

Updated

15 years ago
Attachment #132055 - Flags: review?(Xiaobin.Lu)

Updated

15 years ago
Attachment #132057 - Flags: review?(Xiaobin.Lu)

Updated

15 years ago
Attachment #132391 - Flags: superreview?(shaver)
Attachment #132391 - Flags: review?(Xiaobin.Lu)

Comment 53

15 years ago
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

15 years ago
Created attachment 137518 [details] [diff] [review]
... and again for the IBM plugin

Comment 55

15 years ago
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.

Updated

15 years ago
Attachment #137518 - Flags: superreview?(shaver)
Attachment #137518 - Flags: review?(blizzard)

Comment 56

15 years ago
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 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.
Attachment #137518 - Flags: superreview?(shaver) → superreview+
Comment on attachment 137518 [details] [diff] [review]
... and again for the IBM plugin

r=blizzard
Attachment #137518 - Flags: review?(blizzard) → review+

Updated

15 years ago
Component: General → Plug-ins
Product: Firebird → Browser
Version: unspecified → Trunk

Updated

15 years ago
Attachment #137518 - Flags: approval1.6?

Updated

15 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 59

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

a=leaf on behalf of drivers for 1.6
Attachment #137518 - Flags: approval1.6? → approval1.6+

Comment 60

15 years ago
Checked in.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.