74206 - Potential discrepancies in some variable definitions

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P4)

Description

[av (gone)]

We have a whole bunch of variables that are used in GetValue and SetValue calls 
in both old and new plugin APIs some of them correspond to each other but have 
different values in the two worlds. I am filing this bug to trigger a discussion 
on whether it could have a negative impact and if it is not too late to fix if 
necessary.

In the old API we have the following (npapi.h):

typedef enum {
  NPPVpluginNameString = 1,    //1
  NPPVpluginDescriptionString, //2
  NPPVpluginWindowBool,        //3
  NPPVpluginTransparentBool,   //4
  NPPVjavaClass,               //5
  NPPVpluginWindowSize,        //5
  NPPVpluginTimerInterval      //6
} NPPVariable;

typedef enum {
  NPNVxDisplay = 1,          //1
  NPNVxtAppContext,          //2
  NPNVnetscapeWindow,        //3
  NPNVjavascriptEnabledBool, //4
  NPNVasdEnabledBool,        //5
  NPNVisOfflineBool          //6
} NPNVariable;

In the new API we have (nsplugindefs.h):

enum nsPluginVariable {
  nsPluginVariable_NameString                = 1,
  nsPluginVariable_DescriptionString         = 2
};

enum nsPluginManagerVariable {
  nsPluginManagerVariable_XDisplay           = 1,
  nsPluginManagerVariable_XtAppContext       = 2
};

enum nsPluginInstancePeerVariable {
  nsPluginInstancePeerVariable_NetscapeWindow  = 3
//  nsPluginInstancePeerVariable_JavaClass     = 5,
//  nsPluginInstancePeerVariable_TimerInterval = 7
};

enum nsPluginInstanceVariable {
  nsPluginInstanceVariable_WindowlessBool          = 3,
  nsPluginInstanceVariable_TransparentBool         = 4,
  nsPluginInstanceVariable_DoCacheBool             = 5,
  nsPluginInstanceVariable_CallSetWindowAfterDestroyBool = 6
};

Not only have some of them not matching values but to my understanding are 
placed under wrong type. I think more correct arrangement in the new API would 
be:

// the following two types are corresponding NPPVariable
enum nsPluginVariable {
  nsPluginVariable_NameString         = 1,
  nsPluginVariable_DescriptionString  = 2
+//  nsPluginVariable_JavaClass         = 5,
+//  nsPluginVariable_TimerInterval     = 7 not sure, maybe 
                                            nsPluginInstanceVariable?
};

enum nsPluginInstanceVariable {
  nsPluginInstanceVariable_WindowlessBool          = 3,
  nsPluginInstanceVariable_TransparentBool         = 4,
-  nsPluginInstanceVariable_DoCacheBool             = 5,
-  nsPluginInstanceVariable_CallSetWindowAfterDestroyBool = 6
+  nsPluginInstanceVariable_PluginWindowSize        = 6, 
+  nsPluginInstanceVariable_DoCacheBool                   = 8,
+  nsPluginInstanceVariable_CallSetWindowAfterDestroyBool = 9
};

// the following two types are corresponding NPNVariable
enum nsPluginManagerVariable {
  nsPluginManagerVariable_XDisplay           = 1,
  nsPluginManagerVariable_XtAppContext       = 2
  nsPluginManagerVariable_JSBool             = 4,
  nsPluginManagerVariable_asdBool            = 5,
  nsPluginManagerVariable_OfflineBool        = 6
};

enum nsPluginInstancePeerVariable {
  nsPluginInstancePeerVariable_NetscapeWindow  = 3
-//  nsPluginInstancePeerVariable_JavaClass     = 5,
-//  nsPluginInstancePeerVariable_TimerInterval = 7
};

My biggest concern if we are going to fix it is DoCache and 
CallSetWindowAfterDestroy.

Comment 1

[sean echevarria]

Changing the value of nsPluginInstanceVariable_DoCacheBool or 
nsPluginInstanceVariable_CallSetWindowAfterDestroyBool will definitely break 
the Beatnik Player xpcom plugin.  It's explicitly setting the value for those 2 
when my nsIPluginInstance::GetValue() method is called.

Comment 2

[av (gone)]

Did we already have those two in NS6 RTM? I just cannot immediately recall...

Comment 3

[sean echevarria]

Yes - these were added Sept 14 2000:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/modules/plugin/public/nsplugindefs.h

Comment 4

[drapeau]

av: Sean's got it; the changes were the result of a meeting amongst Netscape and
plugin vendors, to discuss changes in the plugin lifecycle.  One problem that I
know of is that the Sun Java Plug-In had problems with the existing lifecycle,
so we lobbied to change it.  We also lobbied to have a flag that a plug-in could
set, asking whether to cache a plugin instance; the Java Plug-In wanted control
over applet caching, not having the browser control it.
Does that ring a bell?  If not, lemme know, and I'll happily send you the
meeting notes written by Eric Krock.

Comment 5

[av (gone)]

No, no, George, I remember all this. It just came up recently that there is no 
uniformity and variable values overlap partially, so some binaries may confuse 
this DoCache thing with GetJavaClass which may probably not be that important 
since new plugins shouldn't worry about java class anyway. Also I noticed that 
some variables belong in wrong places. Which is annoying but not a big issue.

Comment 6

[Peter Lubczynski]

Hm...this explain a lot. If Java is reporting it's cache variable incorrectly 
then that explain why may patch to crashing in ~nsPluginInstanceOwner 
on release works for 4.x plugins and Real Player XPCOM but not Java. I 
will take a closer loook.

Comment 7

[anthonyd]

peter? any update on this? Any thing I can do to help investigate this?

anthonyd

Comment 8

[Peter Lubczynski]

I don't know what can be done about this now. I suggest staying clear of these
messed up variables.

Comment 9

[rubydoo123]

leaving in future, we will try to resolve this during general plug-in clean-up

Comment 10

[Josh Aas]

I cleaned up npapi.h vs. nsplugindefs.h discrepancies a while ago, basically by eliminating the latter.