Closed Bug 208121 Opened 21 years ago Closed 21 years ago

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

Categories

(Core Graveyard :: Java: OJI, defect)

Product:
Core Graveyard
Component:
Java: OJI
Platform:
x86
Windows NT
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: p_samson, Assigned: yuanyi21)

Details

Attachments

(1 file, 2 obsolete files)

new patch addressed review comments, also add some null pointer checking
6.17 KB, patch
benjamin
: review+
brendan
: superreview+
asa
: approval1.6+
Details | Diff | Splinter Review 
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.
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
It happened on my machine, very interesting because all string should be same
action.
Status: NEW → ASSIGNED
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?
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
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.
Attached patch nsVoidKey -> nsIDKey (obsolete) — — Splinter Review 
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
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
good idea, I am making patch!
Thank!
it seems that nsDataHashtable can't use struct as a key,
trying nsTHashtable.
->kyle
Assignee: joshua.xia → kyle.yuan
Status: ASSIGNED → NEW
Kyle,

You can try to fix this bug as a Liveconnect bug fix demo.
Status: NEW → ASSIGNED
Attached patch new patch using nsDataHashtable (obsolete) — — Splinter Review 

    
  

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

    
    

    
  
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

    
    

    
  


  
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

    
  

        Attachment #137276 -
        Flags: superreview?(brendan)

        Attachment #137276 -
        Flags: review?(bsmedberg)

    
  

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

    
    

    
  
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+

    
    

    
  
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?

    
    

    
  
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 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+

    
    

    
  
checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED

    
  
Flags: blocking1.6?

    
    

    
  
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

    
    

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

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: