182117 - NPAPI variables that pass COM objects need an ABI bit

Status: RESOLVED FIXED

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

Description

[Peter Lubczynski]

This bug is basically a re-do of bug 167178 in that we need a more generic way
of ensuring that the COM objects passed back from the plugin (scriptable peer)
and the ones the browser gives the plugin (service manager and DOM) are of the
same ABI as the reciever. Rather than defining new variables for each object and
platform, how about encoding a bit value in each variable.

Comment 1

[Peter Lubczynski]

Attachment: patch v.1

Here's a patch implementing the idea.

Comment 2

[Patrick C. Beard]

Comment on attachment 107531 [details] [diff] [review]
patch v.1

r=beard

Comment 3

[Simon Fraser [no longer active]]

Comment on attachment 107531 [details] [diff] [review]
patch v.1

+ * Flag for indicating compatible ABI formats
+ * for COM object returned from NPAPI variables.
+ */
+#ifdef TARGET_RT_MAC_MACHO
+#define NP_ABI_BASE_FLAG   1024
+#else
+#define NP_ABI_BASE_FLAG   0
+#endif
+ 

Should this be generalized a bit so that it can be extended to deal with binary
incompatibilities on other platforms, and between different compilers on the
same platform?

Comment 4

[Peter Lubczynski]

so we need another bit for compiler version, like for the gcc 2.x ----> 3.x switch

Comment 5

[Simon Fraser [no longer active]]

I think we need more. I would expect to see a series of #ifdefs that set
NP_ABI_BASE_FLAG for each ABI-compatible compiler version on each platform.

Comment 6

[Peter Lubczynski]

Attachment: patch v.2

Here's a patch which encodes each compiler defined ABI version into any
variable that deals with C++ objects.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think __GXX_ABI_VERSION is only defined by gcc 3.0 and above, so this won't
distinguish egcs 1.1.2 from gcc 2.95 from gcc RH-2.96.

Comment 8

[Bradley Baetz (:bbaetz)]

Whilst I'm not really qualified to comment on the patch, this looks good.

One thing to keep in mind is that in theory the g++ABI is corss vendor, so an
x86 linux plugin can run on x86 solaris, for example. theres a different define
to handle that, although theres notmuch point in testing for it until we know
that the implementation bugs have been fixed in gcc.

Comment 9

[Peter Lubczynski]

Attachment: patch v.3

so cls says gcc 2.95 has the same ABI as egcs 1.1.2 so it doesn't need this. It
does link against a different stdlib so if there a problem, we'd crash earlier
on loading the library. RH-2.96 does produce a different ABI so I've given it a
bit in this patch. On the trunk, Mach-O builds have __GXX_ABI_VERSION as 100.

Comment 10

[Patrick C. Beard]

Comment on attachment 109678 [details] [diff] [review]
patch v.3

r=beard (I prefer the | operator to the + operator when doing bit twiddling).

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 109678 [details] [diff] [review]
patch v.3

Do we care about non-gcc compilers and platforms? Should we make the CFM/MachO
difference explicit?

Comment 12

[hacker formerly known as [e-mail]]

I don't know if we care about them but Irix builds have the same ABI issue.  The
4.x builds & plugins were built using the -o32 ABI.  xptcall for irix has only
been ported to the -n32 ABI so the old plugins won't work.  I'm not sure if we
see any extremely bad behavior (ie, crashes) because of the old plugins though.

CFM/Mach-o probably does need to be more explicit. CFM builds & 10.1 mach-o
builds will both have a base offset of zero.  And it doesn't look like CW8
defines __GXX_ABI_VERSION even though it's supposedly supposed to be compatible.

Comment 13

[Simon Fraser [no longer active]]

Comment on attachment 109678 [details] [diff] [review]
patch v.3

Need to address issues that cls brings up.

Comment 14

[Peter Lubczynski]

Is the vtable layout generated by CW8 compatible with any version of gcc?

Are we going to need to evangelize the specific compiler used in various
embedding products or standardize on one so scripting can be made to work in Mach-O?

Comment 15

[Jonathan Brecher]

Um, guys, this is getting a little scary.  We really don't want to switch our 
whole project to Mach-O, but that's something that we could at least consider.  
Switching to gcc?  Not going to happen.

Our issues aside, how is any plugin developer supposed to install a scriptable 
plugin in the best of circumstances?  There's no way to figure out automatically 
at install time what browser a user prefers.  Can you imagine a dialog asking 
users to provide that information? "Please indicate if your preferred browser is 
a CFM application, was compiled with gcc X.x, gcc Y.y, or CodeWarrior."  And a 
decision *is* necessary since you cannot install more than one plugin for a 
given MIME type.  And heaven help the user who regularly uses more than one 
browser!

I really fear that we need to find a solution that allows a single scriptable 
plugin to work with any browser, irrespective of ABI.  Anything else is going to 
be a nightmare for developers and for users.

Clearly it's possible for Mach-O browsers to talk to CFM plugins in general, 
with the exception of scriptability.  Would it be worth considering a very 
minimal scriptability interface that didn't depend on COM objects?  I agree that 
COM objects are an excellent technical solution, but I'd much rather have one 
that worked.  FWIW, our entire interface consists exclusively of methods and 
accessors:

  NS_IMETHOD Method(void)
  NS_IMETHOD Accessor(const char *type, char **_retval)

Would it be possible to have some sort of callback into the browser that simply 
registered function pointers of this type?

NPP_RegisterMethod("MethodName1", (NPPMethodPtr)Method1);
NPP_RegisterMethod("MethodName2", (NPPMethodPtr)Method2);
NPP_RegisterAccessor("AccessorName1", (NPPAccessorPtr)Accessor1);
NPP_RegisterAccessor("AccessorName2", (NPPAccessorPtr)Accessor2);
NPP_RegisterAccessor("AccessorName3", (NPPAccessorPtr)Accessor3);

Something like that would get us up and running nice and smoothly.  I don't know 
if other developers need more, but this would probably be pretty good for a lot 
of them.

Well, it's a thought, anyway.  Regardless, I hope you'll keep in mind that not 
every plugin is bundled with the browser, and that whatever ends up happening in 
terms of scriptability, it has to be something that external developer can 
support and that external users can use...

Comment 16

[Peter Lubczynski]

per topembed+ triage: topembed+ for 1.4

Note that scriptability is not the only time COM objects are passed between the
browser and the plugin. The plugin could request a pointer to nsIServiceManager
and have access to almost anything. Also starting with 1.2, the plugin also has
direct access to the nsIDOMNode for the tag and document.

Comment 17

[Jonathan Brecher]

Well, then maybe this bug should be split up into the general parts 
(scriptability) and the Mozilla/Netscape-specific parts (everything else)?  Does 
any other browser allow access to nsIServiceManager and nsIDOMNode?  We don't 
need access to those, but we're stymied in providing a scriptable plugin because 
some browsers require CFM for access to scriptability, and some require Mach-O, 
and it's not impossible to satisfy both at the same time.

Comment 18

[Peter Lubczynski]

Attachment: patch v.4

Here's a new patch that adds two masks to the NPAPI, one for gcc3 and one for
Mach-O. The idea is that these masks can be used for backwards compatibility to
determine the ABI format the browser expects. This lumps both 10.1 builds and
CW-Mach together into the same effective mask but that should be fine as there
are no shipping browsers off these and that'll work good enough to stop crashes
if there ever are.

There are different ideas for how to hand back a properly formated object and
not have duplicate plugins. One idea calls for having the C++ scripting shim
live in seperate libraries (in the bundle) and the plugin is responsible for
dynamically loading and instantiating the right object based on what the
browser asks. Another idea is to possibly take a look at the sample C
implemented XPCOM component in bug 72083 and perhaps to construct compatible
vtables by hand for nsISupports and nsIClassInfo.

Comment 19

[Peter Lubczynski]

This fix is needed if Mozilla 1.3 wants scriptable plugins in gcc3 or Mach-O builds.

Comment 20

[Simon Fraser [no longer active]]

+#define NP_ABI_GCC3_MASK  0x10000000
+#define NP_ABI_MACHO_MASK 0x01000000
+
+#if (defined (XP_UNIX) && defined(__GNUC__) && (__GNUC__ >= 3))
+#define _NP_ABI_GCC3_MASK NP_ABI_GCC3_MASK
+#else
+#define _NP_ABI_GCC3_MASK 0
+#endif
+
+#if (defined(TARGET_RT_MAC_MACHO))
+#define _NP_ABI_MACHO_MASK NP_ABI_MACHO_MASK
+#else
+#define _NP_ABI_MACHO_MASK 0
+#endif

I had to stare at this for a few minutes before I noticed that
_NP_ABI_MACHO_MASK and NP_ABI_MACHO are not the same thing. Please make them
more different. I guess the approach is fine, but I'd still like to see some
comments to to say why this is only being done for Unix/Macho, just to keep the
Windows developers in the loop.

Comment 21

[Peter Lubczynski]

Attachment: patch v.4.1

patch with better comments

Comment 22

[Peter Lubczynski]

Attachment: patch v.5

I found that if I include npapi.h in nsplugindefs.h, I'm going to have to
change any REQUIRES line that includes plugins to also include java because of
the dependancy on jri.h. nsplugindefs.h is really for deprecated XPCOM plugins
and not plugin vendors and should only be used internally. So this new patch
does not make any changes to nsplugindefs.h but instead removes the ussage of
nsPluginInstanceVariable in calling GetValue.

Comment 23

[Peter Lubczynski]

patch checked in last night so this is in today's trunk builds, marking FIXED

Simon, can you check this into the Chimera branch as well?

QA: This fix can be verified by attempting to script a plugin, like Flash, and
not crashing. Mach-O and gcc3 (like w/RH8) builds should not crash but scripting
will not work until plugin vendors update their code.

Comment 24

[Jonathan Brecher]

Speaking of "until plugin vendors update their code", it would be really really 
helpful if there was sample code that showed how to pull that off.  We are not 
able to abandon all of the people currently using CFM browsers (read: all of the 
people currently using our plugin), so we need a solution that lets a single 
plugin provide the proper connections to both CFM and Mach-O browsers.

I've split this issue into a separate bug 195264.

Comment 25

[Peter Lubczynski]

Comment on attachment 115649 [details] [diff] [review]
patch v.5

Requesting approval for Mozilla 1.3 final:

This fix is needed for 1.3 final because the Mach-O/CFM switch on OSX. The
branch currently has plugin scripting with the NPAPI completely disabled so it
is not possible for a plugin vendor to write a scriptable plugin for Mach-O
without this fix. Users will see this as a regression from CFM and it won't be
possible for plugin vendors to release a compatible plugin without this fix. In
addition, the branch currently crashes when a plugin like Flash is accessed
from Javascript in builds made with gcc3. This fix changes the values of enums
in the NPAPI for Mach-O and gcc3 builds only, therefore preventing crashes and
providing a unique selectors for plugins to hand back their scripting
interfaces in the correct ABI.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 115649 [details] [diff] [review]
patch v.5

a=asa (in behalf of drivers) for checkin to 1.3 branch.