Closed Bug 393928 Opened 12 years ago Closed 12 years ago

Let Java applets be scriptable through NPRuntime as well. (make OJI obsolete)

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Assigned: jst)

References

Details

Attachments

(3 files)

Future Java plugins are likely to be scriptable using NPRuntime rather than going through OJI/LiveConnect etc. We should stop relying on that already and let applets have a chance to be scriptable through other scripting mechanisms as well.

This bug is a placeholder for now, actual work on this may happen here, or in dependent bugs, depending on the scope of the problem.
Flags: blocking1.9+
And of course once that's a reality, LiveConnect and OJI will no longer be part of Mozilla products.
Summary: Let applets be scriptable through NPRuntime as well. → Let Java applets be scriptable through NPRuntime as well. (make OJI obsolete)
Depends on: 394086
Attached patch Proposed fix.Splinter Review
This patch does a couple of things, and I think it does all we need to do to support a Java plugin that doesn't need LiveConnect. What it does is this:

- Make Mozilla recognize that a Java plugin that supports NPRuntime scripting is installed. This is done by the Java plugin advertising a new hidden mimetype "application/x-java-vm-npruntime", and this mimetype must be at the end of the mimetype list as once we see that mimetype, we stop scanning. This is a temporary hack if you will that can be removed once support for LiveConnect is removed as well.

- Lazily initializes LiveConnect on window.java etc property resolves, and that way only does the initialization if a java plugin that supports NPRuntime is *not* found.

- Make nsGlobalWindow hold on to a new dummy java plugin instance owner (for lack of a better name) that's used solely to expose window.java etc if the Java plugin supports NPRuntime scripting.

- Implement a new NPRuntime API for exposing constructors through NPRuntime.

Biesi, would you be able to help review this? If not all the JS stuff, at least the plugin side of things, and the new plugin instance owner in nsGlobalWindow etc? Let me know...
Attachment #282338 - Flags: review?(cbiesinger)
Biesi: ping?
Comment on attachment 282338 [details] [diff] [review]
Proposed fix.

a/dom/src/base/nsDOMClassInfo.cpp

+        if (!::JS_HasProperty(cx, obj, ::JS_GetStringBytes(str), &hasProp)) {
+          return NS_ERROR_FAILURE;
+        }
...
+        isResolvingJavaProperties = PR_FALSE;

Shouldn't you set isResolvingJavaProperties to false before that early return?

a/dom/src/base/nsDOMClassInfo.h
+  static jsval sJava_id;
+  static jsval sPackages_id;
+#ifdef OJI

those two jsvals should also be in the ifdef, right?

b/dom/src/base/nsGlobalWindow.cpp
+  if (dummyPlugin) {
+    // Init plugin, tell plugin code to init the properties etc.
+
+    return;

I don't really understand the comment here? Is some code missing?

b/modules/plugin/base/public/nsIPluginInstanceInternal.h
This should get a new IID

b/modules/plugin/base/public/nsPIPluginHost.idl
so should this

b/modules/plugin/base/src/nsJSNPRuntime.cpp
-    // manipulating, and make it own any JSObject wrappers created
+    // manipulating, and make it owns any JSObject wrappers created

Wasn't the old version correct?

     // obj is a callable object that is being called, no method name
     // available then. Invoke the default method.
 
Shouldn't you move this comment to the invokeDefault call you're adding, and
remove the old one below this comment?

+NPObjWrapper_Convert(JSContext *cx, JSObject *obj, JSType type, jsval *vp)
+{
+  // Do nothing.

Please also document why it exists

modules/plugin/base/src/nsPluginHostImpl.cpp
-NS_IMETHODIMP nsPluginHostImpl::TrySetUpPluginInstance(const char *aMimeType,
-                                                       nsIURI *aURL,
-                                                       nsIPluginInstanceOwner *aOwner)
+NS_IMETHODIMP
+nsPluginHostImpl::TrySetUpPluginInstance(const char *aMimeType,

That file seems to be pretty inconsistent about whether NS_IMETHODIMP is on
its own line :/

+      if (tag->mIsNPRuntimeEnabledJavaPlugin) {

Wouldn't it be easier to just change the for loop above to:
  for (int i=0; i<tag->mVariants + tag->mIsNPRuntimeEnabledJavaPlugin; i++) {

or is there a difference in the two fprintfs that I'm missing?

--
I see nothing that ensures that ensures that we use the npruntime java plugin
if multiple java plugins exist, is that intentional?
Attachment #282338 - Flags: review?(cbiesinger) → review+
(In reply to comment #4)
> +      if (tag->mIsNPRuntimeEnabledJavaPlugin) {
> 
> Wouldn't it be easier to just change the for loop above to:
>   for (int i=0; i<tag->mVariants + tag->mIsNPRuntimeEnabledJavaPlugin; i++) {
> 
> or is there a difference in the two fprintfs that I'm missing?

Yes, there's a difference. The mimetype array in the tag doesn't contain the mimetype that the code writes out here (it's a internal magic mimetype that we don't store or expose in any way except in this boolean member), so we can't write it out in the loop that writes out the supported mimetypes.

> I see nothing that ensures that ensures that we use the npruntime java plugin
> if multiple java plugins exist, is that intentional?

That's up to the Java plugin installer to ensure. I've talked to the Sun engineers about this and they're fine with the installer taking on that responsibility.

I fixed the rest (good catches!), and let me know if you disagree with the above...
(In reply to comment #5)
> Yes, there's a difference. The mimetype array in the tag doesn't contain the
> mimetype that the code writes out here (it's a internal magic mimetype that we
> don't store or expose in any way except in this boolean member), so we can't
> write it out in the loop that writes out the supported mimetypes.

But looking at the nsPluginTag constructors, the type exists, it's just not included in the count. Hm... I guess the copy constructor wouldn't copy it, so writing it out separately is better. ok.

> That's up to the Java plugin installer to ensure. I've talked to the Sun
> engineers about this and they're fine with the installer taking on that
> responsibility.

Well, you can have an old version of Java installed at the same time as newer ones. Would we pick the latest version?
Attached patch Updated fix.Splinter Review
Boris, I know you're busy, but you're really the best guy to look at this. Any chance at all you can sr this before the beta? If not, please just let me know and I'll bug someone else!
Attachment #282795 - Flags: superreview?(bzbarsky)
oh, I forgot to mention this:
+#ifdef OJI
+    if (id == sJava_id || id == sPackages_id
+        || id == sNetscape_id || id == sSun_id || id == sJavaObject_id ||
+        id == sJavaClass_id || id == sJavaArray_id || id == sJavaMember_id
+        ) {

Would be good to consistently place the || at either the end of the line or the beginning of the next one
Yes, totally. Leftover stuff from earlier versions where the #ifdef was in the middle of that if check. Thanks.
I'll try to get to this ASAP, but it might take a few days.... :(
Ok, thanks bz.
Comment on attachment 282795 [details] [diff] [review]
Updated fix.

>+++ b/dom/src/base/nsDOMClassInfo.cpp
>+        PRBool oldVal = sDoSecurityCheckInAddProperty;

So... with the XOW stuff, all AddProperty checks now is that id != sLocation_id.  As a followup bug, can we just leave sDoSecurityCheckInAddProperty true whenever we're not actually resolving .location, and reverse the order of tests in AddProperty?  This code could just go away, then.

>+        if (hasProp) {
>+          *objp = obj;

You need a return somewhere around here.  Otherwise you'll call nsEventReceiverSH::NewResolve and it might clobber the prop you just resolved, right?

>+++ b/dom/src/base/nsGlobalWindow.cpp

>+#include "nsILiveConnectManager.h"

Why not put it up near the nsIJVMManager include above, so they're in the same ifdef?

>+class nsDummyJavaPluginOwner : public nsIPluginInstanceOwner

So this is built unconditionally, even if OJI is not defined.  Is there a reason?

>+  NS_DECL_ISUPPORTS

This should be participating in cycle collection, esp. since it adds the arcs window -> nsDummyJavaPluginOwner -> document

The window should traverse this pointer, at least.  Not sure how best to handle unlink.

>+nsGlobalWindow::InitJavaProperties()

>+  // Check if the plugin supports NPRuntime, if so, init through it,

s/if/whether/

>+++ b/modules/plugin/base/public/nsPIPluginHost.idl

>+  [noscript] void instantiateDummyJavaPlugin(in nsIPluginInstanceOwner aOwner);

Please document that if there is an NPRuntime plugin, this will make sure that aOwner's instance is non-null, and otherwise it'll make sure it's null?  Or should this document that the argument must have a null instance (and assert accordingly)?

>+++ b/modules/plugin/base/src/ns4xPluginInstance.cpp

>+ns4xPluginInstance::DefineJavaProperties()
>+  // The dummy Java plugins scriptable object is what we want to

s/plugins/plugin's/

>+++ b/modules/plugin/base/src/nsJSNPRuntime.cpp
>+NPObjWrapper_Convert(JSContext *cx, JSObject *obj, JSType type, jsval *vp)

We don't need to set *vp to anything here?

sr=bzbarsky with the nits picked and cycle collection added.
Attachment #282795 - Flags: superreview?(bzbarsky) → superreview+
I'd have nsDummyJavaPluginOwner traverse mInstance too.  It doesn't participate in cycle collection so far, but that could change any time....
Depends on: 399406
This was checked in on 2007-10-09 18:24. Marking FIXED as I believe we're done with what's needed to let a NPRuntime enabled Java plugin work with Firefox 3 now.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 401053
Depends on: 638171
You need to log in before you can comment on or make changes to this bug.