309546 - Fix file submission issues

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[jhp (no longer active)]

This bug is for fixing some of the file submission issues I noticed when working
on bug 275453.

Comment 1

[jhp (no longer active)]

Attachment: patch

- I changed |ParseTypeFromNode()| to return the the actual namespace URI rather
than just the textual prefix.  This is more useful, and all of the code that
currently uses this call is doing the conversion from prefix to URI anyway.
- In |nsXFormsSubmissionElement::CopyChildren()|, the code was never going down
the path to add upload the contents of a local file.  This is because it was
calling |GetElementEncodingType()| on 'destChild', which is a clone of
'currentNode', but doesn't contain the relevant attributes.  Passing
'currentNode' fixed this issue.
- In |nsXFormsSubmissionElement::SerializeDataMultipartRelated()|, we were
never checking the return value for |SerializeDataXML()|.  So if that failed,
we would still try to submit, and crash.
- Some of the code incorrectly referenced 'base64binary', when we should be
using 'base64Binary' (noticed capitalized 'B').

Comment 2

[aaronr]

Any testcase?

Comment 3

[jhp (no longer active)]

I've only got a test case that works with the upload patch.  So this can wait
until that gets checked in.

Comment 4

[aaronr]

Comment on attachment 196958 [details] [diff] [review]
patch


>Index: nsXFormsSubmissionElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsSubmissionElement.cpp,v
>retrieving revision 1.40
>diff -u -6 -p -r1.40 nsXFormsSubmissionElement.cpp
>--- nsXFormsSubmissionElement.cpp	15 Sep 2005 11:37:37 -0000	1.40
>+++ nsXFormsSubmissionElement.cpp	21 Sep 2005 22:19:47 -0000

>@@ -1227,13 +1223,13 @@ nsXFormsSubmissionElement::CopyChildren(
>       // an attachments array, then we need to perform multipart/related
>       // processing (i.e., generate a ContentID for the child of this element,
>       // and append a new attachment to the attachments array).
> 
>       PRUint32 encType;
>       if (attachments &&
>-          NS_SUCCEEDED(GetElementEncodingType(destChild, &encType)) &&
>+          NS_SUCCEEDED(GetElementEncodingType(currentNode, &encType)) &&
>           encType == ELEMENT_ENCTYPE_URI)

nit: You should probably change the comment above this change to say
currentNode instead of destChild, right?


>Index: nsXFormsUtils.h
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/xforms/nsXFormsUtils.h,v
>retrieving revision 1.30
>diff -u -6 -p -r1.30 nsXFormsUtils.h
>--- nsXFormsUtils.h	23 Aug 2005 11:00:50 -0000	1.30
>+++ nsXFormsUtils.h	21 Sep 2005 22:19:47 -0000
>@@ -368,18 +368,20 @@ public:
>    */
>   static NS_HIDDEN_(nsresult) GetInstanceNodeForData(nsIDOMNode *aInstanceDataNode,
>                                                      nsIModelElementPrivate *aModel,
>                                                      nsIDOMNode  **aInstanceNode);
> 
>   /**
>-   * This function takes an instance data node, finds the type bound to it, and
>-   * returns the seperated out type (integer) and namespace prefix (xsd).
>+   * This function takes an node from the main document and an instance data
>+   * node, finds the type bound to it, and returns the seperated out type and
>+   * namespace URI.
>    */

Please seperate out the individual parameters to comment on each like the other
functions below it in the header file.	Might also want to spell out that
aElement can be ANY element from the XForms document and that it is used to
resolve the NS prefix to the proper NS URI as defined in the XForms document. 
Otherwise it isn't really clear what the caller should pass in for aElement.

With that, r=me

Comment 5

[jhp (no longer active)]

Attachment: patch v1.1

Patch with Aaron's suggested fixes.

Comment 6

[Allan Beaufour]

(In reply to comment #3)
> I've only got a test case that works with the upload patch.  So this can wait
> until that gets checked in.

Ok, I'll wait then because I'd rather see a testcase first.

Comment 7

[jhp (no longer active)]

Attachment: tescase

To recreate submission attachment error:
1) Click Browse button and select a file.
2) Click Submit button.
=> This is a multipart-post, so the page that loads should show the instance
data, followed by the file you selected.  The path name in the <attachment>
node should be replaced by an ID string, which points to the attached file.

To recreate crash:
1) Click Browse button and select a file.
2) Click Clear.
3) Click Submit.
=> Should fail to submit, since the <attachment> node doesn't validate. 
Instead, it tries to submit and crashes.

Comment 8

[jhp (no longer active)]

Attachment: patch v1.2

No new functionality/fixes.  Just updated to work better with latest patch for
bug 275453.

Comment 9

[jhp (no longer active)]

Attachment: patch v1.3

Comment 10

[aaronr]

Comment on attachment 202267 [details] [diff] [review]
patch v1.3

r=me if you open a new bug about not being able to upload and submit big files using the attached testcase.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 202267 [details] [diff] [review]
patch v1.3

> /* static */ nsresult
>-nsXFormsUtils::ParseTypeFromNode(nsIDOMNode *aInstanceData,
>-                                 nsAString &aType, nsAString &aNSPrefix)
>+nsXFormsUtils::ParseTypeFromNode(nsIDOMElement* aElement,
>+                                 nsIDOMNode *aInstanceData,
>+                                 nsAString &aType, nsAString &aNSUri)
> {
>   nsresult rv = NS_OK;
> 
>   // aInstanceData could be an instance data node or it could be an attribute
>   // on an instance data node (basically the node that a control is bound to).
> 
>@@ -1271,28 +1272,39 @@ nsXFormsUtils::ParseTypeFromNode(nsIDOMN
> 
>   if (NS_FAILED(rv) || !typeVal) {
>     return NS_ERROR_NOT_AVAILABLE;
>   }
> 
>   // split type (ns:type) into namespace and type.
>+  nsAutoString prefix;
>   PRInt32 separator = typeVal->FindChar(':');
>   if ((PRUint32) separator == (typeVal->Length() - 1)) {
>     const PRUnichar *strings[] = { typeVal->get() };
>     // XXX: get an element from the document this came from
>     ReportError(NS_LITERAL_STRING("missingTypeName"), strings, 1, nsnull, nsnull);
>     return NS_ERROR_UNEXPECTED;
>   } else if (separator == kNotFound) {
>     // no namespace prefix, which is valid;
>-    aNSPrefix.AssignLiteral("");
>+    prefix.AssignLiteral("");
>     aType.Assign(*typeVal);
>   } else {
>-    aNSPrefix.Assign(Substring(*typeVal, 0, separator));
>+    prefix.Assign(Substring(*typeVal, 0, separator));
>     aType.Assign(Substring(*typeVal, ++separator, typeVal->Length()));
>   }
> 
>-  return NS_OK;
>+  if (prefix.IsEmpty()) {
>+    aNSUri.AssignLiteral("");
>+  } else {
>+    // get the namespace url from the prefix
>+    nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(aElement, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = domNode3->LookupNamespaceURI(prefix, aNSUri);
>+  }
>+
>+  return rv;
> }

This is (still) wrong, I think. The namespace lookup should happen first on
aInstanceData node and if not found there, then using the <xf:instance> element
(which can be get using GetInstanceNodeForData()).

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #10)
> (From update of attachment 202267 [details] [diff] [review] [edit])
> ...not being able to upload and submit big files
> using the attached testcase.
> 
Any idea what is causing this problem?

Comment 13

[jhp (no longer active)]

Attachment: patch v1.4

Comment 14

[jhp (no longer active)]

Comment on attachment 202412 [details] [diff] [review]
patch v1.4

How's this, smaug?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 202412 [details] [diff] [review]
patch v1.4

>-  return NS_OK;
>+  if (prefix.IsEmpty()) {
>+    aNSUri.AssignLiteral("");
>+  } else {
>+    // get the namespace url from the prefix using instance data node
>+    nsCOMPtr<nsIDOM3Node> domNode3 = do_QueryInterface(aInstanceData, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = domNode3->LookupNamespaceURI(prefix, aNSUri);
>+
>+    if (NS_FAILED(rv) || aNSUri.Length() == 0) {

Hmm, domNode3->LookupNamespaceURI never fails, but returns null if
namespace was not found. So I'd use:

if (DOMStringIsNull(aNSUri))


(if that works ;) )  r=me

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #15)
> (From update of attachment 202412 [details] [diff] [review] [edit])
> Hmm, domNode3->LookupNamespaceURI never fails, but returns null if
> namespace was not found. So I'd use:
> 
> if (DOMStringIsNull(aNSUri))
> 

Sorry, nsDOMAttribute may return an error. So:
if (NS_FAILED(rv) || DOMStringIsNull(aNSUri))

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> (In reply to comment #15)
> Sorry, nsDOMAttribute may return an error. So:
> if (NS_FAILED(rv) || DOMStringIsNull(aNSUri))
> 

Ugh, idiot me. nsDOMAttribute never returns an error.
if (DOMStringIsNull(aNSUri)) should be enough!

Comment 18

[jhp (no longer active)]

Checked in to trunk with smaug's change.  -> FIXED

Opened bug 315767 for large-file-submission issue.

Comment 19

[aaronr]

checked into MOZILLA_1_8_BRANCH via bug 323691.  Reopening for now until it gets into 1.8.0

Comment 20

[aaronr]

verified fixed in MOZILLA_1_8_BRANCH