241157 - nsDefaultEncoder::Decode does not handle null valued elements correctly

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[Darin Fisher]

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 :-(

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch to fix

If the value is null, then construct a variant object and set its value to null
using SetAsISupports(nsnull) (approach recommended by jst)

Comment 2

[Darin Fisher]

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.

Comment 3

[Johnny Stenback (:jst)]

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.

Comment 4

[Darin Fisher]

jst: looks like you are correct.

Comment 5

[Johnny Stenback (:jst)]

Attachment: Let variants contain null interface pointers.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 148394 [details] [diff] [review]
patch to fix

sr=jst for this change if taken with the other change I just attached.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 148471 [details] [diff] [review]
Let variants contain null interface pointers.

r=[e-mail]

Comment 8

[Darin Fisher]

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

Comment 9

[timeless]

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

[Johnny Stenback (:jst)]

(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.

Comment 11

[Darin Fisher]

jst: yeah, good point.  multiple inheritance :(

Comment 12

[Johnny Stenback (:jst)]

Landed both patches on the trunk.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 14

[Mike Kaply [:mkaply]]

Comment on attachment 148394 [details] [diff] [review]
patch to fix

a=mkaply for 1.7.1

Comment 15

[Mike Kaply [:mkaply]]

Comment on attachment 148471 [details] [diff] [review]
Let variants contain null interface pointers.

a=mkaply for 1.7.1

Comment 16

[Cédric Chantepie]

*** Bug 243904 has been marked as a duplicate of this bug. ***

Comment 17

[Cédric Chantepie]

Attachment: Small modifications for apache SOAP

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

[Cédric Chantepie]

*** Bug 248923 has been marked as a duplicate of this bug. ***

Comment 19

[Darin Fisher]

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

[Hiro]

The patch is not checkined yet to AVIARY_1_0_20040515_BRANCH.
Please checkin to AVIARY_1_0_20040515_BRANCH.

Comment 21

[Cédric Chantepie]

Comment on attachment 152213 [details] [diff] [review]
Small modifications for apache SOAP

View bug #243904

Comment 22

[Darin Fisher]

yeah, it looks like only the first patch made it onto the aviary 1.0 branch.  we
should probably land the second one too.

Comment 23

[Johnny Stenback (:jst)]

nsVariant changes landed on the aviary branch.

Comment 24

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

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.)

Comment 25

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

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.