193782 - [AxPlugin] Property bag implementation in plugin/control is wrong

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Adam Lock]

The implementation of IPropertyBag::Read in PropertyBag.cpp is incorrect. It
should be treating the VARIANT param as [in,out], not just [out].

This added correctness might break badly behaved controls which are not
supplying a valid [in] value so I might have to add some pointer tests to ensure
the method ignores this value if it is filled with garbage. Possibly I will also
have to remove the smart code in LegacyPlugin.cpp that coerces the params
supplied in the content into VT_I4, VT_BOOL etc. types and just leave them as
VT_BSTR for the control to deal with when it reads them.

Comment 1

[Adam Lock]

Attachment: Patch

This patch fixes IPropertyBag::Read implementation. It grabs the vt specified
by the [in] variant, VariantInit's the variant and then attempts to copy into
it the named value with the type specified in vt. I also changed the return
code which should be E_FAIL according to the docs.

Comment 2

[Adam Lock]

Attachment: Patch

This patch fixes IPropertyBag::Read implementation. It grabs the vt specified
by the [in] variant, VariantInit's the variant and then attempts to copy into
it the named value with the type specified in vt. I also changed the return
code which should be E_FAIL according to the docs.

Comment 3

[Adam Lock]

Comment on attachment 114799 [details] [diff] [review]
Patch

Duped attachment

Comment 4

[Adam Lock]

Comment on attachment 114800 [details] [diff] [review]
Patch

David & Chris, can I have an r/sr on this patch please? Thanks.

Comment 5

[David Bradley]

Comment on attachment 114800 [details] [diff] [review]
Patch

Why not use CComVariant methods? this would clean up some of the casts.

+	     if (vt == VT_EMPTY)
+	     {
+		 hr = vNew.Copy(m_PropertyList.GetValueOf(i));
+	     }
+	     else
+	     {
+		 hr = vNew.ChangeType(vt, m_PropertyList.GetValueOf(i));
+	     }

...


	     // Copy the new value
-	     VariantCopy(pVar, const_cast<VARIANT
*>(m_PropertyList.GetValueOf(i)));
+	     vNew.Detach(pVar);
	     return S_OK;

Comment 6

[Christopher Blizzard (:blizzard)]

Comment on attachment 114800 [details] [diff] [review]
Patch

sr=blizzard

Comment 7

[Adam Lock]

Attachment: Updated patch

David, new patch addresses your issues. Can you take a look? Thanks

Comment 8

[David Bradley]

Comment on attachment 114903 [details] [diff] [review]
Updated patch

r=dbradley

Do you really need the const cast in the assignment to pvSrc. From what I can
tell all uses of it take a const VARIANT  pointer.

Comment 9

[Adam Lock]

Comment on attachment 114903 [details] [diff] [review]
Updated patch

Requesting approval for 1.3 checkin.

Risk is low, confined to ActiveX and fixes crash issues with some controls.

Comment 10

[Adam Lock]

Nice catch, I'll remove the const_cast for checkin.

It was originally necessary to remove the const for calls to the
VariantCopy/VariantChangeType functions, but the CComVariant equivalents seem to
do the right thing.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 114903 [details] [diff] [review]
Updated patch

a=asa (on behalf of drivers) for checkin to 1.3

Comment 12

[Adam Lock]

Fix is checked in

Comment 13

[Ashish Bhatt]

Marking as verified