Some String properties of a Java object are read as 'null' by Javascript

RESOLVED FIXED

Status

Core Graveyard
Java: OJI
--
major
RESOLVED FIXED
15 years ago
8 years ago

People

(Reporter: patrick samson, Assigned: Kyle Yuan)

Tracking

Trunk
x86
Windows NT

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3) Gecko/20030312
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3) Gecko/20030312

Javascript calls an applet which returns a Java object. In its simpliest form
this object has more than 8 properties of String type.
Javascript reads the properties at positions 8, 9 and  10 as null. Any other
position is correct.

Context:
Mozilla 1.3
Java Plug-in 1.4.1_02


Reproducible: Always

Steps to Reproduce:
Test case:
Note: the appletReady mecanism in the following code is not related to the
current bug, but to this one:
http://developer.java.sun.com/developer/bugParade/bugs/4837333.html.
MissingProperties.html
----------------------
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<HTML>
  <HEAD>
   <TITLE>Test</TITLE>
   <SCRIPT>
// wait for the applet to be loaded before calling it.
var appletReady = false;
function doOnLoad() {
 if (appletReady) test()
 else setTimeout("doOnLoad()",100)
}
function test() {
// get the info from the applet
 var info = document.theApplet.getInfo();
// browse the Java object to confirm its correctness.
 var f = info.getClass().getFields()
 var lg = f.length
 var s = ""
 for (var i = 0; i < lg; i++)
  s += "(" + f[i].toString() + ") " + f[i].getName() + "=" + f[i].get(info) + "\n"
 alert(s) // is correct
// some JS accesses are incorrect
 alert(info.s8) // should be "s8" but is empty!
 // shown as null by the debugger
 // same for s9 & s10.
 // s0..s7 and s11.. are correct.
}
   </SCRIPT>
  </HEAD>
  <BODY onload="doOnLoad()">
<APPLET code="MissingProperties.class" width="1" height="1" name="theApplet"
MAYSCRIPT>
</APPLET>
  Test
  </BODY>
</HTML>

MissingProperties.java
----------------------
public class MissingProperties extends java.applet.Applet {

public class Info {
 public String s0 = "s0";
 public String s1 = "s1";
 public String s2 = "s2";
 public String s3 = "s3";
 public String s4 = "s4";
 public String s5 = "s5";
 public String s6 = "s6";
 public String s7 = "s7";
 public String s8 = "s8";
 public String s9 = "s9";
 public String s10 = "s10";
 public String s11 = "s11";
 public String s12 = "s12";
 public String s13 = "s13";
}

public Info getInfo() {
    return new Info();
}
public void start() {
 super.start();

 // insert any code to be run when the applet starts here
 netscape.javascript.JSObject.getWindow(this).eval("appletReady = true;");
}
}

Actual Results:  
Some properties are empty (null).

Expected Results:  
Show the actual content of properties.

Comment 1

15 years ago
Confirming bug on WinNT4.0 using Java Plug-in 1.4.1;
using Mozilla trunk build 2003052304.

Properties |info.s8|, |info.s9|, |info.s10| evaluate to |null|.
All the others evaluate correctly; e.g. |info.s11| === 's11'.

As Patrick says, one can see this directly while stopped
in the Mozilla JavaScript Debugger (Local Variables view);
or via debugging code such as this added to the HTML file:


function debug(info)
{
  var LF = '<br>';
  var msg = '';

  for (var i=0; i<14; i++)
  {
    var prop = 's' + i;
    msg += prop + ' = ' + info[prop] + LF;
  }

  with (document)
  {
    open();
    write(msg);
    close();
  }
}


Reassigning to OJI component, and cc'ing Patrick -
Assignee: rogerl → joshua.xia
Status: UNCONFIRMED → NEW
Component: Live Connect → OJI
Ever confirmed: true
QA Contact: pschwartau → dsirnapalli

Comment 2

15 years ago
It happened on my machine, very interesting because all string should be same
action.
Status: NEW → ASSIGNED

Comment 3

15 years ago
This bug crash mozilla on Unix.

The reason is that the fieldID Hashtable (theIDTable) uses no unique key,
some of two series of fieldIDs from two classes is same! (in oji/src/ProxyJNI.cpp).

Should we add theCLASSTable? or create a complex key base on class id + field id?

Comment 4

15 years ago
This patch works!

Index: src/ProxyJNI.cpp
===================================================================
RCS file: /cvsroot/mozilla/modules/oji/src/ProxyJNI.cpp,v
retrieving revision 1.27
diff -u -r1.27 ProxyJNI.cpp
--- src/ProxyJNI.cpp    25 Feb 2003 03:39:31 -0000      1.27
+++ src/ProxyJNI.cpp    19 Jun 2003 10:28:58 -0000
@@ -725,7 +725,7 @@
         jfieldID outFieldID = NULL;
         nsresult result = secureEnv->GetFieldID(clazz, name, sig, &outFieldID);

         if (result == NS_OK && outFieldID != NULL) {
-            nsVoidKey key(outFieldID);
+            nsVoidKey key((jfieldID)((PRInt32)clazz + (PRInt32)outFieldID));
             JNIField* field = (JNIField*) theIDTable->Get(&key);
             if (field == NULL) {
                 field = new JNIField(name, sig, outFieldID);
Sorry, you can't assume the sum of two 32-bit numbers is unique.

Are field ids supposed to be unique among all classes, or only within a class? 
Is there a spec that should govern here?

If field ids are unique only within a given class, then you need a pair (class
id, field id) as the key, and you can't fit that in a void * (nsVoidKey).

/be
> Sorry, you can't assume the sum of two 32-bit numbers is unique.

... when stored in a void * (which may be 32 bits), I should have written.

/be

Comment 7

15 years ago
Yes, field ids are unique only within a given class, how to use (class
id, field id) as the key? 

Can this used to be a key? key = ((PRInt64)clazz) << 32 | field 
If you need to use PRInt64 or PRUint64, you should use the prlong.h LL_* macros,
for portability to systems that lack native long long support.  (There may not
be any such systems currently running Mozilla -- timeless?)

Please read http://www.mozilla.org/projects/xpcom/hashtable-guide.html and see
whether there is a better hash table package to use for theIDTable.

/be
Note that Mozilla also has a very nice wrapper for PRUint64, namely nsInt64,
which might be useful.

Comment 10

15 years ago
Created attachment 126738 [details] [diff] [review]
nsVoidKey -> nsIDKey

use nsID { m0 = clazz, m1 = HIWORD of fieldID, m2 = LOWORD of fieldID, m3 = 0}
as nsID
and use nsIDKey(nsID) as IDKey.

I think this is the best way because it seems that nsVoidKey save only 32bit
pointer.
Why not use nsDataHashtable.h instead?  Using nsIDKey is a kludge, because you
really don't need 128-bit keys, and the struct layout is not what you want, either.

/be

Comment 12

15 years ago
Hi Brendan,

You means use nsDataHashtable instead of nsHashTable, 
and use nsIDKey?

sorry, nsID clazzFieldID should be  {(PRInt32)clazz, ((PRInt32)outMethodID) >>
16, ((PRInt32)outMethodID) & 0x0000FFFF};
>You means use nsDataHashtable instead of nsHashTable,

Yes.

>and use nsIDKey?

No -- use a key you declare specifically for the job here:

struct ClassFieldKey {
    jclass      clazz;
    jfieldID    fieldID;
};

/be

Comment 14

15 years ago
good idea, I am making patch!
Thank!

Comment 15

15 years ago
it seems that nsDataHashtable can't use struct as a key,
trying nsTHashtable.

Comment 16

15 years ago
->kyle
Assignee: joshua.xia → kyle.yuan
Status: ASSIGNED → NEW

Comment 17

15 years ago
Kyle,

You can try to fix this bug as a Liveconnect bug fix demo.
Status: NEW → ASSIGNED
(Assignee)

Comment 18

15 years ago
Created attachment 137222 [details] [diff] [review]
new patch using nsDataHashtable
(Assignee)

Updated

15 years ago
Attachment #137222 - Flags: review?(joshua.xia)

Comment 19

15 years ago
Comment on attachment 137222 [details] [diff] [review]
new patch using nsDataHashtable

>+  PRBool KeyEquals(KeyTypePointer aKey) const { return aKey->clazz == mValue.clazz && aKey->memberID == mValue.memberID; }
>+
>+  static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; }
>+  static PLDHashNumber HashKey(KeyTypePointer aKey) { return NS_PTR_TO_INT32(aKey->memberID); }

You check key equality based on both members but you only hash one? That seems
like a recipe for hash conflicts. Try this:
  {
    PRUint32* v1 = NS_PTR_TO_INT32(aKey->memberID);
    PRUint32* v2 = NS_PTR_TO_INT32(aKey->clazz);
    return (v1>>28) ^ (v1<<4) ^ v2;
  }

>+    if (theIDTable == NULL) {
>+        theIDTable = new nsDataHashtable<JavaClassMemberKey, void*>;
>+        theIDTable->Init(16);
>+    }

Always check the return value of the Init() function... it can fail on OOM
conditions.

Otherwise, this looks good to me.
Regarding

  {
    PRUint32* v1 = NS_PTR_TO_INT32(aKey->memberID);
    PRUint32* v2 = NS_PTR_TO_INT32(aKey->clazz);
    return (v1>>28) ^ (v1<<4) ^ v2;
  }

PRUint32* v1 should be PRUint32 v1, ditto for v2, of course -- just wanted to
point that out to save confusion.

I don't see a point in rotating v1 right by 4, though.  Simply xor'ing the two
should be good enough, and if memberID has well-distributed low bits, you don't
want to rotate v1 at all.

/be
(Assignee)

Comment 21

15 years ago
Created attachment 137276 [details] [diff] [review]
new patch addressed review comments, also add some null pointer checking

Since both clazz and memberID are the pointer to some JNI internal struct, I'd
like to buy Brendan's simply xor'ing two PRUint32.
Attachment #126738 - Attachment is obsolete: true
Attachment #137222 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #137276 - Flags: superreview?(brendan)
Attachment #137276 - Flags: review?(bsmedberg)
(Assignee)

Updated

15 years ago
Attachment #137222 - Flags: review?(joshua.xia)

Comment 22

15 years ago
Comment on attachment 137276 [details] [diff] [review]
new patch addressed review comments, also add some null pointer checking

r=bsmedberg  really, the ProxyJNIEnv class should have an Init() member instead
of doing the init in the constructor and failing miserably in OOM conditions,
but that problem already exists in the current code.
Attachment #137276 - Flags: review?(bsmedberg) → review+
Comment on attachment 137276 [details] [diff] [review]
new patch addressed review comments, also add some null pointer checking

How about a followup bug to add an Init method and move fallible code from the
ctor to there, checking its return value?  Thanks,

/be
Attachment #137276 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 24

15 years ago
new bug 228520 filed for the Init issue.

nominate for 1.6. this is a crash bug on *nix, and the fix won't introduce any
risk - it just re-writes the some logic in a better manner.
Flags: blocking1.6?
(Assignee)

Comment 25

15 years ago
Comment on attachment 137276 [details] [diff] [review]
new patch addressed review comments, also add some null pointer checking

oops, using the wrong tag. patch description is in my previous comment.
Attachment #137276 - Flags: approval1.6?

Comment 26

15 years ago
Comment on attachment 137276 [details] [diff] [review]
new patch addressed review comments, also add some null pointer checking

a=asa (on behalf of drivers) for checkin to Mozilla 1.6
Attachment #137276 - Flags: approval1.6? → approval1.6+
(Assignee)

Comment 27

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

Updated

15 years ago
Flags: blocking1.6?

Comment 28

15 years ago
This commit have added a "may be used uninitialized" warning on brad TBox:

+modules/oji/src/ProxyJNI.cpp:956
+ `JNIField*field' might be used uninitialized in this function
Comment on attachment 137276 [details] [diff] [review]
new patch addressed review comments, also add some null pointer checking

Argh, I skimmed too fast when reviewing, so did bsmedberg.  That new warning
indicates a fatal flaw:

>@@ -910,11 +952,13 @@
>         jfieldID outFieldID = NULL;
>         nsresult result = secureEnv->GetStaticFieldID(clazz, name, sig, &outFieldID);
>         if (result == NS_OK && outFieldID != NULL) {
>-            nsVoidKey key(outFieldID);
>-            JNIField* field = (JNIField*) theIDTable->Get(&key);
>-            if (field == NULL) {
>+            JavaClassMember key(clazz, outFieldID);
>+            JNIField* field;
>+            PRBool bFound = theIDTable && theIDTable->Get(key, (void **)field);

The actual argument passed for the final out param should be (void **)&field,
not (void **)field, of course.

I'm fixing this right now.  Sorry I missed it in review.  Kyle, please take
note.

/be
Thanks to Aleksey for his usual eagle-eyed report on new "may be used
uninitialized" warnings.

/be
(Assignee)

Comment 31

15 years ago
I'm sorry for the typo. Thanks to Aleksey & Brendan.

Updated

8 years ago
Component: Java: OJI → Java: OJI
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.