243904 - SOAP code problem with null, unpositioned array item

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[Cédric Chantepie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040113

Even if in the SOAP response from Webservice there is the attribute 'nil="true"'
for a param, this param will be an empty string rather than a null one in when
trying to use it in javascript code.

Reproducible: Always
Steps to Reproduce:
1. Call a SOAP operation that returns an object 'defaultLocale' with 'variant'
property null.
2. In javascript test whether '(defaultLocale.variant == null)' is true
Actual Results:  
defaultLocale.variant = ""

Expected Results:  
defaultLocale.variant = null

Comment 1

[timeless]

Attachment: untested patch for consideration

Comment 2

[Harshal Pradhan]

Comment on attachment 148739 [details] [diff] [review]
untested patch for consideration

It might take me upto a week to get to around to reviewing this.

Comment 3

[Cédric Chantepie]

Patch doesn't seems to work with firefox 0.8+ debug build

Comment 4

[Harshal Pradhan]

Comment on attachment 148739 [details] [diff] [review]
untested patch for consideration

Cancelling review request till we figure out why this doesn't work.

Comment 5

[Cédric Chantepie]

Attachment: testcase

Please read included 'README' file

Comment 6

[Cédric Chantepie]

*** This bug has been marked as a duplicate of 241157 ***

Comment 7

[Cédric Chantepie]

Attachment: Patch using null variant with support for apache SOAP xsi:nil

Still have some case where SOAP null values aren't supported, see following
testcase

Comment 8

[Cédric Chantepie]

Attachment: testcase (20040705)

To give it a try please first add 213.41.158.81 for ipage-test.corsaire.fr in
your host file.

Comment 9

[Cédric Chantepie]

Stacktrace for the NS_ERROR_CANNOT_CONVERT_DATA when null property should be string

#0  nsDebugImpl::Break(char const*, int) (this=0x1116fd0, aFile=0x1c9a27c
"nsVariant.cpp", aLine=804) at nsDebugImpl.cpp:283
#1  0x01c598c4 in nsDebug::Break(char const*, int) (aFile=0x1c9a27c
"nsVariant.cpp", aLine=804) at nsDebug.cpp:91
#2  0x01bc6c2c in ToString(nsDiscriminatedUnion const&, nsACString&)
(data=@0x10671d0c, outString=@0xbfffc890) at nsVariant.cpp:804
#3  0x01bc709c in nsVariant::ConvertToAString(nsDiscriminatedUnion const&,
nsAString&) (data=@0x10671d0c, _retval=@0xbfffcf80) at nsVariant.cpp:889
#4  0x01bca670 in nsVariant::GetAsAString(nsAString&) (this=0x10671d00,
_retval=@0xbfffcf80) at nsVariant.cpp:1805
#5  0x1e3e0048 in WSPProxy::VariantToValue(unsigned char, void*,
nsIInterfaceInfo*, nsIVariant*) (aTypeTag=15 '\017', aValue=0xbfffcf80,
aInterfaceInfo=0x0, aProperty=0x10671d00) at wspproxy.cpp:998
#6  0x1e3e92fc in WSPPropertyBagWrapper::CallMethod(unsigned short,
nsXPTMethodInfo const*, nsXPTCMiniVariant*) (this=0x10808f10, methodIndex=5,
info=0x2b84840, params=0xbfffcc00) at wsppropertybagwrapper.cpp:167
#7  0x01c494f0 in PrepareAndDispatch (self=0x10808f10, methodIndex=5,
args=0xbfffcdc8, gprData=0xbfffcd0c, fprData=0xbfffcd28) at
xptcstubs_ppc_rhapsody.cpp:234
#8  0x01c4a058 in SharedStub () at nsTObsoleteAStringThunk.cpp:51
#9  0x01c49818 in _XPTC_InvokeByIndex () at nsTObsoleteAStringThunk.cpp:51
#10 0x01c48b50 in XPTC_InvokeByIndex (that=0x10808f10, methodIndex=5,
paramCount=1, params=0xbfffcef0) at xptcinvoke_ppc_rhapsody.cpp:144
#11 0x030483fc in XPCWrappedNative::CallMethod(XPCCallContext&,
XPCWrappedNative::CallMode) (ccx=@0xbfffd1e0, mode=CALL_GETTER) at
xpcwrappednative.cpp:2027

Comment 10

[Johnny Stenback (:jst)]

Attachment: Would this fix it?

Comment 11

[timeless]

seems reasonable. this should probably be testable from xpcshell...

Comment 12

[Cédric Chantepie]

(In reply to comment #10)
> Created an attachment (id=152628)
> Would this fix it?
> 

no :-)

Comment 13

[Cédric Chantepie]

Attachment: Patch supporting xsi:nil for apache SOAP and working for null string

Patch also fix bug for one dimension array from apache SOAP that are unnumbered
(no explicit "position" attribute).

Comment 14

[Cédric Chantepie]

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

Comment 15

[Cédric Chantepie]

Comment on attachment 152690 [details] [diff] [review]
Patch supporting xsi:nil for apache SOAP and working for null string

Also fix unnumbered one dimension array (item without "position" attribute)

Comment 16

[Mike Connor [:mconnor]]

wait'll we have reviews on this before we look at putting it on the branch.

Comment 17

[Cédric Chantepie]

Attachment: Patch proposal that work on aviary

Fixes null value problem testing apache SOAP xsi:nil and supportings soap null
string as empty string. Fixes problem about unpositionned items of one
dimension array so that it works. Temporarily fix problem about inheritance
getting explicit type from soap response DOM element (xsi:type), inheritance
support should be enhanced. Maid from AVIARY, I'll submit a patch for trunk as
soon as I can.

Comment 18

[Cédric Chantepie]

Attachment: Patch for trunk

Comment 19

[Cédric Chantepie]

Attachment: Patch including changes for nsVariant::ToString [trunk]

Changes nsVariant::ToString to supports null string.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 156586 [details] [diff] [review]
Patch including changes for nsVariant::ToString [trunk]

- In HandleNull():

+  if (aNullAttr.Equals(gSOAPStrings->kTrue)
+      || aNullAttr.Equals(gSOAPStrings->kTrueA)) {
...
+    if (typeName.Equals(NS_LITERAL_STRING("string")) ||
+	 typeName.Equals(NS_LITERAL_STRING("normalizedString"))) {

Inconsistent operator placement, pick one (I'd prefer operators at the end of
the line in stead of at the beginning).

+	 nsCOMPtr<nsIVariant> variant = do_QueryInterface(strVar);
+	 
+	 *_retval = variant;
+	 
+	 NS_ADDREF(*_retval);
+	 
+	 return NS_OK;

How about replacing the above with the single line:

+	 return CallQueryInterface(strVar, _retval);

... and

+      }
+      else {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }

remove the else-after-return. How about:

+      nsCOMPtr<nsIWritableVariant>
strVar(do_CreateInstance("@mozilla.org/variant;1"));
+      
+      if (!strVar) {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }
+
+      return CallQueryInterface(strVar, _retval);

... and then...

+    }
+    
+    else {

One more else-after-return.

+      if (nullVariant) {
+	 nullVariant->SetAsISupports(nsnull);
+	 nsCOMPtr<nsIVariant> variant = do_QueryInterface(nullVariant);
+	 *_retval = variant;
+	 
+	 NS_ADDREF(*_retval);
+	 
+	 return NS_OK;

Same thing:

+	 return CallQueryInterface(varian, _retval);

...
+      }
+      
+      return NS_ERROR_OUT_OF_MEMORY;
+    }

... and flip this around too to return the error in the exception case, and let
the normal flow go to the end of this scope (like I suggested above).

+  NS_ERROR("How have you get here ?");

"How did you get here?"

- In GetExplicitType():

...
+    rc = schemaLoader->GetType(name, ns, getter_AddRefs(type));
+
+    if (NS_SUCCEEDED(rc) && type) {
+      *_retval = type;
+
+      NS_ADDREF(*_retval);
+
+      return NS_OK;
+    }
+
+    else {
+      *_retval = nsnull;
+
+      return rc;
+    }

else-after-return, and this could be simplified to:

+    NS_IF_ADDREF(*_retval = type);
+
+    return rc;

Note the _IF_ in NS_IF_ADDREF().

+  NS_ERROR("::GetExplicitType: Wow how do you get here");

See above.

- In nsVariant.cpp

+	 outString.Assign("");
+	 outString.Truncate();

No need to assign an empty string into a string just before truncating it.

sr=jst with those changes, please fix that, and request review from
[e-mail]

Comment 21

[David Bradley]

Will all loaders past and present be required to provide a schema collection?
Just wondering if interface inheritance is the right solution. Admittedly I'm
not familiar with the WSDL object structure, so take that into consideration.
I'm more trying to gain knowledge than suggest an alternative. Maybe the naming
is messing me up, as I wouldn't expect a loader to be a collection and QI to the
collection, I would have expected to ask it via a method for a collection of things.

Comment 22

[Cédric Chantepie]

Attachment: Fixed trunk patch

Comment 23

[Cédric Chantepie]

Attachment: Fixed trunk patch

Comment 24

[Cédric Chantepie]

Attachment: Patch for trunk with changes in soap decoder

Comment 25

[Cédric Chantepie]

Attachment: Updated patch for trunk

Comment 26

[Cédric Chantepie]

Comment on attachment 172048 [details] [diff] [review]
Updated patch for trunk

With modifications from patch of bug #254897

Comment 27

[Cédric Chantepie]

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

Comment 28

[Cédric Chantepie]

Comment on attachment 172048 [details] [diff] [review]
Updated patch for trunk

+#include "nsIWSPInterfaceInfoService.h"
Is not used for this patch

Comment 29

[Cédric Chantepie]

Attachment: Patch without unneeded include [trunk]

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 172506 [details] [diff] [review]
Patch without unneeded include [trunk]

- In HandleNull():

+      nsAutoString strVal;
+      strVal.Truncate();

No need to truncate that string, it's already empty.

+      nsCOMPtr<nsIWritableVariant>
strVar(do_CreateInstance("@mozilla.org/variant;1"));
+     
+      if (strVar) {
+	 strVar->SetAsAString(strVal);
+
+	 nsCOMPtr<nsIVariant> variant = do_QueryInterface(strVar);
+
+	 *aResult = variant;
+
+	 NS_ADDREF(*aResult);
+
+	 return NS_OK;
+      }
+      else {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }

else-after-return. And how about flipping that around to returning early if
we're out of memory... i.e.:

+      nsCOMPtr<nsIWritableVariant>
strVar(do_CreateInstance("@mozilla.org/variant;1"));
+     
+      if (!strVar) {
+	 return NS_ERROR_OUT_OF_MEMORY;
+      }
+
+      strVar->SetAsAString(strVal);
...

+      nsCOMPtr<nsIWritableVariant>
nullVariant(do_CreateInstance("@mozilla.org/variant;1"));
+     
+      if (nullVariant) {
+	 nullVariant->SetAsISupports(nsnull);
+	 nsCOMPtr<nsIVariant> variant = do_QueryInterface(nullVariant);
+	 *aResult = variant;
+
+	 NS_ADDREF(*aResult);
+
+	 return NS_OK;
+      }
+     
+      return NS_ERROR_OUT_OF_MEMORY;

Same thing, return NS_ERROR_OUT_OF_MEMORY if (!nullVariant).

sr=jst wiht that changed.

Comment 31

[Cédric Chantepie]

Attachment: Changed patch

With changes from jst comment

Comment 32

[Cédric Chantepie]

If is ok with jst changes could anyone commit that

Comment 33

[Cédric Chantepie]

If is ok with jst changes could anyone commit that

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 174744 [details] [diff] [review]
Changed patch

This still needs a review, requsting from Harshal...

Comment 35

[Harshal Pradhan]

Comment on attachment 174744 [details] [diff] [review]
Changed patch

>Index: extensions/webservices/public/nsISchemaLoader.idl

>-[scriptable, uuid(9B2F0B4A-8F00-4a78-961A-7E84ED49B0B6)]
>-interface nsISchemaLoader : nsISupports {
>+[scriptable, uuid(77061d1d-e191-11d8-a3cc-000393b6661a)]
>+interface nsISchemaLoader : nsISchemaCollection {
>   nsISchema load(in AString schemaURI);
>   void loadAsync(in AString schemaURI, 
>                  in nsISchemaLoadListener listener);
>   nsISchema processSchemaElement(in nsIDOMElement element,
>                                  in nsIWebServiceErrorHandler aErrorHandler);
> };
> 

I have reservations about this change. We already have
a somewhat broken object model in the schema code. I
don't think that making nsISchemaLoader inherit from
nsISchemaCollection makes things any better. Some of the
functions that nsSchemaLoader performs are quite orthogonal
to each other. If someone wanted to clean that up in the
future, this change would make things more difficult.
Also, I am told there are also external users of this interface
in the wild, so we have to be careful when extending its
functionality.

Besides, as far as I can see, the only reason for this
change is to save one QI in the SOAP code. I don't think
that this is enough justifaction. Just do the extra QI to
nsISchemaCollection where you need it. (Or am I missing
something here ?)


>Index: extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp

>+static nsresult HandleNull(nsISOAPEncoding* aEncoding,
>+                           nsIDOMElement* aSource,
>+                           nsISchemaType* aSchemaType,
>+                           nsISOAPAttachments* aAttachments,
>+                           nsAutoString aNullAttr,
>+                           nsIVariant** aResult)
>+{

[snip]

>+   
>+    if (aSchemaType &&
>+        (typeName.Equals(NS_LITERAL_STRING("string")) ||
>+         typeName.Equals(NS_LITERAL_STRING("normalizedString")))) {
>+     
>+      nsAutoString strVal;
>+      strVal.SetIsVoid(true);
>+     
>+      nsCOMPtr<nsIWritableVariant> strVar(do_CreateInstance("@mozilla.org/variant;1"));
>+     
>+      if (!strVar) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+       
>+      strVar->SetAsAString(strVal);
>+
>+      nsCOMPtr<nsIVariant> variant = do_QueryInterface(strVar);
>+      
>+      NS_ADDREF(*aResult = variant);
>+
>+      return NS_OK;
>+    } else {
>+      nsCOMPtr<nsIWritableVariant> nullVariant(do_CreateInstance("@mozilla.org/variant;1"));
>+     
>+      if (!nullVariant) {
>+        return NS_ERROR_OUT_OF_MEMORY;
>+      }
>+       
>+      nullVariant->SetAsISupports(nsnull);
>+      nsCOMPtr<nsIVariant> variant = do_QueryInterface(nullVariant);
>+      
>+      NS_ADDREF(*aResult = variant);
>+      
>+      return NS_OK;


Move the construction of the variant out of the if so that you
only do it once. Also since nsIVariant inherits from nsIWritableVariant
I suspect that you dont need to QI it when you assign it to an nsIVariant.

So, basically something like 

      nsCOMPtr<nsIWritableVariant>
var(do_CreateInstance("@mozilla.org/variant;1"));
      if (!var) {
	return NS_ERROR_OUT_OF_MEMORY;
      }
      if (stringtype) {
	do_string_stuff;
      } else {
	interface_stuff;
      }
      NS_ADDREF(*aResult = variant);

This will be somewhat smaller code.

[snip]

>+static nsresult DecodeStructParticle(nsISOAPEncoding* aEncoding, 
>+                                     nsIDOMElement* aElement, 
>+                                     nsISchemaParticle* aParticle, 
>+                                     nsISOAPAttachments * aAttachments, 
>+                                     nsISOAPPropertyBagMutator* aDestination,
>+                                     nsIDOMElement** aResult)
> {

[snip]


>           nsCOMPtr<nsIDOMElement> next = aElement;
>           while (particleCount > 0) {
>             for (i = 0; i < particleCount; i++) {
>-              child = dont_AddRef(NS_STATIC_CAST
>-                    (nsISchemaParticle*, all->ElementAt(i)));
>+              nsAutoString name;
>+              rc = next->GetTagName(name);
>+              NS_ENSURE_SUCCESS(rc, rc);
>+
>+              // Get schema particle according SOAP element
>+              groupParticles.Get(name, getter_AddRefs(child));
>+              
>+              if (NS_FAILED(rc) || !child) {
>+                nsAutoString msg(NS_LITERAL_STRING("::DecodeStructParticle: "));
>+                msg.AppendLiteral("Cannot find schema particle for \"");
>+                msg.Append(name);
>+                msg.AppendLiteral("\"");
>+                NS_ERROR(NS_ConvertUTF16toUTF8(msg).get());

Return this error. Or if you really want to ignore it, then put it in
a #ifdef DEBUG. There is no point in paying for the extra codesize in
release builds.


r=me if you revert the schema interface change and fix above nits.

The changes in the SOAP decoder are somewhat scary. Unfortunatly, I don't
even have a build right now and am unable to test anything. You have been
carrying this patch for a while, so I am going to assume that it must
have been tested, and my r= above is based on this. Please make
sure that you do sufficient testing with different some live servers 
before this gets checked in.

And finally, thanks for your patience on this bug .... I'm sorry for the
long wait on reviews for your patches.

Comment 36

[Cédric Chantepie]

Attachment: Patch without nsISchemaLoader change and some cleanup

Comment 37

[Doron Rosenberg (IBM)]

I still see schemaLoader changes in the patch.

Comment 38

[Cédric Chantepie]

Attachment: Without schemaloader change

Comment 39

[Doron Rosenberg (IBM)]

Comment on attachment 181912 [details] [diff] [review]
Without schemaloader change

I tested a bit and doesn't seem to break anything.

Comment 40

[Asa Dotzler [:asa]]

Comment on attachment 181912 [details] [diff] [review]
Without schemaloader change

a=asa

Comment 41

[timeless]

Comment on attachment 181912 [details] [diff] [review]
Without schemaloader change

2005-05-02 12:42:
mozilla/extensions/webservices/soap/src/nsDefaultSOAPEncoder.cpp	1.93
mozilla/extensions/webservices/soap/src/nsSOAPUtils.cpp 	1.39
mozilla/extensions/webservices/soap/src/nsSOAPUtils.h	1.26
mozilla/xpcom/ds/nsVariant.cpp	1.27