Closed Bug 120682 Opened 23 years ago Closed 23 years ago

Make form submission not suck

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.9

People

(Reporter: john, Assigned: john)

References

Details

Attachments

(2 files, 9 obsolete files)

Patch v1.4.4
202.41 KB, patch
sicking
: review+
jst
: superreview+
Details | Diff | Splinter Review
backtrace of crash when clicking "submit query" in bugzilla
3.42 KB, text/plain
Details 
The current form submission interface (nsIFormControl) is cumbersome (four
functions where one will do); inefficient (it mallocs arrays when it does not
need to); and non-reusable (intimately tied to our current set of HTML form
controls).

To address all these issues, form controls should implement an interface,
nsISubmittable, which has one function in it:

- submitNamesValues(nsIFormSubmitter* submitter, PRBool sendFiles)

The form submitter's main loop will call this function once on each form
control, which will look at its name and values and call one of these two
functions on the form submitter:

- addNameValuePair(nsAString& name, nsAString& value)
- addNameValueFile(nsAString& name, nsAString& value, nsAString& filename)

The form submitter, when it receives these names and values, will do whatever it
needs to build the submit stream out of all names/values on the form.

Because this interface allows you to add names and values one by one, it does
not require the construction of *any* arrays of names and values.  The form
submitter can build its multipart or URL-encoded request dynamically, directly
from this information.

The filename will probably be replaced with a stream class or a string so that
constructs can send any data they want as the "file."  This will allow
submittable OBJECTs to more easily send files based on arbitrary information.
can we punt the usage of nsFileSpec from the form submitter as we do this?  :)
I have been doing some investigation, and it is possible to submit multiple
files.  Bug 44464 is filed on this.  The interface must reflect it.  Thus I
propose we use an earlier proposal of sicking's and make the interface like this:

addNameValuePair(name,value)
addNameFilePair(name,filename,boolean moreFilesToCome)

Reading the RFC and the method of communication, this seems to be what makes the
most sense.

We will do the actual "multipart/mixed" work for multiple file submission
separately; the interface should just support it.  Right now we will just send
each file separately; since the form controls only support one file per name,
that should not be a problem.

To see how multiple files would be implemented, see RFC 1867
(http://www1.ics.uci.edu/pub/ietf/html/rfc1867.txt), section 6.  That should
give a pretty good flavor of how this will all work.

bz: I think that's likely.  sicking is working on a file stream class that
should make our file usage much more ... glorious.
Blocks: 78071
Blocks: 96576
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
sicking's multiplexing input stream (going in as part of this) will fix bug
114106.  Yay!
Blocks: 114106
Just for discussion's sake, here is how Form Submission will work in the new
world (after discussing and implementing I think we've come up with the best
idea we can):

FORMS are responsible for knowing *what* data to submit.
FORM SUBMISSION objects are responsible for knowing *how* to submit.

There will be multiple form submission classes, because urlencoded and
multipart/form-data are encoded completely differently.  In essence:

1. The form will grab the appropriate nsIFormSubmission object.
2. The form will walk through form.elements[] and have each element give their
data to the naIFormSubmission object (calling AddNameValuePair() or
AddNameFilePair() on it for each variable it wants to submit).
3. When the object receives data, it will build it into the query string or POST
data or multipart/form-data string that it is going to send to the server.
4. The form will call "SubmitTo()" on the nsIFormSubmission object with the
appropriate action and target, and the object will take the data it has
marshalled, and submit it.

In support of encoding the data, sicking has created an
nsIMultiplexedInputStream, which essentially strings together input streams. 
This means we can submit the name/value pairs as a string stream, then submit
the file as a file input stream, then more name/value pairs as a string stream,
and then ... you get the idea.  We won't have to read the files from the disk
until they are actually submitted.

Especially because we are getting rid of the arrays and not dumping all the data
to a file, this is a big big win for performance.
This is a test comment, with the patch.  Bugzilla: the ultimate form test.
Attached patch Patch (obsolete) — — Splinter Review 
OK, this has been tested against GET, POST and multipart w/files.  I am
actually submitting this patch using the patched build.
Attached patch Patch (obsolete) — — Splinter Review 
OK, this has been tested against GET, POST and multipart w/files.  I am
actually submitting this patch using the patched build.
Comment on attachment 67161 [details] [diff] [review]
Patch

Heh, looks like it needs work, it's not submitting the full length of file. 
Available() problems probably.
Attachment #67161 - Attachment is obsolete: true
Attachment #67162 - Attachment is obsolete: true
Attached patch Patch v1.1 (obsolete) — — Splinter Review 
This version has been tested on local *and* remote with GET, POST, and
multipart/form-data with all types of controls
(http://www.johnkeiser.com/cgi-bin/mozform.pl).  Especial attention given tto
testing in Bugzilla, including queries, comments and *ahem* patch submission. 
Remaining tests include internationalization and BIDI.	Everything you see here
submitted with the patched build.

This patch includes:
- new improved control submission interface!
- all new dazzling nsMIMEInputStream and nsMultiplexInputStream!
- complete removal of the temporary file!
- significant performance and readability enhancements!
- fix to nsBufferedInputStream::Available() to report size of underlying stream
The problem before, BTW, was a Necko thing--file stream was not doing the right
thing when you're talking to a remote pipe (i.e. a narrow pipe).  That bug is
filed as bug 122849.  We've fixed it by using a buffered input stream like form
sub was using before, which should speed things up anyway.
P.S. 100x thanks to sicking for the awesome streams, which are all his!
+        PRInt64 size;
+        rv = file->GetFileSize(&size);
....
+              rv = NS_NewLocalFileInputStream(getter_AddRefs(rawStream), file);

What does all that do in cases when the file is not readable?  Will we end up
creating a raw stream?  Or no?  (/me hopes not).
I am not actually sure where in the process it fails, but it *does* fail, and
I'm guessing it's at the stream.  The upshot is, if the file does not exist, it
submits as though the file input were input type=text (this is how we send it in
method=get and method=post with url-encoded type, so it should be safe).

I did just test this, both with a bad directory and a bad file within a good
directory.  Both work as expected.  'Twas definitely a good point.
-    *result = mFillPoint - mCursor;
-    return NS_OK;
+  nsresult rv = NS_OK;
+  if (mStream) {
+    rv = Source()->Available(result);
+  }
+  return rv;

shouldn't you return

Source()->Available() + mFillPoint - mCursor

Doh!  I'll put it into the next version.  Meantime, this shouldn't fail for
anyone and testing can continue.  As long as Available() returns <= the total
number of possible bytes to be read, everyone is obviously happy, since that's
what we were doing before.
Comment on attachment 67338 [details] [diff] [review]
Patch v1.1

assuming that it was intenisve tested

r= alexsavulov

for the form submission part (excluding the input stream stuff)
great piece of job, John!

(for subsequent patch versions just transfer the r= if changes are not
substantial)
Attachment #67338 - Flags: review+
Remove the size argument from AddNameFilePair, the MIME stream relies on
stream->Available() returning the correct size anyway, and you don't seem to 
use it.

Tha |aSource| argument to AddNameValuePair and AddNameFilePair should IMHO be 
an nsIDOMHTMLElement. Since that's what you always use it as anyway there's no 
point in trying to hide it. If some object that doesn't implement 
nsIDOMHTMLElement submits it can just provide null as aSource.

How about making UnicodeToNewBytes a non-static. That way you can remove the 
last 4 arguments to that function.

Why do you need the StartEncode() functions? why not simply put that code in 
the Init() function? Same with EndEncode, couldn't that be put at the beginning 
of SubmitTo and have the respecitve handler implement OnSubmit, the codereuse 
is minimal IMHO (do rods and pollman really want those debugthingies still?)

Did we really need that platformencoder? wasn't that unneccesary since the 
<input> opens the file?
Attached patch Patch v1.2 (obsolete) — — Splinter Review 
Fixes buffered stream
Attachment #67338 - Attachment is obsolete: true
Comment on attachment 67538 [details] [diff] [review]
Patch v1.2

>Index: xpcom/io/nsIMIMEInputStream.idl

>+#include "nsIInputStream.idl"
>+
>+
>+[scriptable, uuid(dcbce63c-1dd1-11b2-b94d-91f6d49a3161)]
>+interface nsIMIMEInputStream : nsIInputStream
>+{
>+    attribute boolean addContentLength;
>+    void init();
>+    void addHeader([const] in string name, [const] in string value);
>+    void setData(in nsIInputStream stream);
>+};

please add documentation to these methods.  eg. what does this interface do?
and to nsIMultiplexInputStream.idl as well.


>Index: xpcom/io/nsMIMEInputStream.cpp

>+/*
>+ * The MIME stream separates headers and a datastream. It also allows
>+ * automatic creation of the content-length header.
>+ */

how is this generic enough to belong in xpcom/io?  perhaps it really belongs in
necko.



>+    nsStringInputStreamConstructor(nsnull,
>+                                   NS_GET_IID(nsIStringInputStream),
>+                                   getter_AddRefs(mHeaderStream));
>+    nsStringInputStreamConstructor(nsnull,
>+                                   NS_GET_IID(nsIStringInputStream),
>+                                   getter_AddRefs(mCLStream));

do not hard code against this symbol.  it's not meant for mass consumption.
it would be better to introduce a NS_NewStringInputStream method or
simply use do_CreateInstance() until we have an alternate constructor (and
file a bug).  unfortunately NS_NewStringInputStream currently is used for
something else... yuck :-(


>+/**
>+ * Forward everything else to the mStream after calling initStreams()
>+ */
>+

you might want to define an inline function that checks mStartedReading before
calling initStreams to avoid
the overhead of always calling initStreams.

>Index: xpcom/io/nsMultiplexInputStream.cpp

>+        rv = stream->Read(aBuf, aCount, _retval);
>+        NS_ENSURE_SUCCESS(rv, rv);

what if the underlying stream returns NS_BASE_STREAM_WOULD_BLOCK and you have
already read something?
then you should not return NS_BASE_STREAM_WOULD_BLOCK... you should return
NS_OK + whatever you've already
read.


>Index: netwerk/base/src/nsBufferedStreams.cpp

>-    *result = mFillPoint - mCursor;
>-    return NS_OK;
>+  nsresult rv = NS_OK;
>+  *result = 0;
>+  if (mStream) {
>+    rv = Source()->Available(result);
>+  }
>+  *result += (mFillPoint - mCursor);
>+  return rv;

you need to verify that Seek() will not break this.
Attachment #67538 - Flags: needs-work+
1. The reason for the size argument being there is so that you can send a
network stream if you want.  But now that I think of it, we'd have to make
network streams implement Available() anyway.  So, consider it removed.

2. Make aSource nsIDOMHTMLElement.  Good point.  Will do.

3. I've been going back and forth on UnicodeToNewBytes().  I suppose for now it
couldn't hurt to make it a class method though; and it would make the code
easier to read.

4. StartEncoding() / EndEncoding(): we could move StartEncoding() into Init(),
yes.  Then we'd have to add the requirement that subclasses call
nsFormSubmitter::Init() before they do their work.  I am not sure what clean
alternative there is for EndEncoding().  What you say may actually make things
better; we wouldn't have to have GET return a null stream, it could just not
worry about POST streams at all.  But that, again, is transport, not encoding. 
(In fact, slapping the MIMEStream onto it is probably transport too.)  I'll
think about it.

5. PlatformEncoder handles the encoding of the filename.  Filenames are always
encoded in the encoding of the platform.  I am queasy about these semantics
(doesn't it seem like file values should *always* be interpreted with platform
encoding?), but rather than change that too, I think we should fix it separately
(it is already acting this way).

6. I'll start removing rods's and pollmann's debug thingies :)
BTW, so far I have tested this patch on GET, POST and multipart, local *and*
remote, on debug, optimized and non-BIDI builds.  I have tested
internationalization as far as I am capable.

Remaining tests: BIDI, Windows, Mac (needs stuff to check new files in on the
Mac for sure).
Attached patch Patch v1.3 (obsolete) — — Splinter Review 
1. Removed size arg from AddNameFilePair()
2. Changed aSource arg from nsIContent* to nsIDOMHTMLElement*
3. Made UnicodeToNewBytes non-static
4. Removed some of rods's and pollman's debug thingies

None of these changes should materially affect anything.  Submitting patch with
patched build to be sure.

Input stream changes still need to go in.
Attachment #67538 - Attachment is obsolete: true
Blocks: 114356
Blocks: 117363
Marking nsbeta1+
Keywords: nsbeta1+
not to self and jkeiser: platformencoder is due to bug 29062
more notes: if bug 124628 turns out to be a good suggestion we can get rid of
the ::Init functions in nsFormSubmitter and nsMIMEInputStream if we wanted
review comments on Johns code:

+ if (processedValue) {
+  delete processedValue;
+ }
just do |delete processedValue| (twice)


+  mQueryString = NS_LITERAL_CSTRING("");
mQueryString.Truncate();


Why is the aFilename argument an nsAString* while all others are nsAString&?
nsAString& is better IMHO.


For URL-encoded POST is there really any need to create a MIME stream? It would
be more optimised to just put everything in a single string stream.


Use iterators to cut-n-paste the #-stuff and the ?-stuff


+        if (namedAnchor.IsEmpty()) {
+          path.Append(namedAnchor);
+        }
is this *really* intended ;-)
IMHO you could always just append namedAnchor, appending an empty string should
be as fast as the extra |if|


I wonder if you should convert linebreaks in the name too in
nsFSMultipartFormData::AddNameValuePair.


Use the errorvalue from do_CreateInstance and do_GetService and
NS_ENSURE_SUCCESS(rv, rv) instead of checking the returnvalue for null.


in nsFSMultipartFormData::AddPostDataStream, do an NS_ENSURE_SUCCESS(rv, rv)
after creating the stringstream


+  if (!*aFormSubmission) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
NS_ENSURE_TRUE(*aFormSubmission, NS_ERROR_OUT_OF_MEMORY);


nsFormSubmission::GetSubmitCharset:
The |nsIPresContext* aPresContext| argument doesn't seem to be used, and please
make the outparam the last argument.

The code doesn't pass through the BIDI part when accept-charset attribute is
used, but I'm not sure if that's intentional.

The function should an nsresult.

Shouldn't the XXX comment be removed since we do get the charset from
accept-charset first?

Use iterators when searching the accept-charset value for a charset. The list of
charsets is space and/or commaseparated. Though you maybe wanna do that in a
different bug?


nsFormSubmission::GetEncoder:
+  rv = nsServiceManager::GetService(kCharsetConverterManagerCID ,
use an nsCOMPtr and do_GetService instead (I think that's possible)

+     if (encoder) {
+       rv = NS_ERROR_FAILURE;
+     }

uhoh :-), this seems to be an error from the first submission rewrite, and was
an error even in the FormFrame (should be *encoder). Just do the
NS_ENSURE_SUCCESS on rv instead.


the nsFSURLEncoded::URLEncode functions seems to have strayed from the other
nsFSURLEncoded functions?


I wonder if you should inline UnicodeToNewBytes into EncodeVal, that seems to be
the only place it's called (once the platformencoder is gone). Not sure if you
had some special plans for that function thouhg? I got that impression before.


That's it for nsFormSubmission.cpp



oh, and this was of course submitted with a patched build ;-)
nsHTMLInputElement::SubmitNamesValues should always cut the path from the
filename when not submitting the file.

nsHTMLSelectElement::SubmitNamesValues (some of this is old code, but it would
be nice to get it fixed now):
+  //
+  // Get the value
+  //
+  nsAutoString value;
+  rv = GetValue(value);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
copy'n'paste error?

    mOptions->ItemAsOption(optIndex, getter_AddRefs(option));
    if (option) {
this should always succeeded, right? use NS_ENSURE_TRUE instead

      rv = option->GetSelected(&isSelected);
      if (NS_FAILED(rv) || !isSelected) {
        continue;
only |continue| on !isSelected, NS_ENSURE_SUCCEESS rv

      nsCOMPtr<nsIOptionElement> optionElement = do_QueryInterface(option);
      if (optionElement) {
less if, more ENSURE :), or shouldn't an assertion be enough?


Just nsHTMLFormElement left...



please add some small comments on the new nsHTMLFormElement methods.


The |nsIPresContext* aPresContext| argument to DoReset doesn't seem to be used


nsHTMLFormElement::DoSubmit, less if more ENSURE. You really only need a single
if in that function.  (sorry, i'm allergic to this)

nsHTMLFormElement::NotifySubmitObservers:
well, you know, ENSURE...

+    nsIDocument* document;
+    GetDocument(document);
use mDocument instead, but make sure it's != null

+      nsCOMPtr<nsIFormSubmitObserver> formSubmitObserver(
+                      do_QueryInterface(inst, &rv));
don't do use &rv here. It's an exception to the rule. Reason being that there is
basically only one thing that can fail for QIs (at least i think that's the reason).

+        nsresult notifyStatus = formSubmitObserver->Notify(this,
+        nsresult notifyStatus = formSubmitObserver->Notify(this,
+                                                           window,
+                                                           aActionURL,
+                                                           aCancelSubmit);
+        if (NS_FAILED(notifyStatus)) { // assert/warn if we get here?
+          return notifyStatus;
+        }
why not just |rv =|? And use NS_ENSURE_SUCCESS, that way we'll get the assertion

+      if (*aCancelSubmit) {
+        return NS_OK;
+      }
I'm not sure on this, but i think we should notify all observers, even if one
tells us to cancel. What did the old code do?


nsHTMLFormElement::CompareNodes:
+  rv = a->GetParentNode(getter_AddRefs(aParent));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
I'm not sure what we wanna return if some node isn't in the tree (doesn't have a
parent)? Should it be an error?

+    nsCOMPtr<nsIContent> aParentContent(do_QueryInterface(aParent));
+    nsCOMPtr<nsIContent> aContent(do_QueryInterface(a));
+    rv = aParentContent->IndexOf(aContent, aIndex);
you need to check if the first QI is successfull, not all nodes are nsIContents,
documents and attributes for example.


nsHTMLFormElement::GetActionURL:
remove the comment about needing the document to notify observers since that's
not done here any more.

+  nsIDocument* document;
+  GetDocument(document);
use mDocument instead.

please make a note in the function-description that the submission should be
aborted if no url is returned, for example for security reasons.

The code for when the href is empty is very strange IMHO. Now it uses the
base-url as href and then resolves it against the base-url again! Just set
actionURL = docURL if the href is empty.

use ENSURE instead of the if after |securityManager->IsCapabilityEnabled|.
bbaetz also asked that we put a comment above this code that it has never been
really tested, since mailto: forms don't work in moz.


Thats it! Excellent work!
Sorry, i should say that most of my nits are on code that's not origianlly
Johns, but is copied from old code nsFormFrame.

So thanks a million for fixing this!
Attached patch Patch v1.4 (obsolete) — — Splinter Review 
Great feedback!  This addresses all of sicking's issues except:

1. "just do |delete processedValue| (twice)" - I don't get this. 
processedValue could be null.  You don't delete nulled memory.
2. aFilename was made an nsAString* so that it could be sent as |nsnull| for
interfaces that didn't have a filename.  But from RFC 1867, we're supposed to
try real hard to supply the filename, so we'll just make 'em send blank if
there is no filename.
3. MIME stream for POST -- yeah, but MIME stream is so cool!  The performance
problems are negligible, and you get the (also negligible in this case) benefit
of re-using the code.  It *does* look more pretty and easy to maintain,
however, which is a very real benefit.
4. Linebreak conversion happens everywhere -- it's a MIME thing.
5. Our accept-charset functionality is somewhat broken anyway, I *would* rather
fix it up in another bug.
6. UnicodeToNewBytes seems like a useful entity on its own.  I'd rather leave
it there rather than try to rip it out of EncodeVal when I need it later.
7. 2 or 3 others we talked about and agreed upon in IRC.

We are now using this in both Linux and Windows builds.  Optimized and debug
has been tested.  Internationalization has been tested.  Remaining:
- test BIDI and Mac
- decide how or whether to make the streams threadsafe
Attachment #67671 - Attachment is obsolete: true
> 1. "just do |delete processedValue| (twice)" - I don't get this. 
> processedValue could be null.  You don't delete nulled memory.

delete (unlike free) checks for null, so |delete nsnull;| is safe.

> 3. MIME stream for POST -- yeah, but MIME stream is so cool!  The performance
> problems are negligible, and you get the (also negligible in this case)
> benefit of re-using the code.  It *does* look more pretty and easy to
> maintain, however, which is a very real benefit.

Sounds fine

> 4. Linebreak conversion happens everywhere -- it's a MIME thing.

But you have explicit code that does it for the value, but none for the name?

> 5. Our accept-charset functionality is somewhat broken anyway, I *would*
> rather fix it up in another bug.

Sounds fine. I was terribly confused by that code.

> 6. UnicodeToNewBytes seems like a useful entity on its own.  I'd rather leave
> it there rather than try to rip it out of EncodeVal when I need it later.

Okidoki
Attached patch Patch v1.4.1 (obsolete) — — Splinter Review 
- Made streams threadsafe
- Fixed delete to not do if ()
Attachment #68846 - Attachment is obsolete: true
in the declaration of GetEncoder, change |mCharset| to |aCharset| (it's right
everywhere else)

You've forgot to change the filename from nsAString* to nsAString&

Do the changes to <input type=file> we talk about on irc; change the |if (file)
{| to ENSURE, change the |} else {| to |if(!submittedValue)|

Rename EndEncoding to GetEncodedSubmission

the |if (dataStream) {| isn't needed in nsFSURLEncoded::EndEncoding

in nsFormSubmission::SubmitTo:
+  if (NS_OK == aPresContext->GetLinkHandler(getter_AddRefs(handler))) {
don't compare to NS_OK, use NS_SUCCEEDED instead.

in nsHTMLFormElement::CompareNodes you still need to check that the QI's to
nsIContent succeeds (at least on the parent).


with that, r=sicking
Attached patch Patch v1.4.2 (obsolete) — — Splinter Review 
Fixed all issues in previous comment.  This is a pretty minor change.
Attachment #68908 - Attachment is obsolete: true
Attachment #68936 - Flags: review+
Comment on attachment 68936 [details] [diff] [review]
Patch v1.4.2

nit: don't change the indentation in nsBufferedStreams.cpp

mac project file changes?

otherwise, sr=darin on the netwerk and xpcom stream changes.
Blocks: 81758
Attached patch Patch v1.4.3 (obsolete) — — Splinter Review 
- Mac build changes (we are tested on the Mac now!)
- small stream changes (my r= comments)
Attachment #68936 - Attachment is obsolete: true
Comment on attachment 69292 [details] [diff] [review]
Patch v1.4.3

r=jkeiser on streams
sr=darin on streams
r=sicking on form changes
Attachment #69292 - Flags: review+
There is other information on this earlier in the bug, but here is the broad
overview of this patch (what happens when you call nsHTMLFormElement::OnSubmit()):

GET THE SUBMISSION OBJECT
1. OnSubmit(): Gets an nsIFormSubmission instance that knows how to encode the
way you want to encode.

ENCODE THE NAME/VALUE PAIRS
2. Walk over all the form controls you want to submit and call
SubmitNamesValues(myFormSubmission) on them.
3. Each of these form controls is then responsible for calling
myFormSubmission->AddNameValuePair() (or AddNameFilePair()).
4. AddNameValuePair() takes the name and value and gets it ready for shipping
(in the case of url-encoded, it appends "&name=value" to a big string).

SUBMIT THE INFORMATION
5. nsIFormSubmission::SubmitTo(): knows how to take this info and send it to the
specified action/target.  For example, the url-encoded submission class appends
the encoded string to the URL or places it into a POST data stream, depending on
the method specified.


MULTIPART
Multipart submission uses an nsIMultiplexInputStream to store its data.  This is
is a stream composed of other streams--string streams and file streams, in our
example--that will be concatenated together as though they were one stream (they
will be read sequentially).  Multipart takes name/value pairs and turns them
into MIME sections of data that contain the name and value.  When it receives a
file, it creates the headers for the MIME section that will contain the file,
wraps up a string stream with those headers, and adds that to the multiplexed
stream.  It then adds the file input stream to the multiplexed stream as well.

For efficiency purposes, we store as many sections as possible in a string
stream--for example, if there are 8 input type=texts, one input type=file, and
then 4 more input type=texts, you will have 3 streams--one string stream, one
file stream, and one more string stream)


MIMESTREAM
Both multipart and POST data streams use nsIMIMEStream(), which is a nice helper
stream that lets you set the headers and also, as a bonus, calculates the size
of the data for a dynamic Content-Length: header using the Available() method on
the stream (which reports the size for all streams we are using).  This last
feature is necessary in case you reload a file submission URL (reposting the
POST data stream) and the size of one of the files being submitted has changed.


This is the relevant information that I can think of.  I endeavored not to
change charset or BIDI except to use our string classes better in some cases
where it was doing excessive string copies (it is still doing more than it needs
to).
- The aTarget argument to nsIFormSubmission::SubmitTo() should be const, no? And
argument descriptions are missing from the header.

- The aSource argument to nsIFormSubmission is of type nsIDOMHTMLElement*,
should we not leave this as an nsIDOMElement* for potential future XForms n' who
knows what standards that come along?

- What's up with all those "protected:"'s in nsFormSubmission :-)

More later...


- In nsFSURLEncoded::GetEncodedSubmission():

+    mimeStream->SetData(dataStream);
+
+    *aPostDataStream = mimeStream;
+    NS_ADDREF(*aPostDataStream);
+
+    NS_ASSERTION(*aPostDataStream,
+                 "Unable to create POST data stream!  Bad bad things.");

That assertion is pointless, it will never be hit, you'll crash before getting
there.

- ... and just below that, I'd prefere an early return if |schemeIsJavaScript|
is true.

- ... and below that:

+      path.Append(mQueryString);
+
+      // Bug 42616: Add named anchor to end after query string
+      path.Append(namedAnchor);

Could you not use path.Append(mQueryString + namedAnchor) here to eliminate a
.Append() call?

- In nsFSURLEncoded::URLEncode():

+  if (!inBuf)
+    inBuf  = ToNewCString(aString);
...
+  delete [] inBuf;

Double space before '=', no OOM checking, and free/delete missmatch. Use
nsMemory::Free() and not delete[] on the string (this would generate FMM's (Free
Memory Mismatch) in Purify).

- Also, if you'd make nsFSURLEncoded::URLEncode() take a nsCString& (or
nsACString&) out parameter you would save an allocation per call since you don't
need to allocate the returned nsCString.

- The argument list in the declaration of the nsFSMultipartFormData
constructor don't line up wel...

- In nsFSMultipartFormData::AddNameValuePair(), using an nsCAutoString just to
Adopt() something into it doesn't make much sense, or does it? nsCString seems
more appropriate (and cheaper).

- ... also:

+  mPostDataChunk += NS_LITERAL_CSTRING("--") + mBoundary
+                 + NS_LITERAL_CSTRING(CRLF)
+                 + NS_LITERAL_CSTRING("Content-Disposition: form-data; name=\"")
+                 + nameStr + NS_LITERAL_CSTRING("\"" CRLF)
+                 + NS_LITERAL_CSTRING(CRLF)

This results in a double CRLF, is that intentional? If so, you could eliminate
one string concatenation if you move the latter up into the one before it.

- In nsFSMultipartFormData::AddNameFilePair(), inconsistent variable initialization:

+  nsString* processedValue;
+  processedValue = ProcessValue(aSource, aName, aFilename);

How about:

+  nsString* processedValue =
+    ProcessValue(aSource, aName, aFilename);

in stead? Also nsCAutoString/Adopt here...

- ... also:

+  filenameStr.Adopt(nsLinebreakConverter::ConvertLineBreaks(filenameStr.get(),
+                 nsLinebreakConverter::eLinebreakAny,
+                 nsLinebreakConverter::eLinebreakNet));

bad next line argument indentation.

- ... and futher down:

+       + NS_LITERAL_CSTRING(CRLF)
+       + NS_LITERAL_CSTRING(CRLF);

make that NS_LITERAL_CSTRING(CRLF CRLF), no?

- In nsFSMultipartFormData::Init():

+  mPostDataStream = do_CreateInstance(
+                      "@mozilla.org/io/multiplex-input-stream;1", &rv);

Moving the |do_CreateInstance(| down to the next line would be clearer, no?

- In nsFSMultipartFormData::GetEncodedSubmission():

+  nsCOMPtr<nsIMIMEInputStream> mimeStream(
+    do_CreateInstance("@mozilla.org/network/mime-input-stream;1", &rv));

use var = ... in stead of var(...) here for clarity.

- ... and below that:

+  mimeStream->AddHeader("Content-Type",
+                        PromiseFlatCString(
+                          NS_LITERAL_CSTRING("multipart/form-data; boundary=")
+                          + mBoundary).get());

We know NS_LITERAL_STRING(...) + mBoundary is not a flat string, so I would
guess using a nsCAutoString here directly would be cheaper.

- In nsFSMultipartFormData::AddPostDataStream():

+  rv = NS_NewCStringInputStream(getter_AddRefs(postDataChunkStream),
+                                               mPostDataChunk);
                                 ^<-------------|
- ... and:

+  mPostDataChunk = NS_LITERAL_CSTRING("");

I guess this works, but why not simply mPostDataChunk.Truncate()?

- In GetSubmissionFromForm():

+  nsCOMPtr<nsIFormProcessor> formProcessor =
+           do_GetService(kFormProcessorCID, &rv);

two space indentation is enough here...

- ... and:

+  ((nsFormSubmission*)*aFormSubmission)->Init();

use NS_STATIC_CAST here to make the compiler catch more potential future problems...

More comments on its way...
Blocks: 125559
- In nsFormSubmission::SubmitTo():

+      nsCOMPtr<nsGenericElement> formElement = do_QueryInterface(aForm);

Eek, you can't do that. nsGenericElement doesn't have an IID, you don't know
what'll come out of that. QI to an interface you know nsGenericElement
implements, and static cast from that. Hmm, it seems like the only reason you do
this is to get to an nsIContent interface pointer, you should QI directly to
nsIContent then.

- ... also:

+  if (NS_SUCCEEDED(aPresContext->GetLinkHandler(getter_AddRefs(handler)))) {
+    if (handler) {

No need for the double error checking here, just the null check is enough.

- In nsFormSubmission::GetSubmitCharset():

+  if (NS_CONTENT_ATTR_HAS_VALUE == rv) {
+    if (eHTMLUnit_String == value.GetUnit()) {

What you're asking here is if var == constant, not if constant == var, so swap
them around (I bet you copied this from somewhere :-) and maybe combine those
two checks into one if check too?

- ... that method could use some string iterator love, you could eliminate a
.Mid() call with Substring()'s n' all that cool stuff, your call.

- In nsFormSubmission::UnicodeToNewBytes(), inconsisten use of spaces around
operators.

- In nsFormSubmission::GetEnumAttr(): no default value for the out parameter.
(and backwards constant == val check too :-)

- In nsFormSubmission::ProcessValue(): Do we really need to do so many string
copies there? Can we use nsAutoString if we really do need all those copies?
Couldn't the method take a nsAString& out parameter in stead of having to
allocate the result string? Also, one space missing from the next line arbument
in the declaration.

More comments later...
No longer blocks: 125559
Changed everything so far except:

- The aSource element to AddNameValuePair() should stay as an nsIDOMHTMLElement
until we change the nsIFormProcessor interface to be an nsIDOMElement (filed as
bug 125555)

- string iterator love in GetSubmitCharset(): sicking had the same comment :) 
That function was copied almost verbatim and I would rather not mess with it ATM
if I could.  charsets are going to be getting some lovin' later anyway, I can
mess with it then.  (The main reason I don't want to do it now is Bo Don't Know
Iterators and these are the last days of this patch.)

- In nsFormSubmission::UnicodeToNewBytes(), inconsistent use of spaces around
operators: I just went through and tried to consistent-ize them as much as
possible, but note that there is still not *always* a space between the operator
and the args; I try to use my judgement on readability there.  When in an = I
use spaces; when there are only two args and it's in brackets or a for() I tend
to not use them.  (The inconsistent spacing before was due to copied code,
BTW--I was trying not to modify character encoding code.)

- In nsFormSubmission::GetEnumAttr(), there can be no proper default value. 
Callers supply that themselves, it is different for every attribute.

- In nsFormSubmission::ProcessValue(), again, this is a faulty nsIFormProcessor
interface.  Bug 125555 handles the nsString issue as well.  The return value of
nsString* is due to the interface again--nsCAutoString won't work.  I think we
are doing a minimum of copies; unfortunately, the minimum is a lot.  (The return
value of nsString* rather than dumping stuff into nsCString& is also because
mFormProcessor does not exist a lot of the time and I don't want to even bother
*initializing* a string for its return value if I don't have to.)


I'll wait to post the updated patch until you are done with the sr, unless you
want to see it now?
Blocks: 125559
- In nsHTMLButtonElement::SubmitNamesValues():

+  rv = aFormSubmission->AddNameValuePair(this,name,value);

Space-after-commas for consistency.

- In nsHTMLFormElement::NotifySubmitObservers(), make contractId static.

- In nsHTMLFormElement::CompareNodes(), don't name local variables |aFoo|, i.e.
rename aParent, aContent, and aParentContent, those names make them look like
method arguments in the mozilla code.

- In nsHTMLFormElement::WalkFormElements():

+        nsCOMPtr<nsIFormControl> image =
+do_QueryInterface(imageElementNode);

Indent two spaces from the line above, if the whole thing doesn't fit in 80 columns.

- In nsHTMLFormElement::GetActionURL():

+  if (!mDocument) return NS_OK; // No doc means don't submit, see Bug 28988

Don't put the if condition and the statement on the same line, and add braces
too. There's more of those in that same function.

Fix those issues as well (unless you disagree, then we'll talk, your comments
above make sense), and you'll have sr=jst
No longer blocks: 125559
Attached patch Patch v1.4.4 — — Splinter Review 
- fixes all JST's issues except those listed in my comment
Attachment #69292 - Attachment is obsolete: true
OK, this patch has passed the Bugzilla Acid Test.  jst, sicking, you OK with it?
Comment on attachment 69573 [details] [diff] [review]
Patch v1.4.4

Fine with me if you went through all those issues, which it seems like you did.

sr=jst
Attachment #69573 - Flags: superreview+
On a linux CVS build, i've started to get a lot of crashes very recently.
Not sure all are related: i've crashed when clicking links as well as submit
buttons.
One that has repeated several times now is when clicking "Submit Query" or
"Commit" button here in bugzilla. I believe this checkin was made in the
timeframe between good/bad build. Attaching backtrace, in case it's worth
looking at.
Hmm.  I am not seeing the crash, but looking at the stack trace and the code,
it's probably crashing when it tries to get nsCharsetConverterManager's contract ID:

mCharsetConverterManager = do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID,
&rv);

rkaa, what Internationalization settings do you have on your browser?  We might
be able to reproduce better with that.
Actually, if you could do me a favor and open a new bug report on that, it would
be appreciated.

Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
rkaa: I managed to reproduce, I think I've found the problem.  I'll file the bug
and make the patch when I have surfed it for a while and it doesn't crash.
This is total dogfood for me...I've crashed six times in the past five minutes
doing typical work with Mozilla: searching lxr, querying bugzilla, looking up
incidents on talkback.  I'm going to have to use 4.x this weekend.

In the future, it seems like such a large rewrite of fundamental functionality
would benefit from test builds.
Blake: Why 4.x and not 0.9.8?
> I'll file the bug
> and make the patch when I have surfed it for a while and it doesn't crash.

jkeiser: bug # ?
It's fixed already (fix went in Saturday) ... bug 125950.
Cannot verify this since it is code level. Can one of the developers please verify?
looking at lxr i see that the new code sucks much less then the old :-)

verifying
Status: RESOLVED → VERIFIED
Blocks: 151443
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: