Last Comment Bug 309546 - Fix file submission issues
: Fix file submission issues
Status: RESOLVED FIXED
: fixed1.8.0.2, fixed1.8.1
Product: Core
Classification: Components
Component: XForms (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: jhp (no longer active)
: Stephen Pride
Mentors:
Depends on: 275453
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-21 15:26 PDT by jhp (no longer active)
Modified: 2006-07-07 11:50 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (11.01 KB, patch)
2005-09-21 15:36 PDT, jhp (no longer active)
aaronr: review+
Details | Diff | Review
patch v1.1 (11.79 KB, patch)
2005-09-22 13:57 PDT, jhp (no longer active)
no flags Details | Diff | Review
tescase (2.51 KB, application/xhtml+xml)
2005-09-28 16:12 PDT, jhp (no longer active)
no flags Details
patch v1.2 (13.10 KB, patch)
2005-09-28 16:14 PDT, jhp (no longer active)
no flags Details | Diff | Review
patch v1.3 (13.89 KB, patch)
2005-11-08 09:56 PST, jhp (no longer active)
aaronr: review+
bugs: review-
Details | Diff | Review
patch v1.4 (13.71 KB, patch)
2005-11-09 12:43 PST, jhp (no longer active)
bugs: review+
Details | Diff | Review

Description jhp (no longer active) 2005-09-21 15:26:55 PDT
This bug is for fixing some of the file submission issues I noticed when working
on bug 275453.
Comment 1 jhp (no longer active) 2005-09-21 15:36:26 PDT
Created attachment 196958 [details] [diff] [review]
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 2005-09-22 11:07:10 PDT
Any testcase?
Comment 3 jhp (no longer active) 2005-09-22 11:39:15 PDT
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 2005-09-22 12:00:07 PDT
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) 2005-09-22 13:57:58 PDT
Created attachment 197087 [details] [diff] [review]
patch v1.1

Patch with Aaron's suggested fixes.
Comment 6 Allan Beaufour 2005-09-23 02:31:48 PDT
(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) 2005-09-28 16:12:32 PDT
Created attachment 197763 [details]
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) 2005-09-28 16:14:24 PDT
Created attachment 197764 [details] [diff] [review]
patch v1.2

No new functionality/fixes.  Just updated to work better with latest patch for
bug 275453.
Comment 9 jhp (no longer active) 2005-11-08 09:56:19 PST
Created attachment 202267 [details] [diff] [review]
patch v1.3
Comment 10 aaronr 2005-11-08 14:24:11 PST
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] (high review load, please consider other reviewers) 2005-11-09 10:01:36 PST
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] (high review load, please consider other reviewers) 2005-11-09 10:02:50 PST
(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) 2005-11-09 12:43:32 PST
Created attachment 202412 [details] [diff] [review]
patch v1.4
Comment 14 jhp (no longer active) 2005-11-09 12:44:30 PST
Comment on attachment 202412 [details] [diff] [review]
patch v1.4

How's this, smaug?
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2005-11-09 13:16:24 PST
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] (high review load, please consider other reviewers) 2005-11-09 13:19:59 PST
(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] (high review load, please consider other reviewers) 2005-11-09 13:21:44 PST
(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) 2005-11-09 14:33:30 PST
Checked in to trunk with smaug's change.  -> FIXED

Opened bug 315767 for large-file-submission issue.
Comment 19 aaronr 2006-02-02 17:22:43 PST
checked into MOZILLA_1_8_BRANCH via bug 323691.  Reopening for now until it gets into 1.8.0
Comment 20 aaronr 2006-07-07 11:50:02 PDT
verified fixed in MOZILLA_1_8_BRANCH

Note You need to log in before you can comment on or make changes to this bug.