248409 - LiveConnect should equate java and Packages.java, etc.

Status: RESOLVED FIXED

(Core Graveyard :: Java: Live Connect, defect)

Description

[Brendan Eich [:brendan]]

Same for sun and netscape.  See bug 225547.

/be

Comment 1

[Kyle Yuan]

mass reassign my bugs to Pete Zha.

Comment 2

[Pete Zha]

mass reassign to Alfred

Comment 3

[Alfred Peng]

Attachment: Patch v1

I don't know how the patch will impact the other part of Javascript Engine. It's just a experimental change and make them equal except netscape(https://bugzilla.mozilla.org/show_bug.cgi?id=225547#c7).

Brendan, how is the comparison of Object handled in Javascript engine? It seems that the "false" result is related to this.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 252303 [details] [diff] [review]
Patch v1

Don't patch the JS engine, changing ECMA-defined == semantics.

This bug is about the lack of a global "java" property with the same value as "Packages.java", etc. for "sun" and "netscape". It should be fixed using the DOM's ability to lazily resolve properties of the global object. Talk to jst on IRC about it.

/be

Comment 5

[Alfred Peng]

Attachment: Patch v2

After talking with jst, he suggested two ways to go:
1. Expose the same JSObject as both java and Packages.java.
2. Implement the "equality" hook of JSExtendedClass and return true if the JSObjects represent the same Java package.

This patch goes with the second suggestion. With that applied, java == Packages.java returns true, but not java === Packages.java.

Comment 6

[Brendan Eich [:brendan]]

The first suggestion is what Netscape 4 did, and it ought to be even smaller as a patch -- is it hard to do for some reason? It's the compatible fix.

/be

Comment 7

[Alfred Peng]

Brendan, as for the current implementation of Java Package, some information will be stored in the private slot of JSObject. I'm not sure how we can share the same private slot between java and Packages.java.

Comment 8

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

(In reply to comment #7)
> Brendan, as for the current implementation of Java Package, some information
> will be stored in the private slot of JSObject. I'm not sure how we can share
> the same private slot between java and Packages.java.

Make them return the same object, and they'll share the same everything.

You'd be seeing the equivalent of this script:

java = Packages.java;

or more verbosely:

var global = { get java() { return theJavaPackageObject() },
               Packages: { get java() { return theJavaPackageObject() } };

Comment 9

[Brendan Eich [:brendan]]

Yes, two properties with the same value (which for objects is a reference to the same object).

/be

Comment 10

[Alfred Peng]

Attachment: Patch v3

I thought the wrong way for the private slot sharing. Thanks for the explanation.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 252739 [details] [diff] [review]
Patch v3

>-    package_obj = JS_DefineObject(cx, parent_obj, obj_name, &JavaPackage_class, 0, JSPROP_PERMANENT | access);
>+    /*
>+     * Expose the same JSObject for Packages.java and java, Ditto for sun and
>+     * netscape.
>+     * See bugzilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=248409.
>+     */
>+    if (!strcmp(obj_name, path) && (obj = JS_GetParent(cx, parent_obj)) &&
>+        JS_LookupProperty(cx, obj, obj_name, &v) && JSVAL_IS_OBJECT(v)) {

Last term should be !JSVAL_IS_PRIMITIVE(v) to avoid JSVAL_NULL.

One term per line is easier to read, IMHO.

>+        package_obj = JSVAL_TO_OBJECT(v);
>+        JS_DefineProperty(cx, parent_obj, obj_name, package_obj, NULL, NULL,
>+                          JSPROP_PERMANENT | access);

Error-check with null return on error here?

>+        return package_obj;
>+    }
>+
>+    package_obj = JS_DefineObject(cx, parent_obj, obj_name, &JavaPackage_class,
>+                                  0, JSPROP_PERMANENT | access);
>     
>     if (!package_obj)
>         return NULL;

Looks like null return on error is right.

/be

Comment 12

[Alfred Peng]

Attachment: Patch v4

Comments addressed. JSVAL_IS_PRIMITIVE is great:)

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 252753 [details] [diff] [review]
Patch v4


>-    package_obj = JS_DefineObject(cx, parent_obj, obj_name, &JavaPackage_class, 0, JSPROP_PERMANENT | access);
>+    /*
>+     * Expose the same JSObject for Packages.java and java, Ditto for sun and
>+     * netscape.
>+     * See bugzilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=248409.
>+     */
>+    if (!strcmp(obj_name, path) &&
>+        (obj = JS_GetParent(cx, parent_obj)) &&
>+        JS_LookupProperty(cx, obj, obj_name, &v) &&
>+        !JSVAL_IS_PRIMITIVE(v))
>+    {
>+        package_obj = JSVAL_TO_OBJECT(v);
>+        if (JS_DefineProperty(cx, parent_obj, obj_name, package_obj, NULL, NULL,
>+                              JSPROP_PERMANENT | access)) {
>+            return package_obj;
>+        } else
>+            return NULL;

1. return null if (!JS_DefineProperty(...)).

2. Is the return package_obj on success in this case correct, or do you want to fall into the next code? I'm not seeing how this patch causes Packages.java and java to be defined at once. I would expect to see two JS_Define* API calls, with different second (obj) parameters...

/be

>+    }
>+
>+    package_obj = JS_DefineObject(cx, parent_obj, obj_name, &JavaPackage_class,
>+                                  0, JSPROP_PERMANENT | access);
>     
>     if (!package_obj)
>         return NULL;
>     
>     /* Attach private, native data to the JS object */
>     package = (JavaPackage_Private *)JS_malloc(cx, sizeof(JavaPackage_Private));
>     if (!package) {
>         JS_DeleteProperty(cx, parent_obj, obj_name);

Comment 14

[Alfred Peng]

(In reply to comment #13)
> 2. Is the return package_obj on success in this case correct, or do you want to
> fall into the next code? I'm not seeing how this patch causes Packages.java and
> java to be defined at once. I would expect to see two JS_Define* API calls,
> with different second (obj) parameters...
>

This patch doesn't define Packages.java and java at one time. They actually fail into different calls of define_JavaPackage. And the second (obj) parameters are different.

"java" will be defined during the java package initialization stage. Packages.java will be lazily resolved by JavaPackage_resolve(ditto for sun and netscape). For example, if Packages.java is going to be resolved, it will get to define_JavaPackage and the corresponding object is found by JS_LookupProperty. The next step is to define this object in Packages object chain with JS_DefineProperty. This object is the same one as the "java" object.

Comment 15

[Alfred Peng]

Attachment: Patch v5

This patch addresses the first problem of the comment 13 and passes the correct fourth parameter to JS_DefineProperty.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 255346 [details] [diff] [review]
Patch v5

Oh, I see. Thanks for pointing out what should be obvious from the comments, but for some reason was not to me.

/be

Comment 17

[Alfred Peng]

Attachment: Patch v6

Add some part of comment 14 to the patch. It's good to make it clear.

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 255423 [details] [diff] [review]
Patch v6

sr=jst

Comment 19

[Alfred Peng]

Checking in js/src/liveconnect/jsj_JavaPackage.c;
/cvsroot/mozilla/js/src/liveconnect/jsj_JavaPackage.c,v  <--  jsj_JavaPackage.c
new revision: 1.25; previous revision: 1.24
done

=> FIXED