337179 - Schema with forward type definitions causing load errors

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[Steve Speicher]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1) Opera 7.55  [en] (IBM EVV/3.8/EAK01AGF/LE)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060508 Minefield/3.0a1

Schema definition like this (complexType forward referencing another complexType) is the cause of the problem:
  <complexType name="fooType">
    <simpleContent>
      <restriction base="tns:textType">
        <maxLength value="2" />
      </restriction>
    </simpleContent>
  </complexType>

  <complexType name="textType">
    <simpleContent>
      <extension base="string">
      </extension>
    </simpleContent>
  </complexType>


Reproducible: Always

Comment 1

[Steve Speicher]

Attachment: test case

Comment 2

[Steve Speicher]

Attachment: driver XForm with link to schema with problem

Comment 3

[Steve Speicher]

Attachment: schema that works

Comment 4

[Doron Rosenberg (IBM)]

Attachment: le patch

this should fix it.  Steve, can you test with your larger forms?

Parts of the code are not schema placeholder (if a type is defined after a user is found) compliant, this makes them behave again.

Comment 5

[Steve Speicher]

(In reply to comment #4)

> Steve, can you test with your larger forms?
> 

Yes and along with patch for bug 337179, works nicely.

Comment 6

[Steve Speicher]

(In reply to comment #5)
> (In reply to comment #4)
> 
> > Steve, can you test with your larger forms?
> > 
> 
> Yes and along with patch for bug 337179, works nicely.
> 

I meant with bug 335366 (though not strictly dependent on it)

Comment 7

[aaronr]

Comment on attachment 221501 [details] [diff] [review]
le patch


>Index: extensions/webservices/schema/src/nsSchemaLoader.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/webservices/schema/src/nsSchemaLoader.cpp,v
>retrieving revision 1.43.2.3
>diff -u -p -1 -0 -r1.43.2.3 nsSchemaLoader.cpp
>--- extensions/webservices/schema/src/nsSchemaLoader.cpp	15 Mar 2006 17:35:52 -0000	1.43.2.3
>+++ extensions/webservices/schema/src/nsSchemaLoader.cpp	9 May 2006 21:45:24 -0000
>@@ -1840,43 +1840,51 @@ nsSchemaLoader::ProcessSimpleContentRest
>  
>   restrictionInst = new nsSchemaRestrictionType(aSchema, EmptyString());
>   if (!restrictionInst) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }
>   simpleBase = restrictionInst;
>   
>   // The base type must actually be a complex type (which itself must
>   // have a simple base type.
>   nsCOMPtr<nsISchemaComplexType> complexBase = do_QueryInterface(aBaseType);
>-  if (!complexBase) {
>-    nsAutoString baseStr;
>-    rv = aBaseType->GetName(baseStr);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-
>-    nsAutoString errorMsg;
>-    errorMsg.AppendLiteral("Failure processing schema, base type \"");
>-    errorMsg.Append(baseStr);
>-    errorMsg.AppendLiteral("\" of restriction must be a complex type ");
>-    errorMsg.AppendLiteral("which itself must be based on a simple type");
>+    if (!complexBase) {
>+    // if base type is a place holder, this is ok
>+    PRUint16 schemaType;
>+    aBaseType->GetSchemaType(&schemaType);

nit: indentation

>+
>+    if (schemaType == nsISchemaType::SCHEMA_TYPE_PLACEHOLDER) {
>+      simpleBase = do_QueryInterface(aBaseType);

you don't check the return type from GetSchemaType and you don't assign an initial value to schemaType.  You like walking the razor's edge!  fix please.

>+    } else {
>+      nsAutoString baseStr;
>+      rv = aBaseType->GetName(baseStr);
>+      NS_ENSURE_SUCCESS(rv, rv);
> 
>-    NS_SCHEMALOADER_FIRE_ERROR(NS_ERROR_SCHEMA_INVALID_TYPE_USAGE, errorMsg);
>+      nsAutoString errorMsg;
>+      errorMsg.AppendLiteral("Failure processing schema, base type \"");
>+      errorMsg.Append(baseStr);
>+      errorMsg.AppendLiteral("\" of restriction must be a complex type ");
>+      errorMsg.AppendLiteral("which itself must be based on a simple type");
> 
>-    return NS_ERROR_SCHEMA_INVALID_TYPE_USAGE;
>-  }
>+      NS_SCHEMALOADER_FIRE_ERROR(NS_ERROR_SCHEMA_INVALID_TYPE_USAGE, errorMsg);
> 
>-  nsCOMPtr<nsISchemaSimpleType> parentSimpleBase;
>-  complexBase->GetSimpleBaseType(getter_AddRefs(parentSimpleBase));
>+      return NS_ERROR_SCHEMA_INVALID_TYPE_USAGE;
>+    }
>+  } else {
>+    nsCOMPtr<nsISchemaSimpleType> parentSimpleBase;
>+    complexBase->GetSimpleBaseType(getter_AddRefs(parentSimpleBase));
>   
>-  if (parentSimpleBase) {
>-    rv = restrictionInst->SetBaseType(parentSimpleBase);
>-    if (NS_FAILED(rv)) {
>-      return rv;
>+    if (parentSimpleBase) {
>+      rv = restrictionInst->SetBaseType(parentSimpleBase);
>+      if (NS_FAILED(rv)) {
>+        return rv;
>+      }

nit: NS_ENSURE_SUCCESS(rv, rv)

nothing major that I noticed.  r=me

Comment 8

[Allan Beaufour]

Comment on attachment 221501 [details] [diff] [review]
le patch

I do not see something obviously wrong, except for the remarks that Aaron had. But I do not really feel qualified for r+ in that part of the code....

Comment 9

[aaronr]

Comment on attachment 221501 [details] [diff] [review]
le patch

great to have for the next 1.8.0.x release

Comment 10

[Mike Schroepfer]

Comment on attachment 221501 [details] [diff] [review]
le patch

Doesn't meet criteria for RC2.   i.e. "Show Stopper Bugs" - major regression, crash, security issue.  We can pickup in a follow-on release.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 221501 [details] [diff] [review]
le patch

1.8.0.8 is to keep 1.8.0.x security fixes in-line with the FF2 release, we're not taking things they aren't.

Comment 12

[aaronr]

Comment on attachment 221501 [details] [diff] [review]
le patch

Now that FF 2 is out, can we get this in?

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 221501 [details] [diff] [review]
le patch

Doesn't appear to have a super review, This is going to have to wait.

Comment 14

[Doron Rosenberg (IBM)]

Patch was written by module owner and was simple enough to not need a sr.

Comment 15

[Reed Loden [:reed]]

(In reply to comment #14)
> Patch was written by module owner and was simple enough to not need a sr.

Rerequest approval1.8.1.1, or your message will never be seen by the drivers.

Comment 16

[Jay Patel [:jay]]

Comment on attachment 221501 [details] [diff] [review]
le patch

Given Doron's last comment, approved for 1.8 branch, a= jay for drivers.

Comment 17

[aaronr]

doesn't sound eligible for 1.8.0 branch, so removing xf-to-branch