344196 - nsVariant::SetAsDOMString sets type to nsIDataType::VTYPE_ASTRING not VTYPE_DOMSTRING

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew Miller]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Latest CVS trunk

The nsVariant::SetAsDOMString function (which implements part of nsIWritableVariant) delegates the work to SetFromAString, but then doesn't override the type back to nsIDataType::VTYPE_DOMSTRING. This means that there is no way to set a VTYPE_DOMSTRING except through XPConnect.

Reproducible: Always

Steps to Reproduce:
Sample code which demonstrates the problem:
    nsString drawColour;
    ret = aDTCR->GetDrawColour(drawColour);
    NS_ENSURE_SUCCESS(ret,);
    mDrawColour =  do_CreateInstance(NS_VARIANT_CONTRACTID, &ret);
    NS_ENSURE_SUCCESS(ret,);
    mDrawColour->SetAsDOMString(drawColour);
    PRUint16 dt;
    mDrawColour->GetDataType(&dt);
    NS_ASSERTION(dt == nsIDataType::VTYPE_DOMSTRING,
                 "Buggy nsIVariant set wrong type");

Actual Results:  
###!!! ASSERTION: Buggy nsIVariant set wrong type: 'dt == nsIDataType::VTYPE_DOMSTRING', file DataCollector/DataCollector.cpp, line 34


Expected Results:  
The assertion should not trip.

Comment 1

[Andrew Miller]

Attachment: Make SetAsDOMString set the type to nsIDataType::VTYPE_DOMSTRING

Comment 2

[Doug Turner (:dougt)]

sounds like the right thing to do. Maybe you want to copy SetFromAString() into this method to avoid assigning mType twice?

Comment 3

[Andrew Miller]

Attachment: Implement SetAsDOMString so it sets the correct type

New attachment copies the implementation, as suggested by [e-mail]

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 228889 [details] [diff] [review]
Implement SetAsDOMString so it sets the correct type

sr=me (requested over IRC, please use the request flag feature in the future).

/be

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in xpcom/ds/nsVariant.cpp;
/cvsroot/mozilla/xpcom/ds/nsVariant.cpp,v  <--  nsVariant.cpp
new revision: 1.29; previous revision: 1.28
done

Comment 6

[Andrew Miller]

> <reed> A1kmm: Make sure you leave a comment about (any) risks involved in taking a patch on the branch and what exactly it does.

The patch makes nsVariant::SetAsDOMString set the type to VTYPE_DOMSTRING (it previously incorrectly set it to VTYPE_ASTRING).

The patch is relatively safe, because nothing in the standard tree actually calls this method, and it can only be called from C++ (therefore, it will make a difference to extensions).

The patch is especially worth having on the branch because it fixes something to behave consistently, and it otherwise would be hard to use canvas from C++.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 228889 [details] [diff] [review]
Implement SetAsDOMString so it sets the correct type

Seems a little dangerous in case somebody's depending on the bug, but that seems relatively unlikely.

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword once you have done so.