103940 - Seamless support for application/x-oleobject plugins in OBJECT tags doesn't work

Status: VERIFIED FIXED

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

Description

[Adam Lock]

I'll take this bug until I can isolate the issues (if any)

Mozilla has code to automatically look for a plugin that can handle
application/x-oleobject objects when it encounters an OBJECT tag with a CLSID
attribute:

http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsObjectFrame.cpp#995
We have a plugin that can handle application/x-oleobject in the form of the
Mozilla plugin for hosting ActiveX controls but it doesn't appear to work
properly with the built-in support.

This would be a major win if *limited* but seamless support for ActiveX controls
can be added to Mozilla.

Comment 1

[Adam Lock]

Issue appears to boil down to 2 problems:

1. The npmozax plugin has a few bugs in its CLSID param parsing which mean it
doesn't work when the CLSID is of the form
"clsid:00000000-0000-0000-0000-000000000000". These issues have been fixed and
will be checked in soon. I will also change the code so "param_" is no longer a
necessary prefix on named parameters for the control.

2. The nsObjectFrame class in Mozilla does not pass the CLSID to the plugin so
it has no idea what control it should create. The current workaround is to
supply the CLSID twice, once in the OBJECT tag and again as a PARAM to the
plugin, e.g.

  <object width="200" height="300"
    classid="clsid:8E27C92B-1264-101C-8A2F-040224009C02">
    <PARAM name="clsid" value="clsid:8E27C92B-1264-101C-8A2F-040224009C02"/>
    <PARAM name="backcolor" value="65535"/>
  </object>

I'll work on a patch that passes the CLSID to the plugin without it needing to
be specified twice.

Comment 2

[Adam Lock]

Attachment: Patch allows extra parameters to be sent to the plugin.

Comment 3

[Adam Lock]

CC'ing plugin people.

I have a patch here that makes ActiveX control support in Mozilla much better,
almost seamless in fact. 

Would you be able to review it? It builds on the existing code that looks for a
plugin to handle "application/x-oleobject" types if a CLSID was specified in the
OBJECT tag. The patch itself adds a new nsPluginInstanceOwner::AddExtraParameter
method that is called to supply the "CLASSID" parameter to the ActiveX plugin in
 addition to any parameters in the content.

The patch means the plugin knows what control to embed because it had no clue
before. This allows you to do stuff like this:

<object width="200" height="300"
    classid="clsid:8E27C92B-1264-101C-8A2F-040224009C02">
  <PARAM name="backcolor" value="65535"/>
</object>

Obviously this is not FULL control support. You can only instantiate the control
 if it is installed and my plugin has no support for scripting it. 

Comment 4

[Peter Lubczynski]

Could you just have used nsPluginInstanceOwner::GetAttributes() from the plugin?

Comment 5

[Adam Lock]

The plugin has support for Mozilla plugins interface but the impl is so old it
has been commented out. For the time being all I have is the legacy NP API impl.
If the plugin interfaces are frozen then I could consider updating my code to
use them.

Comment 6

[Peter Lubczynski]

Oh...then sadly I don't think there is a clean way to get at the new plugin API
interfaces from the old API. Maybe there should be? av, beard?

Your hack seems better than the one I had in mind. Another idea is to possibly
use npp->ndata. In Mozilla, you might be able to cast that to an nsI pointer and
get to GetAttribute through the peer. Maybe something like this:

nsISupports *inst = (nsISupports *) npp->ndata;
nsCOMPtr<nsIPluginInstance> pi = do_QueryInterface(inst);
nsCOMPtr<nsIPluginInstancePeer> peer;
pi->GetPeer(*getter_AddRefs(peer));
nsCOMPtr<nsIPluginInfoTag2> pit2 = do_QueryInterface(peer);
peer->GetAttribute("classid",&result);

Comment 7

[Peter Lubczynski]

Comment on attachment 52897 [details] [diff] [review]
Patch allows extra parameters to be sent to the plugin.

r=peterl on acceptable hack for classid

Comment 8

[Patrick C. Beard]

A couple of nits, and a question about invariants. You are using raw
PL_strdup/PL_free instead of nsCRT::strdup/nsCRT::free(). Also, once
nsPluginInstanceOwner::GetParameters() is called, you are still keeping extra
copies of the extra parameters.

What if somebody calls nsPluginInstanceOwner::AddExtraParameter() again after
GetParameters()? I'd feel better if we just reused some kind of expandable array
to hold ALL of the parameters, rather than adding new variables to manage the
extra parameters. We'd then freeze the parameter arrays on first call to
GetParameters(), and perhaps assert if somebody calls AddExtraParameter() later.

Comment 9

[Adam Lock]

My patch was written in the style of the existing code which uses various PR_ &
PL_ functions, but I'll go through it all and see if it made more sane. A lot of
the mess could probably swept away by using nsCStringArray.

I will also stick a flag in to AddExtraParameter that makes it assert and error
if its called at the wrong time.

Comment 10

[Adam Lock]

Attachment: New patch (work in progress)

This patch covers all of Patrick's issues with the original one. I still have
to test one or two things on it yet.

Comment 11

[Peter Lubczynski]

Adam, I now think this should be fixed to be the same way 4.x did it:

"In Netscape 4.x, plugins invoked by the OBJECT tag would receive the tag's 
attributes in the arg arrays followed by a dummy "PARAM" value in the argn 
array with a corresponding NULL in the argv array then followed by the 
name/value pairs from all the PARAM tags."

See bug 111008 which was recently opened after a newsgroup post.

Comment 12

[Peter Lubczynski]

Attachment: patch to fix up attribute/parameter handeling (please review)

Here's a patch that (hopefully) fixes ALL the issues we have with attributes
and parameters with 4.x-style plugins. It should fix this bug, along with bug
111008 and bug 39609. Please try/review.

Comment 13

[Peter Lubczynski]

--->peterl...taking bug since I posted patch...

Comment 14

[Peter Lubczynski]

Attachment: new patch fixing little nits

New patch fixing two little nits:

1) No longer include PARAM tags without name attributes in roundup
2) Now really getting parity with 4.x by changing null to PARAM. See comment in
bug 111008

Comment 15

[av (gone)]

Comment on attachment 60174 [details] [diff] [review]
new patch fixing little nits

r=av, it's nice clean up too

Comment 16

[Peter Lubczynski]

Patrick, can I get a super review?

Comment 17

[Patrick C. Beard]

Comment on attachment 60174 [details] [diff] [review]
new patch fixing little nits

sr=beard

Comment 18

[Peter Lubczynski]

Patch in *today's* trunk. Marking FIXED and also nominating for the 0.9.4 branch
because this is desired for 4.x NPAPI compatibility.

Comment 19

[Peter Lubczynski]

*** Bug 111008 has been marked as a duplicate of this bug. ***

Comment 20

[Peter Lubczynski]

*** Bug 39609 has been marked as a duplicate of this bug. ***

Comment 21

[Judson Valeski]

We're currently not targeting any plugins for the 0.9.4 release that would
leverage this (AFAIK; someone correct me if I'm wrong). Also, this is enough of
a change that we need this to cook on the trunk for awhile. minusing edt0.9.4

Comment 22

[Judson Valeski]

My bad. reversing the minus to a plus.

Comment 23

[Peter Lubczynski]

Attachment: updated patch for 0.9.4 branch

Comment 24

[Patrick C. Beard]

Comment on attachment 61933 [details] [diff] [review]
updated patch for 0.9.4 branch

sr=beard

Comment 25

[shrirang khanzode]

i am rubber stamping this fix. VERIF