244952 - Crash creating multiple proxy instance for the same webservice

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

When I try to use multiple instance of webservice proxy for the same WSDL (even
if each instance is in different chrome window), it crashes mozilla

Reproducible: Always
Steps to Reproduce:
1. Instanciate the first ws proxy instance
2. Instanciate the 2nd ws proxy instance
3. when the onLoad callback function of the 2nd instance get called it crashes

Actual Results:  
crash

Expected Results:  
works :-)

0x1286ddec in nsSchemaComponentBase::GetTargetNamespace(nsAString&) (
    this=0x1201b368, aTargetNamespace=@0x22b288)
    at c:/Docume~1/cchantepie.INTRANET/mozilla/extensions/webservices/schema/src
/nsSchemaComponentBase.cpp:61
61          return mSchema->GetTargetNamespace(aTargetNamespace);

Comment 1

[Harshal Pradhan]

Could you please provide a testcase ?

Comment 2

[Cédric Chantepie]

Attachment: testcase

Change src for js in the xul file and put all in a chrome app for testing

Comment 3

[Cédric Chantepie]

IP Address for webservices server is 213.41.168.38 (port 52080)
Doesn't forget to add map this ip to ipage-test.corsaire.fr as webservices server
needs it to be accessed through virtual host. Plz tell me if works.

Comment 4

[Cédric Chantepie]

Sorry but the new adresse that will work as long as needed for testing is 
213.41.158.81 (port 52080).

Comment 5

[Harshal Pradhan]

Thanks for the testcase. Purify says this ... 

[E] IPR: Invalid pointer read in
nsSchemaComponentBase::GetTargetNamespace(nsAString&) {1 occurrence}
        Reading 4 bytes from 0x0a2fd9e0 (4 bytes at 0x0a2fd9e0 illegal)
        Address 0x0a2fd9e0 points into a HeapAlloc'd block in unallocated region
of heap 0x02380000
        Thread ID: 0x440
        Error location
            nsSchemaComponentBase::GetTargetNamespace(nsAString&)
[nsschemacomponentbase.cpp:61]
                nsSchemaComponentBase::GetTargetNamespace(nsAString&
aTargetNamespace)
                {
                  if (mSchema) {
             =>     return mSchema->GetTargetNamespace(aTargetNamespace);
                  }
                
                  aTargetNamespace.Truncate();
            nsSchemaComplexType::GetTargetNamespace(nsAString&)
[nsschemaprivate.h:221]
                  virtual ~nsSchemaComplexType();
                
                  NS_DECL_ISUPPORTS
             =>   NS_IMPL_NSISCHEMACOMPONENT_USING_BASE
                  NS_DECL_NSISCHEMATYPE
                  NS_DECL_NSISCHEMACOMPLEXTYPE
                
            nsDefaultEncoder::Decode(nsISOAPEncoding *,nsIDOMElement
*,nsISchemaType *,nsISOAPAttachments *,nsIVariant * *)
[nsdefaultsoapencoder.cpp:1972]
            nsSOAPEncoding::Decode(nsIDOMElement *,nsISchemaType
*,nsISOAPAttachments *,nsIVariant * *) [nssoapencoding.cpp:338]
            nsSOAPBlock::GetValue(nsIVariant * *) [nssoapblock.cpp:177]
            nsSOAPParameter::GetValue(nsIVariant * *) [nssoapparameter.h:59]
            WSPCallContext::CallCompletionListener(void) [wspcallcontext.cpp:353]
            WSPCallContext::HandleResponse(nsISOAPResponse *,nsISOAPCall
*,UINT,int,int *) [wspcallcontext.cpp:153]
            nsHTTPSOAPTransportCompletion::HandleEvent(nsIDOMEvent *)
[nshttpsoaptransport.cpp:533]
            nsXMLHttpRequest::NotifyEventListeners(nsIDOMEventListener
*,nsISupportsArray *,nsIDOMEvent *) [nsxmlhttprequest.cpp:791]

Seems to me that mSchema (which is a weak pointer to something that is supposed
to own nsSchemaComponentBase) has already been deleted.

Comment 6

[Harshal Pradhan]

Looks like an ownership model issue in schema code.

peterv, any idea what might be going on here? 

Comment 7

[Peter Van der Beken [:peterv]]

The nsSchemaLoader holds nsISchema's iirc, and the nsWSDLLoader holds a
nsISchemaLoader. But beyond that, I don't know what's happening.

Comment 8

[Cédric Chantepie]

Attachment: Quick patch from harshal

Thanx a lot harshal, my mozilla no more crashes :-)

Comment 9

[Cédric Chantepie]

Attachment: Enhanced patch

"cvs diff -Npu7" patch
Prevent schema DOM element from being parse again if the schema with the same
'targetNamespace' is already loaded in the hash.

Comment 10

[Harshal Pradhan]

The problem here is (approximately) like this: 

The wsdlloader uses the schemaloader as a service. wsdl parts keep references to
schemacomponents. These are owned by a schema. The components themselves keep
weak pointers to the schema they are owned by. The schemaloader owns all the
schemas and keeps references to them in a hashtable that is keyed on the
targetnamespace.

The testcase creates two wsdlproxies for the same wsdl, one immediately after
the other. When the second wsdlproxy is created, the second wsdlloader asks the
schemaloader to load a schema thats already been loaded by the first one (they
have the same targetnamespace). The old schema is shunted out of the
schemaloader's hashtable, and new one is created. The old schema now doesn't
have anybody holding on to it and is thus destroyed; leaving the first proxy
holding on to references to schemacomponents that are pointing to a deleted
schema. This leads to crashes while decoding the soap message.

The patch that I mailed Cedric (and he attached above) prevented the crash by
not creating a new schema if an old one is present for a particular target
namespace. Cedric's patch seems to do essentially the same thing, but more
efficiently. But I don't know if this is really the right approach. What happens
if the wsdl gets updated and the schema changes? We won't pick up the changes
till mozilla is restarted. Should the schemaloader be a service? The SOAP code
in places just |CreateInstance|'s it. Maybe the wsdl code should do the same. I
really don't know enough to be able to say what the right solution is. Hmmmm...

Comment 11

[Cédric Chantepie]

> if the wsdl gets updated and the schema changes? We won't pick up the changes
> till mozilla is restarted. Should the schemaloader be a service? The SOAP code
> in places just |CreateInstance|'s it. Maybe the wsdl code should do the same. I
> really don't know enough to be able to say what the right solution is. Hmmmm...

It seems that when we change the WSDL content it's recommanded to change the
targetNamespace in order to indicate WSDL version so that the changed WSDL file
will not fit the ancient targetNamespace and will be loaded.

I think using this WSDL version to allow to have some cache and also allow to 
change WSDL. I haven't find more information about the strict syntax that should
be used to add version information into targetNamespace.

Something I don't really understand is why there is schema hash in nsWSDLLoader
whereas there is another hash in nsSchemaLoader which seems more relevant.

Comment 12

[Cédric Chantepie]

Attachment: Patch for AVIARY_1_0_20040515_BRANCH

Comment 13

[Cédric Chantepie]

Comment on attachment 151222 [details] [diff] [review]
Patch for AVIARY_1_0_20040515_BRANCH

Fixes multi wsproxy instances crashes using Apache SOAP as server (with .Net in
some cases)

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 151222 [details] [diff] [review]
Patch for AVIARY_1_0_20040515_BRANCH

+  // Schema for given target namespace has already been 
+  if (os) {

Looks like that comment was cut short.

...
+    *_retval = os;
+  } // end of if
+  
+  // Load schema for this new target namespace
+  else {

This is asking for code to be inserted between the if and else. Please rewrite
that as:

...
+    *_retval = os;
+  } else {
+    // Load schema for this new target namespace

And next time, please attach a diff -w version for review, much easier to see
what really changed in cases like this with a whitespace independent diff.

r+sr=jst

Comment 15

[Mike Connor [:mconnor]]

cedric,  if you could attach a final patch with jst's review comments, I can
land this on the aviary branch.

Comment 16

[Cédric Chantepie]

Attachment: Reviewed patch

Comment 17

[Harshal Pradhan]

Cederic, could you please also post a diff that applies cleanly to trunk so that
someone can check this in for you. 

Comment 18

[Mike Connor [:mconnor]]

cedric: this patch is against an old cvs version from March, which means the
patch is already bitrotted.  Please unbitrot and submit the patch again :/

Comment 19

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

Oh, I am so there. 

I landed Cedric's Aviary patch since it fixed an update service crash I was
experiencing. 

Comment 20

[Cédric Chantepie]

Attachment: Trunk patch

Comment 21

[Harshal Pradhan]

Comment on attachment 154398 [details] [diff] [review]
Trunk patch

r=me. I think its safe to assume that jst's sr still applies (since this is
basically the same patch.)

Comment 22

[Johnny Stenback (:jst)]

Fix checked in on the trunk now too. Marking bug FIXED.

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 151222 [details] [diff] [review]
Patch for AVIARY_1_0_20040515_BRANCH

a=asa for 1.7.x checkin.