OnChange handler on a text input is called before and after the submit if the action is the same page

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: olivier sannier, Assigned: jst)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.9 +
wanted1.9 +
wanted1.8.1.x -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] post 1.8-branch, URL)

Attachments

(4 attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070716 MultiZilla/1.8.3.0a SeaMonkey/1.1.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070716 MultiZilla/1.8.3.0a SeaMonkey/1.1.3

When the action of a form element is the same page where it is located, and that there is a OnChange event handler on an input in that form, the handler is called once before submit and once after submit. If the code in the OnChange handler triggers a submission via a setTimeout call, the form is submitted twice, the first time with the actual values, the second with the values set from the server.
This has been reproduced here in PHP but was initially discovered with ASP.Net generated pages where one cannot control that easily what is generated. Especially, the key factor here is that the submit from the OnChange handler is triggered with a setTimeout.
Note that this bug is most visible when the network between the client and the server is very fast. If the load time of the page is too high, the time out call does nothing.
This is very annoying for ASP.Net based intranet systems where the network is as fast as it can and double validation of the form may cause havoc.

Reproducible: Always

Steps to Reproduce:
1. Go to above mentioned page (http://obones.free.fr/test.php)
2. Type a character in the input box at the top
3. Type the Enter key
Actual Results:  
Page is submitted twice, the post count is 2

Expected Results:  
Page is submitted once, the post count is 1

Using a screen capture software like wink, one can see the actual sequence of events where there is an extra OnChange handler call after the submit call.

This has been tested with the latest publicly available versions of the following browsers:

IE7: no bug
Opera: no bug
Firefox: bug
Seamonkey: bug

So it seems this is a gecko related issue, maybe coming from the fact that the page contents are reused instead of cleared like what IE does.
(Reporter)

Comment 1

10 years ago
Created attachment 274593 [details]
Test PHP page and screen capture

The capture was made with Wink and as you can see on frame 5, there is a call to DoChange() that setups a timeout that is triggered on frame 6, AFTER the first submit. That should not happen.
Confirming using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072905 Minefield/3.0a7pre

What's interesting is, that this only happens, when I have security.warn_submit_insecure set to false. With it set to true, the counter is incremented only by one after pressing enter.
Assignee: general → nobody
Status: UNCONFIRMED → NEW
Component: General → DOM: Events
Ever confirmed: true
Product: Mozilla Application Suite → Core
QA Contact: general → events
Version: unspecified → Trunk
(Reporter)

Comment 3

10 years ago
Yes. And if you put breakpoints in the javascript with Venkman, or server side (ASP.Net for instance), this does not happen anymore. It seems that for this to happen, the focus must remain on the page while the whole process is taking place.
Hence the reason why I did a capture with a third party program that does not interfere with this process.

Comment 4

10 years ago
Here's what happens when you type ENTER:
1. the keypress handler executes and the script sets up a timer to call
   DoPostBack() which calls submit(). Note that the timer is on the
   'window' object.
2. the time arrives: the form is submitted, a POST request is made
3. when the request is completed the old document will be replaced with
   with the result of the POST request; but before the old document 
   goes away a 'blur' event is issued to the currently focused content,
   which is the <input> and as part of the blur handling we also check
   if the user changed the value which is true (we typed some text),
   so a 'change' event is fired leading to your DoChange() script
   being executed, which sets up a new timer that will eventually
   call submit(). Again, the timer is on the same 'window' object.
4. the old document is replaced by the new result document
5. the time arrives for the 2nd timer - we lookup the form in the *new*
   document and submit it.

You can have even more fun if you change 'onchange' to 'onblur'
in your PHP script, then it will submit forever...

Assuming it's correct that the window object stays the same, I don't
see anything wrong with the event handling per se.

(In reply to comment #2)
> What's interesting is, that this only happens, when I have
> security.warn_submit_insecure set to false. With it set to true,
> the counter is incremented only by one after pressing enter.

When the pref is true you get a security warning dialog, which steals focus
and blur the <input> at an earlier stage.
OS: Windows XP → All
Hardware: PC → All
(Reporter)

Comment 5

10 years ago
To me this is bug because it does not happen with any other browsers that I know of. I tested it with MSIE, with Opera, no "double submit" whatsoever.
I know that the test is with a PHP script and that it can be changed very easily but this was first discovered with ASP.Net on the server and I have no way to control what gets generated.
I can understand that the timeouts are not cancelled when a page is reloaded, even if I find it strange. However, why would the OnBlur be called when the page is recreated? It's intended use, as I see it, is on a loaded page, not a page that is "in between".
My 2 cents.
Er... why _aren't_ the timeouts getting canceled?  That seems wrong to me.  Unless it's late enough that we've already canceled them?  But then we shouldn't be running event handlers by that point, I would think.
Flags: blocking1.9?
(Assignee)

Comment 7

10 years ago
Not a blocker unless this is a regression. We'd more than likely take a fix if one shows up before too long.
Flags: blocking1.9? → blocking1.9+
Whiteboard: [wanted-1.9]
Flags: blocking1.9+ → blocking1.9-
Hmm.  So what's going on here is that the new page starts loading, the script on the new page runs, and focuses the input on the new page.  This causes a blur to be dispatched to the input on the old page (SendFocusBlur sends a blur to the gLastFocusedContent, which is the input on the old page).  So the blur handler for the old input runs at this point (when the old input's document has already lost its script global, etc), and runs against the same outer window (of course).  So it sets a timeout on the _new_ inner window (which I think is very very wrong).  Then the timeout fires, etc.

The only real issue here (barring bug 199430 which I think is invalid) is that we're setting the timeout on the new inner window.  That would be a splitwindow regression, I guess: before that the handler would just throw I would assume.  I'm really not sure what to do about that, though, since |window.setTimeout| will naturally end up setting on the current inner window...  Could we somehow detect that the script is running on a different one of our inner windows and bail out?  Alternately, do we want to not fire JS event handlers once the document's mScriptGlobalObject has gone away?  Would that be good enough?

Note that this could also be used for content injection into the newly loading page or whatever else you want to do to it (subject to same-origin restrictions, but still).
Renominating as this scares me a little.
Flags: blocking1.9- → blocking1.9?
Whiteboard: [wanted-1.9] → [wanted-1.9][sg:?]
(Assignee)

Updated

10 years ago
Assignee: nobody → jst
Flags: blocking1.9? → blocking1.9+
(Assignee)

Comment 11

10 years ago
Created attachment 275851 [details] [diff] [review]
Make window.setTimeout() register the timeout on the calling scope when called on the callers own window.

This fixes this bug by ensuring that window.setTimeout() n' friends always forward to the right inner window. This also adds a new JS engine API to get the global object from a JSObject (which wasn't strictly needed, but the code has been duplicated in enough places already that it seemed worth it).
Attachment #275851 - Flags: superreview?(bzbarsky)
Attachment #275851 - Flags: review?(mrbkap)
(Assignee)

Comment 12

10 years ago
Comment on attachment 275851 [details] [diff] [review]
Make window.setTimeout() register the timeout on the calling scope when called on the callers own window.

brendan, wanna look over the JS_GetGlobalForObject() addition in jsapi here?
Attachment #275851 - Flags: review?(brendan)
Comment on attachment 275851 [details] [diff] [review]
Make window.setTimeout() register the timeout on the calling scope when called on the callers own window.

>diff --git a/dom/src/base/nsGlobalWindow.cpp b/dom/src/base/nsGlobalWindow.cpp
>+  if (IsOuterWindow()) {
...
>+    if (callerInner->GetOuterWindow() == GetOuterWindow()) {

The GetOuterWindow() could just be 'this'.
Attachment #275851 - Flags: review?(mrbkap) → review+
Comment on attachment 275851 [details] [diff] [review]
Make window.setTimeout() register the timeout on the calling scope when called on the callers own window.

>+++ b/dom/src/base/nsGlobalWindow.cpp
>+    if (callerInner->GetOuterWindow() == GetOuterWindow()) {

We know we're an outer window here; so shouldn't the RHS of the comparison just be |this|?

sr=bzbarsky with that.  This looks great.  Thanks for picking it up!
Attachment #275851 - Flags: superreview?(bzbarsky)
Attachment #275851 - Flags: superreview+
Attachment #275851 - Flags: review?(mrbkap)
Attachment #275851 - Flags: review+
Comment on attachment 275851 [details] [diff] [review]
Make window.setTimeout() register the timeout on the calling scope when called on the callers own window.


>+JS_PUBLIC_API(JSObject *)
>+JS_GetGlobalForObject(JSContext *cx, JSObject *obj)
>+{
>+    JSObject *parent;
>+    while ((parent = OBJ_GET_PARENT(cx, obj)))
>+        obj = parent;
>+
>+    return obj;

Style nit: move the blank line up to come after the declaration, not before the return statement.

Otherwise looks good to me modulo mrbkap and bz's comments and r+ marks.

/be
Attachment #275851 - Flags: review?(brendan) → review+

Updated

10 years ago
Attachment #275851 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 16

10 years ago
Created attachment 275856 [details] [diff] [review]
Updated fix for checkin.
(Assignee)

Comment 17

10 years ago
Fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Should get in a test for this..
Flags: in-testsuite?
(Assignee)

Comment 19

10 years ago
Created attachment 276219 [details] [diff] [review]
Additional change needed to fix orange.

This was needed in addition to the fix that landed here to fix tinderbox orange due to reftests crashing from this code recursing to death. Turns out that it's not always guaranteed that the caller scope is an inner. In the case of reftest, it appears as if it's calling setTimeout(setTimeout, ...) (I didn't actually find the source code that does that, but that's what it looked like in the debugger) and in that case the running code (chrome) is running in the scope of the outer window. When this happened, callerInner in the patch wasn't actually an inner (it was the outer from which we were forwarding, i.e. |this|. The reason for that is the way the prototype sharing is done between the inner and outer windows, the setTimeout() function's parent is the outer window, since the inners XPConnect prototype is the outer windows prototype.
Flags: wanted1.8.1.x-
Whiteboard: [wanted-1.9][sg:?] → [wanted-1.9][sg:?] post 1.8-branch
Flags: wanted1.9+
Whiteboard: [wanted-1.9][sg:?] post 1.8-branch → [sg:?] post 1.8-branch
Whiteboard: [sg:?] post 1.8-branch → [sg:critical?] post 1.8-branch
You need to log in before you can comment on or make changes to this bug.