Closed Bug 241157 Opened 17 years ago Closed 17 years ago

nsDefaultEncoder::Decode does not handle null valued elements correctly

Categories

(Core Graveyard :: Web Services, defect)

defect
Not set
major

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)

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 :-(
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8alpha
Attached patch patch to fixSplinter Review
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
Attachment #148394 - Flags: superreview?(jst)
Attachment #148394 - Flags: review?(darin)
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 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.
jst: looks like you are correct.
Attachment #148471 - Flags: superreview?(darin)
Attachment #148471 - Flags: review?(bugs)
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+
Comment on attachment 148471 [details] [diff] [review]
Let variants contain null interface pointers.

r=ben@mozilla.org
Attachment #148471 - Flags: review?(bugs) → review+
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;
>     }
> }
(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.
jst: yeah, good point.  multiple inheritance :(
Landed both patches on the trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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?
Attachment #148471 - Flags: approval1.7.1?
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 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+
*** Bug 243904 has been marked as a duplicate of this bug. ***
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).
*** Bug 248923 has been marked as a duplicate of this bug. ***
Attachment #152213 - Flags: review?
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.
The patch is not checkined yet to AVIARY_1_0_20040515_BRANCH.
Please checkin to AVIARY_1_0_20040515_BRANCH.
Comment on attachment 152213 [details] [diff] [review]
Small modifications for apache SOAP

View bug #243904
Attachment #152213 - Flags: review?
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?
nsVariant changes landed on the aviary branch.
Keywords: fixed-aviary1.0
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.
Flags: blocking-aviary1.0PR?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.