Closed
Bug 241157
Opened 20 years ago
Closed 20 years ago
nsDefaultEncoder::Decode does not handle null valued elements correctly
Categories
(Core Graveyard :: Web Services, defect)
Core Graveyard
Web Services
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8alpha1
People
(Reporter: darin.moz, Assigned: bugs)
References
Details
(Keywords: fixed-aviary1.0, fixed1.7.5)
Attachments
(3 files)
2.49 KB,
patch
|
darin.moz
:
review+
jst
:
superreview+
mkaply
:
approval1.7.5+
|
Details | Diff | Splinter Review |
3.06 KB,
patch
|
bugs
:
review+
darin.moz
:
superreview+
mkaply
:
approval1.7.5+
|
Details | Diff | Splinter Review |
14.13 KB,
patch
|
Details | Diff | Splinter Review |
nsDefaultEncoder::Decode does not handle null valued elements correctly. The code that tries to handle xsi:null="true" does not do a very good job. The NS_ENSURE_ARG_POINTER(aValue) in nsSOAPPropertyBag::AddProperty is hit. It seems that the decoder should construct a nsIVariant with a value that best represents null when reflected into JS. Right now, we end up passing NULL to AddProperty, and it fails, which causes decoding to fail entirely :-(
Reporter | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8alpha
Assignee | ||
Comment 1•20 years ago
|
||
If the value is null, then construct a variant object and set its value to null using SetAsISupports(nsnull) (approach recommended by jst)
Assignee: darin → bugs
Assignee | ||
Updated•20 years ago
|
Attachment #148394 -
Flags: superreview?(jst)
Attachment #148394 -
Flags: review?(darin)
Reporter | ||
Comment 2•20 years ago
|
||
Comment on attachment 148394 [details] [diff] [review] patch to fix >Index: soap/src/nsDefaultSOAPEncoder.cpp > nsCOMPtr<nsIVariant> value; ... >+ if (!value) { >+ nsCOMPtr<nsIWritableVariant> nullVariant(do_CreateInstance("@mozilla.org/variant;1")); >+ if (nullVariant) { >+ nullVariant->SetAsISupports(nsnull); >+ value = do_QueryInterface(nullVariant); >+ } >+ } nullVariant is a nsIVariant, so the QI to nsIVariant is redundant. this should be sufficient: value = nullVariant; r=darin with that change.
Attachment #148394 -
Flags: review?(darin) → review+
Comment 3•20 years ago
|
||
Comment on attachment 148394 [details] [diff] [review] patch to fix Doesn't this end up calling: http://lxr.mozilla.org/mozilla/source/xpcom/ds/nsVariant.cpp#1494 and failing with an NS_ERROR_NULL_POINTER error? If so, I can't see how this works as intended. If that's the case, I've got a patch to fix the variant code to deal with null interface pointers that I can clean up and attach.
Reporter | ||
Comment 4•20 years ago
|
||
jst: looks like you are correct.
Comment 5•20 years ago
|
||
Updated•20 years ago
|
Attachment #148471 -
Flags: superreview?(darin)
Attachment #148471 -
Flags: review?(bugs)
Comment 6•20 years ago
|
||
Comment on attachment 148394 [details] [diff] [review] patch to fix sr=jst for this change if taken with the other change I just attached.
Attachment #148394 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 7•20 years ago
|
||
Comment on attachment 148471 [details] [diff] [review] Let variants contain null interface pointers. r=ben@mozilla.org
Attachment #148471 -
Flags: review?(bugs) → review+
Reporter | ||
Comment 8•20 years ago
|
||
Comment on attachment 148471 [details] [diff] [review] Let variants contain null interface pointers. > nsVariant::ConvertToISupports(const nsDiscriminatedUnion& data, > nsISupports **_retval) ... >+ return data.u.iface.mInterfaceValue-> >+ QueryInterface(NS_GET_IID(nsISupports), (void**)_retval); This QueryInterface seems like it shouldn't be necessary since mInterfaceValue is already a nsISupports. XPCOM promises QI(A)->A, right? ;-) sr=darin
Attachment #148471 -
Flags: superreview?(darin) → superreview+
Comment on attachment 148471 [details] [diff] [review] Let variants contain null interface pointers. > nsVariant::ConvertToISupports(const nsDiscriminatedUnion& data, > nsISupports **_retval) ... > { > case nsIDataType::VTYPE_INTERFACE: > case nsIDataType::VTYPE_INTERFACE_IS: >+ if (data.u.iface.mInterfaceValue) { >+ return data.u.iface.mInterfaceValue-> >+ QueryInterface(NS_GET_IID(nsISupports), (void**)_retval); >+ } else { else after return >+ *_retval = nsnull; >+ return NS_OK; >+ } > default: > return NS_ERROR_CANNOT_CONVERT_DATA; > } > }
Comment 10•20 years ago
|
||
(In reply to comment #8) > (From update of attachment 148471 [details] [diff] [review]) > > nsVariant::ConvertToISupports(const nsDiscriminatedUnion& data, > > nsISupports **_retval) > ... > >+ return data.u.iface.mInterfaceValue-> > >+ QueryInterface(NS_GET_IID(nsISupports), (void**)_retval); > > This QueryInterface seems like it shouldn't be necessary since > mInterfaceValue is already a nsISupports. Yes, mInterfaceValue is of type nsISupports, but the interface it points to is not necessarily the identity pointer for the object, so when a non-nsISupports XPCOM interface pointer is converted to a true nsISupports pointer, QI'ing for identity consistency is IMO the right thing to do. else-after-return removed, thanks.
Reporter | ||
Comment 11•20 years ago
|
||
jst: yeah, good point. multiple inheritance :(
Comment 12•20 years ago
|
||
Landed both patches on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 13•20 years ago
|
||
Comment on attachment 148394 [details] [diff] [review] patch to fix I'd like these patches for the 1.7 branch, as without them, some SOAP stuff I'm working on is totally busted.
Attachment #148394 -
Flags: approval1.7.1?
Updated•20 years ago
|
Attachment #148471 -
Flags: approval1.7.1?
Comment 14•20 years ago
|
||
Comment on attachment 148394 [details] [diff] [review] patch to fix a=mkaply for 1.7.1
Attachment #148394 -
Flags: approval1.7.1? → approval1.7.1+
Comment 15•20 years ago
|
||
Comment on attachment 148471 [details] [diff] [review] Let variants contain null interface pointers. a=mkaply for 1.7.1
Attachment #148471 -
Flags: approval1.7.1? → approval1.7.1+
Updated•20 years ago
|
Keywords: fixed1.7.1
Comment 16•20 years ago
|
||
*** Bug 243904 has been marked as a duplicate of this bug. ***
Comment 17•20 years ago
|
||
Modification to make encoder handle "xsi:null" attribute sent by Apache SOAP (not only "xsi:null"). Fixes some problems about unnumbered array of one dimension (without explicit "position" attribute on item in SOAP response).
Comment 18•20 years ago
|
||
*** Bug 248923 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Attachment #152213 -
Flags: review?
Reporter | ||
Comment 19•20 years ago
|
||
Cédric, Thank you for preparing this patch, but could you please file a new bug to track checking in your patch? This bug is already resolved (for some time now), and it would complicate things to deal with both patches in the same bug. Thanks.
Comment 20•20 years ago
|
||
The patch is not checkined yet to AVIARY_1_0_20040515_BRANCH. Please checkin to AVIARY_1_0_20040515_BRANCH.
Comment 21•20 years ago
|
||
Comment on attachment 152213 [details] [diff] [review] Small modifications for apache SOAP View bug #243904
Attachment #152213 -
Flags: review?
Reporter | ||
Comment 22•20 years ago
|
||
yeah, it looks like only the first patch made it onto the aviary 1.0 branch. we should probably land the second one too.
Flags: blocking-aviary1.0RC1?
Whiteboard: needed-aviary1.0?
Updated•20 years ago
|
Whiteboard: needed-aviary1.0?
darin's review comment (comment #2) was ignored for the version of the patch landed on the trunk and the 1.7 branch; it was taken only for the aviary branch. (The issue he mentioned occurs in 2 places.)
The else-after-return mentioned in comment 9 was not addressed when landing on the 1.7 branch, but was when landing on the aviary branch. I didn't check the trunk. Things like this make doing merging between branches harder.
Updated•20 years ago
|
Flags: blocking-aviary1.0PR?
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•