226380 - ActiveX plugin's properties not properly reflected in Mozilla 1.5a and 1.5

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: ActiveX Wrapper, defect)

Description

[Wang Xianzhu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624
Build Identifier: 

Now I have problems using ActiveX plugins in Mozilla 1.5a and after (including
Firebird 0.6.1 and after).  Plugins work well in Mozilla 1.4 (Firebird 0.6).  
 
The problem is: the plugin control can display correctly, but its properties are
not reflected to JavaScript object.
 
I used Adam Lock's Windows Media Player sample, and inserted the following code:
 
var info = "";
for (i in document.Player)
   info += i + ": " + document.Player[i] + "\n";
alert(info);
 
In Mozilla 1.4, the alert dialog shows correct properties of the Media Player
control; while in Mozilla 1.5a, 1.5 and Firebird 0.7, properties of a normal DOM
Node are displayed.
 
I tried different versions of Mozilla of official installations and debug
versions built by myself, and tried plugins installed from your website (for 1.4
and 1.5) and plugins built by myself.
 
I debugged into the source in embedding/browser/activex/src/plugin and
js/src/xpconnect/src, and found the following problem (?):
 
In 1.4, COM control property reflection was done in xpcwrappednativejsops.c
(calling XPCIDispatchExtension::Enumerate and
XPCIDispatchExtention::DefineProperty); while in 1.5a and after, these code are
moved to XPCIDispatchClassInfo.cpp, which is not publicly exported and used by
activex plugin.
 
I tried to modify xpcwrappednative.cpp:
 
    nsCOMPtr<nsIClassInfo> info(do_QueryInterface(identity));
#ifdef XPC_IDISPATCH_SUPPORT
    // If this is an IDispatch wrapper and it didn't give us a class info
    // we'll provide a default one
    if(isIDispatch /*&& !info */)  
       /* <-- comment out !info.  If identity is the activex plugin, 
          identity's classinfo will be of class nsScriptablePeer 
          (in embedding/browser/activex/src/plugin/XPConnect.cpp), 
          which prevents XPCIDispatchClassInfo from being used */
    {
        info =
already_AddRefed<nsIClassInfo>(XPCIDispatchClassInfo::GetSingleton());
    }
#endif
 
and it seems to work.
 
I doubt that if this is actually a bug, because it has been there for 5 months,
since bug 205768 fixed.


Reproducible: Always

Steps to Reproduce:
In 'Details' section.

Comment 1

[Wang Xianzhu]

I checked diff between current CVS branch head and MOZILLA_1_5_RELEASE, and
found nothing important related to this bug, so I believe this bug is also
applicable to 1.6a and current nightly build.

Comment 2

[Bernard Alleysson]

I've tried the samples on Adam Lock web site
The control displays ok but scripting doesn't work. The javascript console
shows a lot of "properties not defined" errors for the reflection of the control
object. Maybe I am seeing this bug.

Comment 3

[Bernard Alleysson]

The error I see on Adam test is (Mozilla trunk 2003112006 + ActiveX plugin):
Error: document.Player.controls has no properties
Source File: http://www.iol.ie/~locka/mozilla/wmp_movies.htm
Line: 76

Comment 4

[Wang Xianzhu]

"... diff between current CVS branch head ..."
Sorry for a typing mistake: branch --> trunk

Comment 5

[Adam Lock]

CC David. 

Comment 6

[David Bradley]

I bet what is happening is that you can access the properties, but the
enumeration is broken. Reporter can you confirm that. Removing the class info
creation while it may work would generally be bad for security and other reasons.

Comment 7

[Wang Xianzhu]

> Reporter can you confirm that.
The properties are neither able to be enumarated, nor able to be called 
("properties not defined").

In the modified XPConnect code (1.5a and after), ActiveX reflection is moved 
from xpcwrappednativejsops.c into XPCIDispatchClassInfo.cpp, but the ActiveX 
plugin implementation can not utilize it.  The classinfo provided by ActiveX 
plugin, which overrides XPCIDispatchClassInfo, can only reflect methods in 
nsIMozAxPlugin interface, after QueryInterface'd to this interface.  

The commented out code is only for test (to prevent to ActiveX plugin's 
classinfo overriding XPIDispatchClassInfo).

Comment 8

[David Bradley]

Ok, I see what you're saying. So with 1.5 the plugin is now providing a
classinfo  implementation, XPConnect is seeing that and using that instead of
the IDispatch one. I'll have to think on what's the best approach to deal with
this hook the existing one or just bypass it for this wrapper. It would be
interesting to know what was behind the plugin using classinfo.

Comment 9

[Bernard Alleysson]

The URL works fine with Netscape 7.1, doesn't work with Mozilla trunk 2003112006
+ ActiveX plugin. Confirming and adding regression keyword.

Comment 10

[Adam Lock]

David can you give me instructions on what to look for here and I will see if I
can get it up and running.

Comment 11

[David Bradley]

Well, apparently the plugin is providing a class info instance for the object
when it didn't before. The posted test patch seems to verify that. I can't
really fault the plugin for providing that. What I'm not sure of, is if I should
hook my classinfo into the existing one or find some other solution for what I
need to do. So I guess I'd be interested in knowing why a class info was
provided by the plugin for the activex object and/or where that implementation is.

Comment 12

[Bernard Alleysson]

asking blocking1.6 because it is a major regression for anyone using the activex
plugin

Comment 13

[Adam Lock]

I've tried returning nsnull for the scriptable peer and all that does is disable
scripting support completely at the plugin level. So I have to return a
scriptable peer of some kind.

So I tried applying the patch and commenting out the info. This works for simple
objects, but not when objects return other objects. For example, the WMP control
still complains that 'player.controls' doesn't exist.

http://devedge.netscape.com/viewsource/2003/windows-media-in-netscape/media/min-object-usage.html

I think this patch would be better than nothing, but it's still some way short
of fixing the problem.

Comment 14

[chris hofmann]

any more thoughts on this?  going to + to make sure it stays on our radar for 1.6

Comment 15

[Bernard Alleysson]

Someone should verify that backing out bug 205768 fixes this.
If this is true and noone is working on this bug then fix for bug 205768 should be 
reopened and backed out.

Comment 16

[Bernard Alleysson]

I have verified that backing out bug 205768 fixes this bug. just run:
cvs update -j1.45 -j1.44 mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp
cvs update -j1.11 -j1.10 mozilla/js/src/xpconnect/src/XPCIDispatchExtension.cpp
cvs update -j1.2 -j1.1 mozilla/js/src/xpconnect/src/XPCIDispatchClassInfo.cpp
cvs update -j1.12 -j1.11 mozilla/js/src/xpconnect/src/XPCDispPrivate.h
and rebuild.

Comment 17

[Bernard Alleysson]

Even with the backout scripting the MS calendar control does not work:

Error: document.cal.invoke is not a function
Source File: http://www.iol.ie/~locka/mozilla/calendar_scripted.htm
Line: 57

I don't know why

Comment 18

[Adam Lock]

The test page is for an older fixed interface version of the plugin that had
methods such as invoke, setProperty and getProperty that invoked the appropriate
method on the control. XPConnect should mean that it can done transparently
without this burdonsome extra step.

If you want to verify that it works, try the NS7.1 link I supplied earlier or
change the calendar html so the onclick handler (on the Next Month button for
example) calls the method directly (e.g. document.cal.nextMonth()).

Comment 19

[David Bradley]

Backing that patch out will cause the problems detailed in that bug. Again,
there apparently was a patch in plugin code that added classinfo to the plugin
objects. It would be interesting to know what patch that is and the reasons
behind providing that class info.

I could replace the class info, but that seems heavy handed. Alternatively I can
hook into the class info, forwarding all calls to the original with the
exception of enumeration and resolving. The question for those, is should I
"append" properties or only enumerate the IDispatch properties. Knowing why
their is an existing class info will help in making decisions reguarding this.

Unfortunately I don't have much time for XPConnect these days, otherwise I'd dig
and find this stuff myself. I just need a file reference to where the existing
class info implementation resides.

Comment 20

[Adam Lock]

The plugin's scriptable peer is implemented in nsIClassInfoImpl

http://lxr.mozilla.org/seamonkey/source/embedding/browser/activex/src/plugin/XPConnect.h#60

Most of the methods return NS_ERROR_NOT_IMPLEMENTED at the moment. This
scriptable peer was originally for the fixed nsIMozAxPlugin interface, although
the peer is still necessary in order for Gecko to think the plugin is scriptable.

Comment 21

[Asa Dotzler [:asa]]

Since bug 205768 says it doesn't fix any known problems, should we just back
that out and make that change in 1.7 or later when we've got a handle on how to
not regress this?

Comment 22

[David Bradley]

I was really hoping to find a better solution than breaking existing working
functionality, but my time is extremely limitted and backing that patch out may
be the best short term approach :-(

Comment 23

[chris hofmann]

leaf is working to back out on the branch

Comment 24

[Daniel (Leaf) Nunes]

patch 123465 has been backed out of the 1.6 branch.

Comment 25

[chris hofmann]

this should be fixed now with leaferoni's back out

Comment 26

[Daniel (Leaf) Nunes]

backout of the patch (alone) caused bustage; i need a bigger js/src/xpconnect
brain to look at the patch to back it out of 1.6 (see
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla1.6/1073606820.17013.gz for
bustage details)

Comment 27

[David Bradley]

Attachment: This should backout the change

This is the backout version of the patch. There were apparently issues with
backing out the XPCIDispatchScriptableHelper. Not sure why, as it hasn't
changed much, I don't think. But this compiles ok for me on a trunk that was
taken before 1.6 branch.

Comment 28

[Adam Lock]

The backout restores the scripting functionality at least as far as the NS7.1
WMP page is concerned. This is a fairly comprehensive real world test involving
events and nested objects so I'm pretty happy about that :)

Comment 29

[Bernard Alleysson]

Sorry, this is not fixed on the trunk because the backout of the patch only
occured on 1.6 branch. I'm asking blocking 1.7a so that the backout could be
considered on the trunk also.

Comment 30

[chris hofmann]

yes, don't want to regress again.  dbradley, can you land the back out on the trunk?

Comment 31

[Daniel (Leaf) Nunes]

Comment on attachment 138677 [details] [diff] [review]
This should backout the change

making an official approval request.

Comment 32

[chris hofmann]

Comment on attachment 138677 [details] [diff] [review]
This should backout the change

a=chofmann for 1.7a

Comment 33

[Daniel (Leaf) Nunes]

back-out committed to the trunk for 1.7a; are we waiting for a "real" fix, or
can this bug be resolved?

Comment 34

[David Bradley]

Thanks Leaf. Yes this bug is resolved now. The original bug 205768 remains which
deals with enumeration. I don't believe that should break any real ActiveX based
sites, since most would access properties/methods directly not via enumeration.
It's more an issue of completeness and that you should be able to do it if you
wanted to.