Fix file submission issues

RESOLVED FIXED

Status

Core Graveyard
XForms
RESOLVED FIXED
12 years ago
10 months ago

People

(Reporter: jhp (no longer active), Assigned: jhp (no longer active))

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
fixed1.8.0.2, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

12 years ago
This bug is for fixing some of the file submission issues I noticed when working
on bug 275453.
(Assignee)

Comment 1

12 years ago
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').
(Assignee)

Updated

12 years ago
Attachment #196958 - Flags: review?(aaronr)

Comment 2

12 years ago
Any testcase?
(Assignee)

Comment 3

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

Comment 4

12 years ago
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
Attachment #196958 - Flags: review?(aaronr) → review+
(Assignee)

Comment 5

12 years ago
Created attachment 197087 [details] [diff] [review]
patch v1.1

Patch with Aaron's suggested fixes.
Attachment #196958 - Attachment is obsolete: true
Attachment #197087 - Flags: review?(allan)

Comment 6

12 years ago
(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.
Status: NEW → ASSIGNED
Depends on: 275453

Updated

12 years ago
Attachment #197087 - Flags: review?(allan)
(Assignee)

Comment 7

12 years ago
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.
(Assignee)

Comment 8

12 years ago
Created attachment 197764 [details] [diff] [review]
patch v1.2

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

Updated

12 years ago
Attachment #197087 - Attachment is obsolete: true
(Assignee)

Comment 9

12 years ago
Created attachment 202267 [details] [diff] [review]
patch v1.3
Attachment #197764 - Attachment is obsolete: true
Attachment #202267 - Flags: review?(aaronr)

Comment 10

12 years ago
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.
Attachment #202267 - Flags: review?(aaronr) → review+
(Assignee)

Updated

12 years ago
Attachment #202267 - Flags: review?(smaug)

Comment 11

12 years ago
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()).
Attachment #202267 - Flags: review?(smaug) → review-

Comment 12

12 years ago
(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?
(Assignee)

Comment 13

12 years ago
Created attachment 202412 [details] [diff] [review]
patch v1.4
Attachment #202267 - Attachment is obsolete: true
Attachment #202412 - Flags: review?
(Assignee)

Comment 14

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

How's this, smaug?
Attachment #202412 - Flags: review? → review?(smaug)

Comment 15

12 years ago
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
Attachment #202412 - Flags: review?(smaug) → review+

Comment 16

12 years ago
(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

12 years ago
(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!

(Assignee)

Comment 18

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

Opened bug 315767 for large-file-submission issue.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch

Comment 19

12 years ago
checked into MOZILLA_1_8_BRANCH via bug 323691.  Reopening for now until it gets into 1.8.0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

12 years ago
Whiteboard: xf-to-branch

Updated

12 years ago
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Keywords: fixed1.8.0.2
Resolution: --- → FIXED

Comment 20

11 years ago
verified fixed in MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.