72906 - Double click on submit causes form to submit twice[form sub]

Status: VERIFIED FIXED

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

Description

[Eric Pollmann]

In bug 72773, Brendan notes that clicking on the submit button twice will cause
the form to be submitted twice.  We need to disable form submission if a
submission is already in progress.

To reproduce the problem:
  - View the attached test case
  - Click on the submit button
    Note the Request ID you are returned (a)
  - Double click on the submit button
    Note the new Request ID you are returned (b)
Observed result: b-a == 2
Expected result: b-a == 1

The Request ID is incremented once per request, so if the request ID has gone up
by two, your double-click resulted in two form submissions, when it should only
have resulted in one.

Comment 1

[Eric Pollmann]

Oops, to reproduce the problem:
  - View the attached test case
  - Click on the submit button
    Note the Request ID you are returned (a)
**- Click on the "back" button**
  - Double click on the submit button
    Note the new Request ID you are returned (b)

Comment 2

[Eric Pollmann]

Attachment: test case

Comment 3

[Brendan Eich [:brendan]]

Nominating.  I'd like to nominate for 0.9, cuz I've done this twice by accident,
but maybe it falls off the priority list.

/be

Comment 4

[Eric Pollmann]

Attachment: patch

Comment 5

[Eric Pollmann]

The problem is that, since form submission is not synchronous, every time a form
submit event occurs (could be initiated by JS, clicking on submit button,
pressing Enter in a text input, ...), a new load event will be thrown on the
queue.  There is nothing preventing multiple events from being thrown on the
queue, as long as they are all added before one of the load events causes the
document to go away.

This patch ignores any requests after the first one.

This should not cause a problem with the case where someone fires a submit event
but cancels it in a JS event handler, because in this case, the event will be
cancelled before nsFormFrame::OnSubmit is called.

Comment 6

[Brendan Eich [:brendan]]

That patch looks out of date w.r.t. the trunk top revision of nsFormFrame.cpp.

How about using PRPackedBool instead of adjacent PRBools in a class or struct?

Forgive my ignorance, but what if the form targets another window, or otherwise
does not lead to its own destruction on submit? Won't the mAlreadySubmitted
flag be stuck set, and you'll never submit again without reloading the page? I
didn't see code other than in the dtor to clear mAlreadySubmitted.

/be

Comment 7

[Eric Pollmann]

Oy, both very good points, I'll have to think about this more.  The problem with
targetting another frame is going to be tricky... thanks for pointing that out!

Comment 8

[Brendan Eich [:brendan]]

When any of

(1) response from server (or JS engine if form's action URL is javascript:...)
comes back;
(2) submit times out;
(3) user aborts

you want to clear that flag.

Also, I think you need to cue the front-end to make the submit button appear
insensitive or disabled when setting the flag, and re-enable it when clearing
the flag.

/be

Comment 9

[Eric Pollmann]

Tom, shouldn't we be getting a single "double click" event instead of two click
events and a "double click" event?

Comment 10

[hirata masakazu]

Will this fix prevent double submission at
http://www.ncbi.nlm.nih.gov/entrez/query.fcgi ?
If you hit return twice, or double click "Go", the current result is overlapping
content in single window.
This disables Mozilla from responding to any input.

Comment 11

[Eric Pollmann]

Pushing back - end of milestone reality check.

Comment 12

[Kevin McCluskey (gone)]

*** Bug 76146 has been marked as a duplicate of this bug. ***

Comment 13

[Kevin McCluskey (gone)]

Added crash keyword because Bug 76146 which has been marked as a dup of the bug
results in a crash

Comment 14

[Eric Pollmann]

Removing crash keyword.  I'm not seeing a crash on the bug that was marked a dup
anymore.

Comment 15

[Blake Ross]

Missed 0.9.3.

Comment 16

[Jaime Rodriguez, Jr.]

This is P1 | Major for TM 0.9.4, and pollmann is no longer with us?

Asa, any ideas on who should get this one? --> trudelle

Comment 17

[Chris Waterson]

cc'ing rods 'cause it's a form control; saari because of events.

Comment 18

[Peter Trudelle]

I've got nothing to do with form submission.  ->rods?

Comment 19

[Kevin McCluskey (gone)]

Set milestone to mozilla0.9.5

Comment 20

[rods (gone)]

over to eric

Comment 21

[Kevin McCluskey (gone)]

Marking nsbranch-. 

Comment 22

[elaineking]

removed keyword nsbranch since it now has nsbranch-, per pdt mtg.

Comment 23

[rods (gone)]

taking bug, I have a fix for this with a different bug

Comment 24

[rods (gone)]

See patch in Bug 85286, it uses the nsIWebProgressListener to know if the submit 
was successful.

Comment 25

[rods (gone)]

Yes, the Request IDs are now correct, one after the other.

Comment 26

[rods (gone)]

Fixed

Comment 27

[Vladimir Ermakov]

verifying on 2001-01-10 branch

Comment 28

[Vladimir Ermakov]

verifying on 2001-01-10 branch on windows 98

Comment 29

[rods (gone)]

I had to back out the fix

Comment 30

[rods (gone)]

moving to 0.9.6

Comment 31

[rods (gone)]

reassinging to new owner of form submission

Comment 32

[Alexandru Savulov]

retargeting to 0.9.8

Comment 33

[Alexandru Savulov]

it think that's a matter of event handling
if not, reassign back. thx!

Comment 34

[John Keiser (jkeiser)]

Alex, I don't think there's any way for the button to know whether the form is
currently in the process of being submitted ... they could disable the button
after submit, but then there are other submit buttons on the screen that the
user can click, and there is JavaScript form.submit() that can trigger it.  I
really think it's best to handle this at the form submit level.

Comment 35

[Alexandru Savulov]

well the botton could prevent a second click anyway. if it was clicked once it
could deactivate itself be cause is not needed anymore.

Comment 36

[John Keiser (jkeiser)]

If we just disable the button when the user clicks it, we have to reenable it in
case the submit fails, the user cancels, or the submit completes in another
frame.  It's just as hard to do there as it is in form submit, and it gives more
benefits when done in form submit.

Comment 37

[Alexandru Savulov]

But otherwise we have to work with flags, observers and so on to prevent double
submitting and my feeling tells me that we're going to run into troubles.

I know, dissabling the button is not a problem free solution either. particulary
the case where the user cancels the submiting or as John mentioned above where
submission fails is where we have to renable the button.

John:

If you feel that you have a solution let me know please. Also feel free to take
ownership if you want.

Comment 38

[John Keiser (jkeiser)]

The proper solution is to set an isSubmitting flag in the form to TRUE when you
start the submit and have a listener find out when the LoadURI() finishes for
any reason, who then sets the flag to FALSE.  See Eric's patch (which was fine
as far as it went) for the first part of the solution.

Events have to do precisely the same thing, it's just that their "flag" is the
disabled attribute on the button.

Comment 39

[Alexandru Savulov]

ok! convinced :-)

re-taking

Comment 40

[tradervik]

You guys realize that developers all over the world have spent thousands of
hours of effort to develop server side and JavaScript solutions to prevent users
from double clicking their way to purchasing two copies of the same item, etc? 
It's great if you can build a solution into the browser but, in my not so honest
and eminently ignorable opinion, this one should have a low low priority, even
if Mr. Eich is prone to double submitting patches. ;-)

Comment 41

[Alexandru Savulov]

oh man! we still want to fix this one. even though IE does exactly what we do
now :-) 

Comment 42

[Alexandru Savulov]

Attachment: just a solution proposal patch (not a definitive fix proposal)

this patch is ment to go in the docshell. i don't really know if this will be
accepted, but it works pretty nice and does not need a listener. take a look at
it folks an tell me what you think. I will attach a testcase that simulates
pretty much all kind of problems. unfortuantely the response server is
available only within the firewall. sorry 'bout that.

what does this patch do?

is adding a registry object to keep track of every submission recording the
referrer URL and DocShell (the place where the user clicks on submitt) and the
target URL and DocShell (the place where the resulting page will be displayed).

If there is a submission going on, and something (user or JS) tries to submitt
until the submission didn't finish loading, the entry will be found and the
submission canceled. when the document ends loading, all the entries related to
the target docshell will be removed so that other submissions can be done.

Folks:
please take a look at this and tell me what you think. I need to know what can
be done better here. Also tell me where do you think that the new created
objects should go + there si a forced cast from nsIDocShell to nsDocShell that
may be written differently and so on. I know that this patchis still ugly and
needs a bunch of cosmetic improvements. I didn't wanted to make a final patch
be cause I'm not sure if everybody accepts this solution.
thx

Comment 43

[Alexandru Savulov]

ps: do you think I should rather use nsXPIDLCString instead of autostring?

Comment 44

[Alexandru Savulov]

Attachment: testcase for the patch

Comment 45

[John Keiser (jkeiser)]

My two cents:

I would prefer to have it in the form with a listener personally, for many
reasons (readability and performance high among them).  I don't really like
having a registry like that--it'll have some strange quirky behaviors, for one
thing, because it works with URLs rather than forms.  And doing it in DocShell
means we waste time iterating over the form controls in nsFormSubmitter before
we find out we really don't want to submit.

I looked into it.  Listeners really aren't that bad :)  You implement
nsIObserverService (or extend nsObserverService) in DocShell and implement
nsIObserver in the form and wallah, listeners ready-made.  You can think of it
as just one guy calling a method on a bunch of other guys when an event occurs.
 It doesn't even have to be a true service--you don't have to export it to
anybody and deal with any of that crap.  Just call the AddObserver() method on
the DocShell directly from the form.

You won't be the only one to use the observer thingy in DocShell, too :)  I
strongly suspect I am going to use the service (with an extra event for document
unload) to help implement bug 108309.  And it looks like there's already a call
in EndPageLoad() that is essentially a notification to the mDocumentViewer.

Comment 46

[Alexandru Savulov]

John:

but we have to keep track of which DocShell initiated a submit. Don't we?

I tried to imagine an observer service that gets notified everytime something 
and i cannot go around this kind of list or whatever keeps the information alive
about the referrer and target shell. I agree with you: implementing an observer
is an elegant solution. Btw: do you think to have the DocShell itself to be the
observer? What if is getting destroyed? (ok, i suppose that it won't submit
anymore :-) Maybe the observer should maintain that list and live independent of
any DocShell. I don't know yet. I'll try to change the patch.

(BIG HMMMM....)

Comment 47

[Alexandru Savulov]

from john's comment:

"And doing it in DocShell means we waste time iterating over the form controls
in nsFormSubmitter before we find out we really don't want to submit."

Yeah, that's where an observer is the bests way to go!

Comment 48

[John Keiser (jkeiser)]

I don't think the form would have to remember which DocShell initiated the
submit.  It just has to add itself as an observer to the DocShell that it calls
LoadURI() on.  Then it can sit back, relax and wait for the stop_load event to
roll in.  (Essentially the DocShell is keeping track of which *form* initiated
the submit--in its list of observers.)

ObserverService is actually the equivalent of Java Observable.  It is the guy
who is sending out the events (in this case the DocShell).  He is responsible
for remembering who was interested in the events so that he can tell them when
the events happen.  The Observer is the guy who receives the events (in this
case the form).

Doesn't DocShell stay around for the entire period of the browser / tab / frame?
 I admit ignorance to this object, it was my impression that it was a single
object that may contain (or work with) many documents.  So if the DocShell is
being destroyed, that means our document is going away and therefore, so is the
form.  So we really have no trouble :)  It is only when the DocShell still lives
that we care about it.

But you bring up a good point--which DocShell do we listen to to find out when
the page stops loading?  If the start happens on one object (like the current
document) and the end happens on another (like the target frame) then does the
EndPageLoad() method happen on the targetted frame's DocShell or the form's
DocShell?  (It may be both.)

In that case a possible solution is to add an nsIObserver parameter to LoadURI()
that specifies the observer to be notified when the final document is finished
loading.  I really think this will turn out to be useful to others.  It seems
like a common operation.

Comment 49

[Alexandru Savulov]

Yes!

If the target of an action is for example "_blank", the target DocShell that
ends the load (executes EndPageLoad) is another one.

take a look at the following code:
NS_IMETHODIMP
nsDocShell::InternalLoad(nsIURI * aURI,
                         nsIURI * aReferrer,
                         nsISupports * aOwner,
                         PRBool aInheritOwner,
                         const PRUnichar *aWindowTarget,
                         nsIInputStream * aPostData,
                         nsIInputStream * aHeadersData,
                         PRUint32 aLoadType,
                         nsISHEntry * aSHEntry)
{
....
    if (aWindowTarget && *aWindowTarget) {
....
        if (!targetDocShell) {
            rv = FindTarget(name.get(), &bIsNewWindow,
                            getter_AddRefs(targetDocShell));
        }
....
        if (targetDocShell) {
....
            rv = targetDocShell->InternalLoad(aURI,
                                              aReferrer,
....
        return rv;
    }
....

     rv = DoURILoad(aURI, aReferrer, owner, aPostData, aHeadersData);
}


If we don't add that information, the target DocShell would never know who is
waiting for the end of the load. On the other side only the initiating DocShell
knows for sure which is the target DocShell. The form cannot know that in
advance  if the target is another one than the current DocShell.

But there is a big problem in all the implementations: 
Is when the target docshell is getting destroyed before it can finish its job.
If  EndPageLoad is not called, the listener needs something like a timeuot or so
to make the submission possible again. (The proposed registry object does not
remove the blocking entry). Sometimes it can happen that we get a hang (very
often seen in Mozilla :-), the throbber "throbbs", and nothing happens. The user
gets angry and clicks on the X of the new window and destroys the DocShell. From
that point,
goodbye my EndPageLoad. Now if the DocShell would notify the listener before is
getting destroyed, that might solve the problem. (also the registry could be
cleaned up in the destructor ofthe docshell).

John, could you send me a sketch example about how do you think to create the
observer and observer service? I didn't get to the point of finding out how to
do that.
Thx!

Comment 50

[Alexandru Savulov]

sorry i didn't finish a phrase:

"if the target is another one than the current DocShell..."
the only thing the form knows is the fact that the target DocShell will be
another one at the submitting point.

Comment 51

[Alexandru Savulov]

Ok John, i got it!

- I will create a new topic for the existent nsIObserverService "@mozilla.org/
observer-service;1" called [whatever];
- As you said, I add/implement the nsIObserver to the form;
- I add a new member to the DocShell called mReferrerForm (stores the form
pointer or whatever we have to identify the form) and a method to set it;
- In DocShell::InternalLoad I set the mReferrerForm of the target DocShell;
- At the beginning of every submission, I set a mIsSubmitting in the form that
can be reset only by the future Observe method of the form

What do you think about his?

Comment 52

[Alexandru Savulov]

oops, i forgot:

- in nsDocShell::EndPageLoad I notify the observers (in this case, the form) the
load has finished for the mReferrerForm. Every Observer looks and the one that
matches, resets his mIsSubmitting.

Comment 53

[John Keiser (jkeiser)]

You pretty much got it! ... but instead of storing it in a member variable I'd
add it to the list of observers on that topic.  Otherwise the mechanism is still
non-general.

Here's what I'd do:
- add a member mIsSubmitting to nsHTMLFormElement
- implement nsIObserver in nsHTMLFormElement (sets mIsSubmitting to false and
calls RemoveObserver(this) on the DocShell's observer service)
- set mIsSubmitting to TRUE at the beginning of form submit

- add a new topic for "end_page_load" to the observer service in DocShell
- add a parameter nsIObserver* endLoadListener to LoadURI() or whatever function
is being called
- call the target DocShell observer service's AddObserver(endLoadListener,
"end_page_load") when you find out the target doc shell
- on EndPageLoad(), call the observer service's NotifyObservers(new
PRSupportsInt32(aStatus), "end_page_load", "")

(Note: I am not 100% certain that the 1st and 3rd parameters to NotifyObservers
there are right.  I wanted to pass aStatus so that we can replace some code that
already exists in EndPageLoad()--see that call where it calls out to another
place with aStatus as the parameter?)

Essentially AddObserver takes the place of mReferringForm.  NotifyObservers()
will call Observe() on everyone who is listening on that topic.

Comment 54

[Alexandru Savulov]

Attachment: patch to fix the bug

this is the observer / observer service version of the patch.

What does this?

- Is extending the class nsHTMLFormElement with nsIObserver and   
  nsSupportsWeakReference.

- Is implementing an own version of ::Observe that reacts on "end_page_load"

- Is adding an nsObserverService member to the nsDocShell

- Is calling AddObserver with a pointer to the referrer nsHTMLFormElement in
the
  method ::InternalLoad (the method arg list had to be extended with one 
  argument)

- Is calling NotifyObservers in the method nsDocShell::EndPageLoad;
  be cause the observer supports weak references, if the observer is not there
  the notification won't occur

- the class nsObserverService declartion is adapted to export the methods
  (compiles on Win32, but will it compile on Mac / Linux? not sure)

need r= / sr=

Comment 55

[Alexandru Savulov]

Attachment: replaces the prevoius patch

i forgot to notify the observers in the destructor

Comment 56

[Alexandru Savulov]

Attachment: similar patch but this time nsDocShell doesn't inherit from nsObserverService

same descrpition as aabove the only dofference is that in this one we create an
enumerator class so the DocShell does not have to inherit from
nsObserverService over the module boudary

needs r= / sr=

Comment 57

[Alexandru Savulov]

we're not yet ready with it

re-setting milestone

Comment 58

[Alexandru Savulov]

re-setting milestone (was wrong)
reassigning to john

Comment 59

[Kevin McCluskey (gone)]

Marking nsbeta1+

Comment 60

[Kevin McCluskey (gone)]

Marking topembed+

Comment 61

[John Keiser (jkeiser)]

Attachment: Patch (nsIWebProgressListener)

- created an nsIRequestor interface who is notified when the DocShell creates a
request.
- extended nsIWebProgressListener in nsHTMLFormElement so that we can find out
when our request is finished.

1a. When form submission starts, set mIsSubmitting to true.  Disallow all
submissions from this form while it is true.
1b. The form (in the form of an nsIRequestor) is passed to the link click
handler on the stack.
2a. DocShell/WebShell handle the request and, when they create a channel, pass
the nsIRequest* to the form (using RequestCreated())
2b. The form remembers the nsIRequest* and registers itself as an
nsIWebProgressListener on the DocShell.
3a. The nsIWebProgressListener eventually sends OnStateChanged(STATE_STOP) to
signify that, for whatever reason, the submit is finished.
3b. The form unregisters itself as an nsIWebProgressListener and sets
mIsSubmitting to false.  One can submit again if one so chooses.

Comment 62

[John Keiser (jkeiser)]

I have tested this patch on normal submissions, the "double submit with ENTER"
problem, and with a form that opens new windows on submit.  All do as expected:
they don't submit twice unless the previous submit has completed.

Comment 63

[Judson Valeski]

cc'ing alecf for review of the new dependency of uriloader in layout.

note: Rick's mods to nsIWebProgressListener in
http://bugzilla.mozilla.org/show_bug.cgi?id=46856 will affect the
nsIWebProgressListener impl and usage in this patch. if rick lands first, be
sure to change your impl. I'll add a similar note to rick's bug so he knows to
catch this if you land first.

Comment 64

[Alec Flett]

Comment on attachment 71613 [details] [diff] [review]
Patch (nsIWebProgressListener)

this dependency is disappointing to me mainly because it seems really strange
for the form element to be an nsIWebProgressListener.. 
it seems like this should be handled at the webshell/docshell level - i.e.
catching the interruption of a 2nd submit to the same URL from the same
docshell... but maybe at that level you can't tell the difference between a
form submit and some other form of GET/POST?

I guess having a DOM element be a web progress listener is just strange to me,
especially given the amount of overhead (5 extra methods added to
nsHTMLFormElement)

on a totally unrelated note, I'm also saddened to see that nsWebShell has
nsString*'s as member variables (mURLSpec and mTargetSpec) as opposed to just
being nsString.. any chance you can fix that (since you switched us to nsCOMPtr
right there anyway)

Comment 65

[John Keiser (jkeiser)]

This cannot be handled at the nsWebShell/nsDocShell level alone--not to be done
right, anyway.  One form can submit to multiple DocShells (in fact, it *will* do
this if its target is _blank).  The only place to reliably block a second submit
is in the form.

Here's a way around the dependency if that's the problem.  If we had nsDocShell
remember the form that submitted a given request, it could check for that when
the request is finished, and call SetSubmitting(PR_FALSE) on the form.

I don't think I like having form-specific stuff in DocShell, though.  This
really doesn't feel like DocShell's responsibility.  DocShell has way more than
enough stuff to worry about without having to mother forms too.

nsString*: I can change nsString* to nsString on the next iteration, if that's
what you're after?

Comment 66

[John Keiser (jkeiser)]

One other current reason to use this solution is bug 49361: there is a request
out there for a form submit observer method that tells the observer when the
channel was opened.  This solution could fulfill that request easily.

As for future reasons, it is a presumption of mine that nsIRequestor will be
useful.  I imagine there must be others out there who would be interested in
narrowly focused listening to only their own request and no others.

Comment 67

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I have no meaningful insight into the form machinery, but "nsIRequestor" seems
like a pretty bad name.  What are you requesting?  nsIDocShellRequestObserver
would seem like a better name, though I then wonder if we can't make do with one
of our existing nsIObserver-esque interfaces.

Comment 68

[John Keiser (jkeiser)]

Good point about the name, I'll change that.

However, I have a personal dislike for nsIObserver; the nature of the interface
doesn't tell you much about why you are extending it or what events you'll be
receiving from it.  It also makes it harder to send arbitrary bits of data
across.  I prefer listener classes a la Java, and since there are other places
in the code that do it that way (i.e. nsIWebProgressListener) that's how I'd
prefer to do it.

Comment 69

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I don't like nsIObserver either, to be honest, and I'm perfectly happy to hear
you say "I looked at it, and it doesn't work for me".  I just don't like
_inadvertent_ reinvention. =)

Comment 70

[Alec Flett]

ok, so I spent some more time with this patch, and here are my impressions:
1) we need something like nsIWebProgressListener that has more granular
information about the request, such as the nsIRequest that necko started with,
and that seems to be where nsIRequestor comes in.
2) there are times when nsIWebProgressListener doesn't fire, in places that this
code needs it to
3) we're not actually using the "aRequestId" parameter

It really seems like we ought to somehow extend the semantics of
nsIWebProgressListener, or make it fire more often. the other thing that I'm
confused about is how we're uniquely identifying requests - I see a
pointer-comparisons between nsIRequests. Is the requestID stored in nsIRequest? 

I also notice that necko has this concept of nsIRequestObserver - and the
onStopRequest has a "statusCode" result which might tell us why the request
stopped. 

I guess that independent of the dependency issue, I'm seeing
yet-another-specialized-listener-interface for network
connections/requests/etc.. are we sure that there isn't already a mechanism in
place (or one that could be slightly tweaked) for the notifications?

Comment 71

[John Keiser (jkeiser)]

1. If I understand you, yes.  nsIRequestor fills in the blanks *before*
nsIWebProgressListener picks up the phone.  Essentially, you have to be able to
associate what you give to DocShell (the thing that will eventually generate a
request) with an nsIRequest.  But at least LinkClickThis is a general pattern
that can (and probably will) be used by others who are now merely guessing which
request is theirs.

2. In what places does nsIWebProgressListener not fire that it needs to? 
Talking to rpotts, it sounded like we should always get start/stop events as
long as OpenURI() fulfills its contract, but if it doesn't it will return
failure code and I will fire nsIRequestor::RequestFailed(), which means I am no
longer *expecting* a stop event.  I only expect a stop event if RequestCreated()
happens but RequestFailed() does not.

And wouldn't nsIWebProgressListener not firing affect the throbber and
everything else?  Issues with nsIWebProgressListener need to be fixed there; if
it breaks form submission it's actually a good thing, because maybe it will get
more attention.  If the design of the nsIWebProgressListener interface itself is
broken, I would like to know about that before using it :)

3. We're not using aRequestId for this particular use of nsIRequestor, but in
the general case I expect we will.  It exists so that you can ask for multiple
requests at once and identify which nsIRequest is which.

4. aRequestId is solely to identify the request to the nsIRequestor.  It is not
even unique across nsIRequestors.  From then on, the unique ID for a request is
the nsIRequest* itself, AFAICT.  That is actually the entire purpose of this
interface: to get me the nsIRequest* so I can filter requests on that.

5. As to tweaking another mechanism: if nsIRequestObserver always fires *before*
STATE_START, then we could tweak it and add two parameters to OnStartRequest:
the event source (nsISupports*) and a request ID unique for that source.  Then
the source could add itself as a listener on that source and filter where source
== this.  One big potential hole in this is, we don't even know which DocShell
we will end up firing the request from.  It may even be a new window that hasn't
been created yet.  So we definitely won't be registered on that source for
nsIRequestObserver notifications.  We'd still need a notification that tells us
which window our request eventually got created on.

Comment 72

[Alec Flett]

ok, it's good to get this all sorted out. I didn't realize that this stuff was
happening before nsIWebProgressListener starts doing things. I didn't mean that
it was broken, just that it didn't (per its contract) put out the notifications
that you were interested in. 

And so, what I'm wondering is if nsIRequestObserver would fill your need? It's
much less frozen than nsIWebProgressListener (Which is UNDER_REVIEW, so could
also be changed to meet your need) but I don't know enough about it either,
maybe we can get darin in on this? Seems like it might simplify your code if you
didn't have to set up a listener, in order to set up another listener :)

Comment 73

[Darin Fisher]

nsIRequestObserver is UNDER_REVIEW and really shouldn't be messed with :-)

that being said, i'd like to understand better why it doesn't satisfy as is.

in the patch, a nsIRequestor is implemented by the caller of LoadURI.  they want
to know when the Load starts and when the load finishes.  they want to know if
there was a problem.  this is the kind of thing that nsIRequestObserver was
designed to handle.

  nsIRequestObserver::OnStartReqeust(aRequest, aContext)

can replace

  nsIRequestor::RequestCreated(aRequest, aDocShell, aRequestID)

you'd want to dream up some interface w/ aDocShell and aRequestID getters.

likewise,

  nsIRequestObserver::OnStopRequest(aRequest, aContext, aStatus)

can replace

  nsIRequestor::RequestFailed(aRequest, aDocShell, aRequestID)
  nsIRequestor::RequestNotCreated(aRequestID)

just come up with meaningful nsresult's pass as aStatus that correspond to
the various event chains (e.g., "RequestCreated -> RequestFailed ->
RequestNotCreated")

it could work like this:

  1) always fire OnStartRequest to indicate that things are starting to happen
  2) fire
       OnStopRequest(..., NS_OK)
       // to indicate success
     or
       OnStopRequest(..., NS_ERROR_XXX_REQUEST_NOT_CREATED)
       // to indicate that the request could not be created
     or
       OnStopRequest(..., NS_ERROR_XXX_REQUEST_FAILED)
       // to indicate that the request is created but fails such that no
       // nsIWebProgressListener calls will be made.

in fact, why do you need to observe nsIWebProgressListener events?  why not just
make OnStopRequest fire when the nsIRequest is done?  isn't that the only event
you really care about?  it looks like you're only observing the
OnStateChange(STATE_STOP) event.

keep in mind: nsIRequestObserver requires OnStartRequest and OnStopRequest to
come in pairs.

finally, why can't you use the nsIRequest in place of a numeric aRequestID?  i
guess it depends on who is creating aRequestID... hmm.

Comment 74

[John Keiser (jkeiser)]

The problems here are twofold:

1. I don't know which DocShell I'll be listening to since I could be submitting
to a new or different window
2. A single DocShell can handle many requests at once, and I need to know just
when the actual submit request finishes (not, for example, images or other content).

Perhaps aContext could handle the second problem?  What do we do about the first?

aRequestId is generated by the requestor, so that I can identify which
nsIRequest coming back from the DocShell is which in nsIRequestor in case I send
out multiple requests.

Comment 75

[John Keiser (jkeiser)]

Oh, and another thing I'm worried about: can I even *add* myself as an
nsIRequestObserver on the form?  Some observer patterns are 1-1--multiple people
cannot listen at once.  I haven't found the place where RequestObservers are
added to whoever is firing them.

Comment 76

[John Keiser (jkeiser)]

Attachment: Patch (synchronous click)

This version does not need a new interface.  Instead it has a synchronous
LinkClick method on nsILinkClickHandler that we call and get back the
nsIRequest* and nsIDocShell* immediately.  Also fixed other issues of
reporters.  uriloader dependency still exists.	nsIWebProgressListener still
exists.  Either the form has to listen to the request or else docshell has to
listen and tell the form what it hears.

I prefer this method; it's more explicit about what we're doing.

Comment 77

[Alec Flett]

After a once-over on the patch, I'm definitely happier with this solution, even
though the dependency still exists. It seems more explicit what's going on here.. 
I'll review this today...

Comment 78

[John Keiser (jkeiser)]

Attachment: Patch (synchronous click)

The previous version of this patch was missing a diff from webshell.  That is
included at the end of the patch, everything else is identical.

Comment 79

[Jesse Ruderman]

Is this bug asking to modify the behavior of all forms, or just post forms? 
Also, is it about double-clicks submitting twice, or is it about two separate
clicks submitting twice?

I often click the submit button a second time when a search result page doesn't
start to load.  I would be annoyed if that stopped working.

Comment 80

[John Keiser (jkeiser)]

If it can't contact the server at all, then you will have to click stop before
you can submit again.  If it submits to the same window and takes a long time to
load, when you hit back submit will work again.

Comment 81

[Alec Flett]

Comment on attachment 72899 [details] [diff] [review]
Patch (synchronous click)

ok, I guess I can't see any way around the dependency right now, but I'm glad
we cleaned this up anyway...

the patch looks fine, sr=alecf but I'd like to see reviews from both darin and
a layout person (just so everyone is happy on both ends)

Comment 82

[Darin Fisher]

Comment on attachment 72899 [details] [diff] [review]
Patch (synchronous click)

r=darin... looks good to me.

Comment 83

[Alexandru Savulov]

layout is happy (particulary me :-)

r=alexsavulov

Comment 84

[Asa Dotzler [:asa]]

Comment on attachment 72899 [details] [diff] [review]
Patch (synchronous click)

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 85

[John Keiser (jkeiser)]

Fix checked in.  Bug 130491 is filed for visual representation of the
non-submittable state.

Comment 86

[Vladimir Ermakov]

Verifying on windows 98 build 2002-03-13-08-trunk
and mac OSX build 2002-03-13-08-trunk

Comment 87

[Vladimir Ermakov]

*** Bug 130579 has been marked as a duplicate of this bug. ***

Comment 88

[Vladimir Ermakov]

verifying on build 2002-03-13-09-trunk linux RedHat 7.0 as well

Comment 89

[Jesse Ruderman]

Caused regression: bug 133895, offline cache miss on form submission breaks form
permanently.

Comment 90

[Alec Flett]

I'm seeing other wierd issues with certain forms where a normal submission
results in a cache warning (i.e. for instance stuff about POSTDATA expiring even
though I'm submitting the firm, not hitting forward/back/etc)

Comment 91

[Darin Fisher]

alec: i'm seeing that too, especially w/ the 2002-03-26-08 build under linux. 
it seems to happen very often when i focus on another window before the form
submission completes.

Comment 92

[John Keiser (jkeiser)]

Hmmm ... I suggest filing another bug, that sounds like it might be related but
might not too.  (I am sort of skeptical, as bug 72906 is only passive--it only
listens, and does not change the character of the first submit.  Subsequent
submits just don't happen--there's no chance for a partial submit or anything
like that.)