224649 - javascript tries to set property of java object fails

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Xiaobin Lu]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; .NET CLR 1.1.4322)
Build Identifier: Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007

Javascript can't set public field of java object.

I attached the java file and a html page. After further debugging into the 
issue, I think this bug should belong to javascript engine instead of 
liveconnect.

Here is my analysis:
 If I write something like below in javascript:
       docuement.appletObject.TEST_MESSAGE = 20;

  Javascript engine will call js_SetProperty (See js\src\jsobj.c), here is the 
code:
        if (!js_LookupProperty(cx, obj, id, &pobj, (JSProperty **)&sprop))
        return JS_FALSE;

    if (sprop && !OBJ_IS_NATIVE(pobj)) {
        OBJ_DROP_PROPERTY(cx, pobj, (JSProperty *)sprop);
        sprop = NULL;
    }

    The js_LookupProperty will succeed, however, since OBJ_IS_NATIVE returns 
false, it will execute OBJ_DROP_PROPERTY. OBJ_IS_NATIVE is a macro like below:
      #define MAP_IS_NATIVE
(map)                                                    \
    ((map)->ops == &js_ObjectOps ||                                           \
      ((map)->ops && (map)->ops->newObjectMap == js_ObjectOps.newObjectMap))  
#define OBJ_IS_NATIVE(obj)  MAP_IS_NATIVE((obj)->map)

 Since obj is a really a JavaObject(if you are familiar with liveconnect), so 
the map->ops won't be &js_ObjectOps or the newObjectMap won't be 
js_ObjectOps.newObjectMap. So the OBJ_IS_NATIVE will return false. This case 
the property be dropped from the object, I guess.

   I want to know whether javascript here does the right thing. Could some 
javascript guru evaluate this and tell us is this a liveconnect bug or 
javascript problem?

    






Reproducible: Always

Steps to Reproduce:
1. Open Mozilla
2. Visit the Test.html

Actual Results:  
The second dialog box shows "Value of MyApplet.getTestMessage():Test"

Expected Results:  
The second dialog box shows "Value of MyApplet.getTestMessage():20"

This works for Netscape 4.7 and IE.

Comment 1

[Xiaobin Lu]

Attachment: Testcase

The testcase contains a Java class/source file & a html page.

Comment 2

[Phil Schwartau]

cc'ing Brendan, Mike on this question -

Comment 3

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

OBJ_DROP_PROPERTY doesn't remove properties from objects, it simply drops a
reference used for controlling the lifetime of the JSProperty in question.  Do
you see the liveconnect property-setter being called, for your test in question?

Comment 4

[Brendan Eich [:brendan]]

Sorry, the JS engine code is correct.  Can you say what is going wrong in more
detail?

/be

Comment 5

[Xiaobin Lu]

The setter of the liveconnect object(jsj_JavaObject::setPropertybyId) is not 
getting called.

Let me try to clarify.
 
In Javascript, I wrote something like: "document.myApplet.TEST_MESSAGE = 20", 
this will call into jsobj.c:js_SetProperty. Here is the code from lxr:
          
     if (!js_LookupProperty(cx, obj, id, &pobj, (JSProperty **)&sprop))
          return JS_FALSE;
 
     if (sprop && !OBJ_IS_NATIVE(pobj)) {
         OBJ_DROP_PROPERTY(cx, pobj, (JSProperty *)sprop);
         sprop = NULL;
     }
     
     attrs = JSPROP_ENUMERATE;
     flags = 0;
     shortid = 0;
     clasp = OBJ_GET_CLASS(cx, obj);
     getter = clasp->getProperty;
-->  setter = clasp->setProperty;
    
  js_LookupProperty will call the liveconnect lookupProperty correctly. I 
noticed that the setter has been set to "XPC_WN_Helper_SetProperty" during my 
debugging. And I think something maybe wrong here and finally cause the wrong 
method to get called. (Just my thought, should we try to get "proto" object 
and if proto is not native object, call "OBJ_SET_PROPERTY"? This is just my 
thought got from the "js_LookupProperty".) 

Comment 6

[Brendan Eich [:brendan]]

ECMA-262 Edition 3 section 8.6.2.2 says that js_SetProperty has to work that
way.  If the property does not exist in the object itself (only on a prototype,
whether native or host object -- step 3 in 8.6.2.2), then a new property is
created which "shadows" the proto-property (step 6).

Why was js_SetProperty even called here?  Shouldn't JavaObject_setPropertyById
have been called instead, if you were setting the property of a JavaObject?

Note that js_SetProperty assumes OBJ_IS_NATIVE(obj).  If you were to implement a
non-native JSObjectOps, and point its setProperty hook at js_SetProperty, you
would create an invalid situation.

/be

Comment 7

[Brendan Eich [:brendan]]

Is the applet object a native (JSObjectOps) object, whose prototype is a Java
(JavaObject_ops) object?  That can't work, given how ECMA specifies [[Put]],
also known as JSObjectOps.setProperty, must work.

/be

Comment 8

[Brendan Eich [:brendan]]

Cc'ing DOM guys in case there's some bogus prototyping going on (see comment 7).

/be

Comment 9

[Xiaobin Lu]

When the browser tries to get the Java object, it creates a JSObject ( I think 
it is a xpc wrapped native embed node) whose prototype is the JSObject wrapped 
around the real applet object. The code is in nsDOMClassInfo.cpp:line 5727 (
nsHTMLExternalObjSH::PostCreate). That is why js_SetProperty is called instead 
of "JavaObject_setPropertyById". 

The problem is if we delegate the call to js_SetProperty of that xpc embed node, 
should we consider how to set the property in its prototype object? As Brendan 
commented (comment #6), if the logic inside js_SetProperty is right, then there 
must be something wrong in the xpconnect code. Since when we put the JavaObject 
as a prototype object in a xpc wrapped native embed node, we should consider how 
to set the property of the its prototype.
 

Comment 10

[Brendan Eich [:brendan]]

When did this regress?  The patch checked in for bug 12367 was different from
the only attachment in that bug, but I don't know why.  I know why the patch
checked in for that bug (rev 3.98 of js/src/jsobj.c) does what it does -- it
does that for ECMA conformance, per comment 6.  But the patch in 12367 does what
the code in nsHTMLExternalObjSH::PostCreate expects.

jst, do you remember any of this?

/be

Comment 11

[Brendan Eich [:brendan]]

Rev 3.98 of jsobj.c was checked in over two years ago, on 03-Sep-2001.

/be

Comment 12

[Xiaobin Lu]

Reassign to DOM guy.

Comment 13

[Phil Schwartau]

And the QA, too -

Comment 14

[Kyle Yuan]

Brendan, I'm curious why the patch for bug 12367
(http://bugzilla.mozilla.org/attachment.cgi?id=47921&action=view) is different
from the real code
(http://lxr.mozilla.org/seamonkey/source/js/src/jsobj.c#2704)? Using the
original patch, setting the field value of java object will be successful.

Comment 15

[Brendan Eich [:brendan]]

Kyle, I know -- that's what I said in comment 10, second sentence.  But I went
on to say that I don't know why the patch wasn't what was checked in.  And that
I do know that the checked-in change was necessary to preserve ECMA conformance,
per comment 6 first paragraph.

The patch attached to bug 12367 is not valid per ECMA-262.

The issue I raised in comment 11 (and comment 10) is this: why wasn't this
broken for the last two years?  Or was it broken, and no one noticed?

I'll try to find jst on IRC and figure out the real fix.

/be

Comment 16

[Kyle Yuan]

Brendan, first, sorry for not being aware that you already mentioned that;
second, I still think the original patch does not break ECMA conformance. 
  if (sprop && !OBJ_IS_NATIVE(pobj)) 
means the property was found, and it's not a native object, so we should go to
ECMA-262 Edition 3 section 8.6.2.2 step 4 - call the object's setProperty
method, rather than pretending the property was not found (set sprop = NULL).

Comment 17

[Kyle Yuan]

I think the reason, that no one noticed this issue before, is partly because of
the poor performance of js->java call. Even with the simplest test case, it will
take 3+ minutes to complete the first js->java call on my P4 1.8GHz windows box.

Comment 18

[Brendan Eich [:brendan]]

Re: comment 16: you are misreading the code and/or the ECMA spec.  The code in
js_SetProperty has a non-null sprop, but it comes from a non-native object pobj,
returned from the call to js_LookupProperty.  That means it comes from a
prototype object of obj, the input parameter to js_SetProperty that is a native
object (by definition -- otherwise you wouldn't have called js_SetProperty on obj).

Since pobj is a prototype, sprop is not "in" obj in the sense of the spec, and
ECMA-262 Edition 3 8.6.2.2 step 3 applies.

Re: comment 17: I cannot believe it takes 3 minutes to call from JS into Java on
such a fast machine.  What is going on?  Are a ton of classes being loaded? 
Whatever the problem, that performance problem is fatal to LiveConnect.  Is
there a bug on file to track that performance problem?

/be

Comment 19

[Xiaobin Lu]

The reason of liveconnect preformace lies in its agrresive reflection to the 
javaobject/javaclass. One of the main goals of JEA design is to resolve the 
performance problem of liveconnect. It utilizes the similar idea as Microsoft 
IDispatch mechanism to do the scripting.

Comment 20

[Brendan Eich [:brendan]]

Xiaobin, thanks for confirming that.  Isn't it the case that LiveConnect used to
perform better?  What changed?  I'm in favor of JEA (I hope you're looking at
the IDispatch support added to xpconnect by dbradley), but if we used to have
better LiveConnect performance with OJI, can't we do something in the short run
to recover that better performance?

/be

Comment 21

[Xiaobin Lu]

The reason of the bad performance of liveconnect is due to the current 
implementation of liveconnect module itself. We never have a better performance 
since then.

There is a way to get a good performance with the current implementation which 
will involves a lot change. I would rather persue the change in JEA. We are now 
busy with the implementation/prototyping. And I got it to work with 
JavaObject already and the performace is comparable to Netscape 4.x liveconnect 
implementation. 

The JEA implementation of liveconnect still uses the current hook between 
javascript engine and liveconnect (JSObjectOps), but the internal 
implementation uses the similar idea of IDispatch (e.g, getPropertybyID will 
call getIDOfName etc). I haven't got a chance to talk to dbradley to see how it 
can utilize the hook of xpconnect IDispatch engine. But I would rather to make 
the current thing work better and later on we can think how to improve it to 
utilize the xpconnect IDispatch stuff.

Comment 22

[Kyle Yuan]

I have some time to look into this problem again. 

As shown in the picture at the end of nsHTMLExternalObjSH::PostCreate(), every
applet object is wrapped by a xpc native node:

  // this.__proto__.__proto__.__proto__
  //   ^      ^         ^         ^
  //   |      |__ pi (plugin instance) wrapper, most likely wrapped
  //   |          by LiveConnect
  //   |__ xpc wrapped native embed node

Whenever we call the lookupProperty/getProperty/setProperty methods of an
applet, it always goes to js_LookupProperty/js_GetProperty/js_SetProperty
(through XPC_WN_NoCall_JSOps). 

An applet object not only has java properties, but also has DOM properties. In
js_LookupProperty, we try to find the native property at first, then over to
obj.proto if failed (http://lxr.mozilla.org/mozilla/source/js/src/jsobj.c#2481).
Same thing we did in js_GetProperty
(http://lxr.mozilla.org/mozilla/source/js/src/jsobj.c#2652). So we should *not*
assume that every obj goes into js_SetProperty is native.

Comment 23

[Kyle Yuan]

I noticed that the setProperty of java applet was already broken in mozilla 1.0
that is the earliest mozilla I can find.

Comment 24

[Brendan Eich [:brendan]]

> So we should *not* assume that every obj goes into js_SetProperty is native.

Yes, we should and do -- you mean "we should *not* assume that every native
object has a native prototype" I think -- but js_SetProperty handles that case
too, per ECMA-262 (it creates a shadowing property directly in the native
object, which hides or "shadows" the prototype's property of the same id).

So the way plugins are prototyped is in conflict with ECMA-262.  We need a way
to merge the DOM and applet objects without using prototyping.  One way to do it
is with a DOM object whose class hooks know how to get and set properties in the
applet, too.  jst, how hard would that be?

/be

Comment 25

[Kyle Yuan]

Modifying the way plugins are prototyped is a big change. Can we just take this
1-line patch as a short term workaround until we rewrite the whole plugins
prototyping things? This bug is a blocker of liveconnect funtionality.

RCS file: /cvsroot/mozilla/js/src/jsobj.c,v
retrieving revision 3.155
diff -p -u -r3.155 jsobj.c
--- jsobj.c     11 Feb 2004 07:21:59 -0000      3.155
+++ jsobj.c     30 Mar 2004 09:08:24 -0000
@@ -2707,7 +2707,7 @@ js_SetProperty(JSContext *cx, JSObject *
  
     if (sprop && !OBJ_IS_NATIVE(pobj)) {
         OBJ_DROP_PROPERTY(cx, pobj, (JSProperty *)sprop);
-        sprop = NULL;
+        return OBJ_SET_PROPERTY(cx, pobj, id, vp);
     }
  
     /*

Comment 26

[Brendan Eich [:brendan]]

Why should we violate the ECMA spec to fix LiveConnect?  That patch will have
bad effects for the usual case, where there is a N:1, not a 1:1, relationship
between the direct object (the plugin DOM object) and its prototype (the applet
object).  Then any setting of a property in a non-native prototype shared among
N direct objects will collide in the prototype, rather than shadow the
proto-property with a native, direct property of the same id.

Kyle, you make a good point that redoing the prototype chain now is a bigger fix
than needed.  A minimal fix in nsDOMClassInfo.cpp is not that hard; it should
avoid hacking directly into the JS engine in a code path not specific to this
LiveConnect special case.  I can't stress enough how important it is to make
hacks and workarounds (and even ultimate fixes, if they require special case
code) be as local to the situation they address as possible -- until you have
enough instances of the special case to generalize into a lower layer.

Attaching a first cut patch.  I'm not going to have time to push it through
testing and review, but if others can help do those tasks, I can approve it for 1.7.

/be

Comment 27

[Brendan Eich [:brendan]]

Attachment: proposed fix, specific to plugin/applet dom classinfo

Kyle, can you remove your jsobj.c patch and test this patch?  Thanks.

/be

Comment 28

[Johnny Stenback (:jst)]

Note that that patch needs an '\' at the end of the line:

+#define EXTERNAL_OBJ_SCRIPTABLE_FLAGS

Comment 29

[Johnny Stenback (:jst)]

Attachment: Add missing '\'.

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 145128 [details] [diff] [review]
Add missing '\'.

r+sr=jst

Comment 31

[Brendan Eich [:brendan]]

Let's get this tested and I'll get another driver to approve the patch for 1.7.

/be

Comment 32

[Kyle Yuan]

Comment on attachment 145128 [details] [diff] [review]
Add missing '\'.

I tested it. It works like a charm. Thanks for fixing this. Should we do the
equivalent to the GetProperty?

Comment 33

[Brendan Eich [:brendan]]

Comment on attachment 145128 [details] [diff] [review]
Add missing '\'.

Kyle: there's no need for extra work in the getProperty case, because the
shadowing problem doesn't exist for get -- only for set ([[Put]] in ECMA
terms).  You can get an id from an object o, and if it has a prototype p in
which id is defined, you'll find that property without creating a property in o
that hides the one in p.  Only if you are setting o[id] will you potentially
make a shadowing property, per ECMA.

Going for 1.7 approval so jst can do the checkin.

/be

Comment 34

[Kyle Yuan]

okay, thanks for the clear and throughout explanation.

Comment 35

[chris hofmann]

Comment on attachment 145128 [details] [diff] [review]
Add missing '\'.

a=chofmann for 1.7

Comment 36

[Johnny Stenback (:jst)]

Fix checked in. Thanks for the patch, brendan!