198668 - [AxPlugin] Ofoto control does not return the value of properties correctly

Status: RESOLVED FIXED

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

Description

[Ashish Bhatt]

Tested with 2003-03-21

When any property of ofoto control is accessed ofoto control does not return the
property correctly.

For example if HOST property is access in document.ofotoControl.HOST it should
return "http://ofoto.com" which it does in IE  it returns "function HOST() {
[native code]}"

Comment 1

[Ashish Bhatt]

Attachment: Testcase

Comment 2

[Adam Lock]

Comment on attachment 118081 [details]
Testcase

Changing to text/plain so I can read it...

Comment 3

[Adam Lock]

Verifying the problem. The syntax in the test case matches the API (i.e.
UPLOADURL property really is uppercase).

The API for IDndCtrl (the automation interface on the Ofoto control) is below:

[
  odl,
  uuid(0DD4833C-DFFA-11D3-94D7-0050DAC353B6),
  helpstring("IDndCtrl Interface"),
  dual,
  oleautomation
]
interface IDndCtrl : IDispatch {
    [id(0x60020000)]
    HRESULT UploadFile(
                    [in] BSTR file, 
                    [in] int size);
    [id(0x60020001)]
    HRESULT Uploading([in] int upload);
    [id(0x60020002)]
    HRESULT DoUpload();
    [id(0x60020003)]
    HRESULT StopUpload();
    [id(00000000), propput]
    HRESULT HOST([in] BSTR pstrValue);
    [id(00000000), propget]
    HRESULT HOST([out, retval] BSTR* pstrValue);
    [id(0x00000001), propput]
    HRESULT PORT([in] BSTR pstrValue);
    [id(0x00000001), propget]
    HRESULT PORT([out, retval] BSTR* pstrValue);
    [id(0x00000002), propput]
    HRESULT USERID([in] BSTR pstrValue);
    [id(0x00000002), propget]
    HRESULT USERID([out, retval] BSTR* pstrValue);
    [id(0x00000003), propput]
    HRESULT WHERE([in] BSTR pstrValue);
    [id(0x00000003), propget]
    HRESULT WHERE([out, retval] BSTR* pstrValue);
    [id(0x00000004), propput]
    HRESULT COLLID([in] BSTR pstrValue);
    [id(0x00000004), propget]
    HRESULT COLLID([out, retval] BSTR* pstrValue);
    [id(0x00000005), propput]
    HRESULT COLLNAME([in] BSTR pstrValue);
    [id(0x00000005), propget]
    HRESULT COLLNAME([out, retval] BSTR* pstrValue);
    [id(0x00000006), propput]
    HRESULT COLLDESC([in] BSTR pstrValue);
    [id(0x00000006), propget]
    HRESULT COLLDESC([out, retval] BSTR* pstrValue);
    [id(0x00000007), propput]
    HRESULT SESSIONID([in] BSTR pstrValue);
    [id(0x00000007), propget]
    HRESULT SESSIONID([out, retval] BSTR* pstrValue);
    [id(0x00000008), propput]
    HRESULT SERVERNAME([in] BSTR pstrValue);
    [id(0x00000008), propget]
    HRESULT SERVERNAME([out, retval] BSTR* pstrValue);
    [id(0x00000009), propput]
    HRESULT CLIENT([in] BSTR pstrValue);
    [id(0x00000009), propget]
    HRESULT CLIENT([out, retval] BSTR* pstrValue);
    [id(0x0000000a), propput]
    HRESULT UPLOADURL([in] BSTR pstrValue);
    [id(0x0000000a), propget]
    HRESULT UPLOADURL([out, retval] BSTR* pstrValue);
    [id(0x0000000b), propput]
    HRESULT MODE([in] BSTR pstrValue);
    [id(0x0000000b), propget]
    HRESULT MODE([out, retval] BSTR* pstrValue);
    [id(0x0000000c), propput]
    HRESULT PHOTOIDS([in] BSTR pstrValue);
    [id(0x0000000c), propget]
    HRESULT PHOTOIDS([out, retval] BSTR* pstrValue);
};

Comment 4

[Adam Lock]

Weird, this code will work:

  alert("UploadURL - " +  document.DndCtrl.UPLOADURL());

I think UPLOADURL is being treated as a function or a parameterized property for
some reason. It is definitely recognised however since calling
document.DndCtrl.IDONTEXIST returns undefined. The IDL below doesn't give any
clues for this behaviour.

Still investigating...

Comment 5

[Adam Lock]

Reassigning to David.

UPLOADURL is definitely being treated as a parameterized property and without
the braces XPCIDispatchExtension::DefineProperty appears to return the property
name as if it were a function rather than calling it.

Perhaps IsParameterizedProperty should check that if the param count is 1 and
the param is flagged with PARAMFLAG_FRETVAL that it is treated as a normal
property with a return code.

The existing code probably only works properly with interfaces declared with the
"dispinterface" notation in IDL and not those declared as "IFoo : IDispatch"
with properties declared using [retval] to denote the return values.

Comment 6

[Adam Lock]

A further note, the WMP uses the dispinterface notation, and this JS works:

alert(document.player.URL);

The IDL for WMP is shown below and uses the dispinterface keyword and using
C-like notation for the return type. Perhaps a call to GetFuncDesc on this would
return FUNCDESC describing a get property with 0 params and a BSTR elemdescFunc
(the return type), while a GetFuncDesc on a property with a [retval] param
returns 1 params and a void elemdescFunc. Both should obviously be treated the
same way.

[
  uuid(7587C667-628F-499F-88E7-6A6F4E888464),
  helpstring("IWMPCore3: Public interface."),
  dual
]
dispinterface IWMPCore3 {
properties:
methods:
    [id(0x00000001), propget, helpstring("Returns or sets the URL")]
    BSTR URL();
    [id(0x00000001), propput, helpstring("Returns or sets the URL")]
    void URL([in] BSTR rhs);
    // ... rest of interface snipped ...
};

Comment 7

[David Bradley]

Thanks for tracking it down, I suspected it was something like that. I'll get a
patch up in a bit.

Comment 8

[David Bradley]

Attachment: checks the paramter to see if it's a return value

I have to take off early, will be back later, but I'm having trouble with my
build unrelated to IDispatch. Was wondering if someone could test this quickly
and let me know if it fixes the problem.

Comment 9

[Adam Lock]

The patch as it stands doesn't work for a couple of reasons, the first being 0
should be passed to GetParamInfo() instead of 1. However even with this change
it doesn't work because the paraminfo comes from the setter version of the
property, not the getter version. The setter and getters have different
parameters and the first param returned actually an [in] param, rather than the
[out,retval] that we would expect to test against.

I'll try and work up a patch to do this.

Comment 10

[Adam Lock]

Attachment: Updated patch

Updated patch also adds a mGetterFuncDesc, SetGetterFuncDesc to the Member
class and ensures IsParameterizedProperty does the right thing. The
GetParamCount and GetParamInfo have a flag to control whether the setter
(default) or getter funcdesc is queried.

My analysis was slightly off - I think the reason for the issue was that
original code checked the setter's funcdesc so IsParameterized property
couldn't possibly return the right answer since it was checking the wrong
funcdesc. This patch will fix the issue and add the extra test to see if the
getter has a single param flagged as the retval.

Comment 11

[David Bradley]

Attachment: Same as before with a few adjustments to clear null pointer references

I think this should do the trick.

Thanks for reorganizing IsParameterizedGetter logic. It threw me for a moment,
as I missed the > 0 to > 1 change. Makes it easier to read.

Comment 12

[David Bradley]

So who should r= this? We've both had our hand in this patch.

Comment 13

[Adam Lock]

I had one final thought which may be a concern. I don't see anything in
XPCDispInterface::InspectIDispatch that checks whether the first member has the
setter or the getter listed first. It may be possible that this code will mix
things up and end up putting the getter funcdesc in the setter and vice versa.

In fact is it even possible for the getter and setter versions not be next to
each other at all, for example if the interface defines the setter at the top of
the idl and the getter some way down?

We probably need some logic here too to make sure things are put in the right place.

Comment 14

[David Bradley]

Let's handle one bug at a time ;-). Good point, I thought I remember reading
somewhere that this assumption was ok, but I can't find it now. Filed bug 199122.

Comment 15

[David Bradley]

Comment on attachment 118390 [details] [diff] [review]
Same as before with a few adjustments to clear null pointer references

Adam, let's both review the patch and give an r=. Unless you have a better
idea. I'd like to see if I can get this in before the freeze tonight, but don't
know if that's possible.

Comment 16

[David Bradley]

I'll be optimistic and set it to 1.4 alpha

Comment 17

[Adam Lock]

Comment on attachment 118390 [details] [diff] [review]
Same as before with a few adjustments to clear null pointer references

r=adamlock

Comment 18

[David Bradley]

r=dbradley for adam's part of the patch

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 118390 [details] [diff] [review]
Same as before with a few adjustments to clear null pointer references

- In XPCDispPrivate.h:

@@ -861,6 +879,7 @@
	 jsval mName;	 // Mutable
	 CComPtr<ITypeInfo> mTypeInfo;
	 FUNCDESC* mFuncDesc; // We keep hold on this so we don't have 
+	 FUNCDESC* mGetterFuncDesc; // We keep hold on this so we don't have 

Looks like both those comments are equally incomplete :-)

sr=jst

Comment 20

[David Bradley]

Fix checked in.