135679 - Form submit doesn't work if target is same page and current url has anchor

Status: RESOLVED WORKSFORME

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

Description

[Ere Maijala (slow)]

If form action attribute has same url as the current page and current url has an
anchor, submitting the form won't work. DocShell thinks that it is only an
anchor to the current page and doesn't submit it. Consequently there would be no
progress notifications and form element would believe that the submission is
still in progress, effectively disabling subsequent submission attempts also. 

Couldn't find out what was causing this before tracing the execution in
debugger. After debugging I think I have fairly good view of the problem and
also a possible fix. I'll attach a proposed patch to fix these.

Comment 1

[Ere Maijala (slow)]

Attachment: Patch to fix the submission

This patch prevents nsDocShell::ScrollIfAnchor() from treating URI without an
anchor as one with anchor. It will also reset nsHTMLFormElement's mIsSubmitting
if the submission didn't return docshell that could be used to notify when the
submission is complete.

Comment 2

[Alexandru Savulov]

reporter:

thanks a lot! let's see what jkeiser has to say about the patch, but i think is
ok. (jkeiser did the change that prevents forms to be submitted twice)


john:
check this out. i think you can say more than me to Ere's patch.

Comment 3

[Alexandru Savulov]

please consider nominating this bug if is very important. attach eventually a
test case too, to underscore its importance for adt and drivers. looks pretty
fundamental to me. 

Comment 4

[Alexandru Savulov]

Ere:

I think you're right with your patch but you need to redo it based on the newer
version of nsDocShell.cpp (there were some changes in between). The second part
in the nsHTMLFormElement.cpp makes sense anyway.


 Please review this and also attach a test case if you have one. we need it for
the approvals. Thanx!

Comment 5

[Alexandru Savulov]

retaking. john has enough bugs, i don't know why i reassigned this one in the
first line anyway. sorry:-)

Comment 6

[Alexandru Savulov]

i have an ideea for 

nsHTMLFormElement::DoSubmit()


- // Mark us as submitting so that we don't try to submit again
- mIsSubmitting = PR_TRUE;
+ mIsSubmitting = PR_FALSE;

  if (!actionURI) {
-   mIsSubmitting = PR_FALSE;
    return NS_OK;
  }

  if (aCancelSubmit) {
-   mIsSubmitting = PR_FALSE;
    return NS_OK;
  }

  if (docShell) {
    nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(docShell);
    NS_ASSERTION(webProgress, "nsIDocShell not converted to nsIWebProgress!");
    rv = webProgress->AddProgressListener(this);
+   mIsSubmitting = PR_TRUE;
  }

Comment 7

[Alexandru Savulov]

even better

  if (docShell) {
    nsCOMPtr<nsIWebProgress> webProgress = do_GetInterface(docShell);
    NS_ASSERTION(webProgress, "nsIDocShell not converted to nsIWebProgress!");
    rv = webProgress->AddProgressListener(this);
+   if(NS_SUCCEDDED(rv)) {
+     // only mark as submitting if we can add us as a progress listener
+     // otherwise who should we be notified 
+     mIsSubmitting = PR_TRUE;
+   }
  }

Comment 8

[Ere Maijala (slow)]

I was thinking about something like that in comment #7, but as I didn't know the
code well enough, I wasn't sure if it's ok to do that. Great, in this case we
can also get rid of the macro NS_ENSURE_SUBMIT_SUCCESS also and use the common
one instead. I'll try to get my build done and create new patch. Testcase coming
soon too.

Comment 9

[Ere Maijala (slow)]

Attachment: Simple test case

Open the page, click on the anchor link and try pressing test button.

Comment 10

[Ere Maijala (slow)]

Comment on attachment 77890 [details]
Simple test case

Oh, this needs to be saved locally before trying. Won't work directly from
bugzilla.

Comment 11

[Ere Maijala (slow)]

Nominating. Sorry for the spam flow.

Comment 12

[Alexandru Savulov]

er... you cannot add adt1.0.0 yet :-) 

we need  r= sr= a= first. removing, sorry

Comment 13

[Ere Maijala (slow)]

Attachment: v2 patch, improved according to the comments

Patch against the current version of nsDocShell.cpp. Includes suggested
changes.

Comment 14

[John Keiser (jkeiser)]

Comment on attachment 78004 [details] [diff] [review]
v2 patch, improved according to the comments

Apologies for the back and forth, but I'd prefer we left mIsSubmitting checks
the way they are.  The mIsSubmitting = PR_TRUE happens at the beginning of the
function to make it threadsafe.

Changes look good, good catch in both places!  r=jkeiser with this.

Comment 15

[John Keiser (jkeiser)]

I think I need to be more specific.  Specifically, setting mIsSubmitting =
PR_TRUE and doing NS_ENSURE_SUBMIT_SUCCESS() at the beginning of the function is
what makes it not threadsafe.  Besides, it really is submitting for the duration
of the function.  The part that is *good* is in the first patch, } else {
mIsSubmitting = PR_FALSE; }.  A check for rv after AddProgressListener() would
be good too.

CC'ing radha to verify the docshell change; it looks good to me.

Comment 16

[Ere Maijala (slow)]

Attachment: Patch v3, back to thread-safe implementation, check for rv

nsHTMLFormElement.cpp redone according to the comments by jkeiser.

Comment 17

[John Keiser (jkeiser)]

Comment on attachment 78076 [details] [diff] [review]
Patch v3, back to thread-safe implementation, check for rv 

r=jkeiser.  Excellent work, I am impressed :)

One nitpick: there were tabs in the nsHTMLFormElement.cpp file.  I can change
those before checking in, but in the future, just be aware you need to use
spaces.  http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl can check for a
lot of that kind of stuff for you.

Comment 18

[Marc Attinasi]

Comment on attachment 78076 [details] [diff] [review]
Patch v3, back to thread-safe implementation, check for rv 

sr=attinasi

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 78076 [details] [diff] [review]
Patch v3, back to thread-safe implementation, check for rv 

a=[e-mail]

Comment 20

[Alexandru Savulov]

Ere, 

this is already your bug :-) since you did great work on it. Please consider to
start the procedures for getting check-in rights for the CVS repository if you
didn't got/started that already.

Comment 21

[Alexandru Savulov]

reassigning

Comment 22

[John Keiser (jkeiser)]

Nominating for ADT to look at just in case ere can't find anyone to check in for
him.  This is a low-risk, low-to-medium-impact fix, but will be *very*
frustrating to people who come across it.

Comment 23

[Ere Maijala (slow)]

I've asked timeless to check this in to the trunk.

Comment 24

[Paul Wyskoczka]

Adt needs to approve bugs to Netscape contributors only.  Others need to go to
Drivers for approval.

Comment 25

[Ere Maijala (slow)]

Now that there's branch, I take it that this should be checked in to it also..

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in on branch

Comment 27

[timeless]

checked into trunk (tabs fixed)

Comment 28

[Miloslaw Smyk]

Fix for this bug caused bug #138313 to appear. I've provided a patch there,
please check if it does not break something for you.

Comment 29

[Radha on family leave (not reading bugmail)]

I think this bug should do some or all of the following to fix it the right way

1) Add code in nsDocShell::ScrollIfAnchor() and look for form submits, through
'Get' methods and prevent scrolling if any form submission is going on. This way
normal page loading will kick in, which will do the submit.

2) In nsDocShell::InternalLoad() look for postdata, and avoid getting into
ScrollIfAnchor() if postdata is found.

The patch here which got checked in on 4/10/02 totally breaks anchor traversals
(please see bug 138313), which is very bad for mozilla1.0 and the pending release.

I'm also not sure how many real websites are affected by this bug. If this is a
really bad problem, this problem should have shown up long time ago. DocShell's
behavior w.r.t. anchors has been this way for a long time. 

Considering the current timeline and all the checkin regulations in mind, I 
would like to backout this patch and restore proper anchor behavior.

Comment 30

[John Keiser (jkeiser)]

Reopening.  This is still working right now, but will shortly be broken again
after bug 138313 is fixed.

The problem is, it's just plain hard to distinguish between a GET and a normal link.

One way we could fix it is to check for a "?" in the URL we're going to and
force it to load the page.  that will "break" in the case where there are no
parameters.  But what we really need to be 100% accurate (we probably don't wnat
to have to reload the page when a link goes back to the same page with a
questionmark) is a flag or load type that flags it is a form submission.

At the same time, I think we should change the variable name since it's
misleading: aWasAnchor implies that there was an anchor.  It should be set if
there was an anchor, and not set if there was not.  But it's not always set,
even if there was an anchor.  aDidScroll is better, though even that is a
misnomer.  What the return value really means is, don't reload the page.  Radha
didn't like aDontLoadPage, but we ought to find something.

Comment 31

[Ere Maijala (slow)]

As John said, I think it would be best to create a new loadtype (LOAD_SUBMIT?)
which would be used in ScrollIfAnchor() to set aWasAnchor to false if this is
true. Comments?

Comment 32

[John Keiser (jkeiser)]

I was thinking for a minute that it might be necessary to *remember* that it was
a submit past the first time it is submitted; but I don't think it is.

One difficulty is, everywhere the DocShell checks the loadType (and there are a
number of them) you have to make the decision all over again whether to check
this one as well.  In this case it's somewhat easy; in others it might not be.

One thing that would be neat is if load types were masks containing semantic
information about what the DocShell should do with that load type, then adding
or changing load types like this would not be such a bother.  You could set all
sorts of flags on it so that things would not be so fragile (to prevent some of
this most recent fiasco--bug 138134); instead of loadType == LOAD_HISTORY ||
loadType == LOAD_RELOAD_NORMAL you could do loadType &
LOADTYPE_MASK_SHOULD_GET_HISTORY.  Then you define LOAD_HISTORY to be an amalgam
of flags, including LOADTYPE_MASK_SHOULD_GET_HISTORY.  I'd like to hear Radha's
perspective, but I suspect this might be useful.

Comment 33

[Alexandru Savulov]

check out bug 138422 also. is it a dup or only related?

Comment 34

[Ere Maijala (slow)]

I think that the patch v1.3 of bug 138134 will fix 138422 also. 

Comment 35

[Alexandru Savulov]

Ere,

in which stage are we now with this baby? (need this for your CVS account)

Comment 36

[Ere Maijala (slow)]

I'm still working on it.

Comment 37

[Ere Maijala (slow)]

Attachment: Patch v4, new fix for nsDocShell

This is a new simple fix for ScrollIfAnchor(). It will return without doing
anything if the url contains a question mark.

Comment 38

[Radha on family leave (not reading bugmail)]

The patch looks simple. I would like more info before I can give it a review.

1)  Does this actually trigger the normal page loading processes for GET
submits, ie., the page scrolls as well as submits the form?
2) Does such a page get registered with session history? ie., can you go
back/forward to the submit result page after you are done submitting?
3) Does the behavior look OK when you go back/forward to the submit result page? 
4) What kind of testing have you done with anchor traversals, including empty
anchors.
5) I presume POST based submits are not an issue. Am I right? Has that been tested?

Miloslaw,  If possible, Can you give the patch a spin. Thanks.

Please understand that I'm just trying to avoid further regressions at this point.

Comment 39

[Ere Maijala (slow)]

I've tested gets, posts, normal links and history (including back and forward)
with empty and non-empty anchor. All of these seemed to work correctly, but we
definitely need also someone else to test them to avoid the previous disaster.

Comment 40

[Ere Maijala (slow)]

Hmm.. nothing here. Is there anyone who could try it?

Comment 41

[Vladimir Ermakov]

Is the bug still present? I don't see the problem in 2002-05-13-08-trunk.

Comment 42

[Ere Maijala (slow)]

Indeed, it seems to work. I recall some changes (events?). Maybe they or
something had an effect on this? 

Comment 43

[Vladimir Ermakov]

resolving works for me.