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

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: andrew, Assigned: andrew)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 228770 [details] [diff] [review]
Make SetAsDOMString set the type to nsIDataType::VTYPE_DOMSTRING
Attachment #228770 - Flags: review?(dougt)
sounds like the right thing to do. Maybe you want to copy SetFromAString() into this method to avoid assigning mType twice?
Assignee: nobody → ak.miller
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

13 years ago
Attachment #228770 - Flags: review?(dougt)
(Assignee)

Comment 3

13 years ago
Created attachment 228889 [details] [diff] [review]
Implement SetAsDOMString so it sets the correct type

New attachment copies the implementation, as suggested by dougt@meer.net
Attachment #228770 - Attachment is obsolete: true
Attachment #228889 - Flags: review?(dougt)

Updated

13 years ago
Attachment #228889 - Flags: review?(dougt) → review+
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
Attachment #228889 - Flags: superreview+
(Assignee)

Updated

13 years ago
Whiteboard: [checkin needed]
Status: NEW → ASSIGNED
Checking in xpcom/ds/nsVariant.cpp;
/cvsroot/mozilla/xpcom/ds/nsVariant.cpp,v  <--  nsVariant.cpp
new revision: 1.29; previous revision: 1.28
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
(Assignee)

Updated

13 years ago
Attachment #228889 - Flags: approval1.8.1?
(Assignee)

Comment 6

13 years ago
> <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 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.
Attachment #228889 - Flags: approval1.8.1? → approval1.8.1+
You need to log in before you can comment on or make changes to this bug.