Closed Bug 226380 Opened 21 years ago Closed 21 years ago

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

Categories

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

Product:
Core Graveyard
Component:
Embedding: ActiveX Wrapper
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: phnixwxz1, Assigned: adamlock)

References

(
URL
)

Details

(Keywords: fixed1.6, regression)

Attachments

(1 file)

This should backout the change
21.57 KB, patch
chofmann
: approval1.7a+
Details | Diff | Splinter Review 
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.
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.

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.
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

"... diff between current CVS branch head ..."
Sorry for a typing mistake: branch --> trunk
CC David. 
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.

> 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).

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.
The URL works fine with Netscape 7.1, doesn't work with Mozilla trunk 2003112006
+ ActiveX plugin. Confirming and adding regression keyword.
URL: http://www.iol.ie/~locka/mozilla/wmp_...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
David can you give me instructions on what to look for here and I will see if I
can get it up and running.
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.
asking blocking1.6 because it is a major regression for anyone using the activex
plugin
Flags: blocking1.6?
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.
any more thoughts on this?  going to + to make sure it stays on our radar for 1.6
Flags: blocking1.6? → blocking1.6+
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.

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.
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



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()).
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.
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.


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?
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 :-(

leaf is working to back out on the branch
patch 123465 has been backed out of the 1.6 branch.
this should be fixed now with leaferoni's back out
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
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)
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.
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 :)
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.
Status: RESOLVED → REOPENED
Flags: blocking1.7a?
Keywords: fixed1.6
Resolution: FIXED → ---
yes, don't want to regress again.  dbradley, can you land the back out on the trunk?
Flags: blocking1.7a? → blocking1.7a+
Comment on attachment 138677 [details] [diff] [review]
This should backout the change

making an official approval request.
Attachment #138677 - Flags: approval1.7a?
Comment on attachment 138677 [details] [diff] [review]
This should backout the change

a=chofmann for 1.7a
Attachment #138677 - Flags: approval1.7a? → approval1.7a+
back-out committed to the trunk for 1.7a; are we waiting for a "real" fix, or
can this bug be resolved?
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.
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: