109854 - Can't log into the Technology Credit Union web branch any more :-(

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Johnny Stenback (:jst)]

Sometime roghly between one and two weeks ago mozilla stopped letting me log in
to the TechCU website. To reproduce this bug you need an account at TechCU, I'm
filing this mostly to get this problem on file.

From what it looks like, when you try to log in, you enter a account number and
a pin, then that's submitted and you get a page that says "Verifying your
login..." and then that page is redirected (?) to the real page. In stead of
mozilla going to the real page, mozilla shows a 404 error :-(

Did someone change the redirect code lately? Is there some debugging I could
turn on to see what's going on here?

Comment 1

[Johnny Stenback (:jst)]

s/debugging/logging/

Comment 2

[Johnny Stenback (:jst)]

Taking bug, it's not a network problem after all, it's a form submission
problem, or rather it's a problem with form.elements[] being incorrecly ordered
so the form submission ends up submitting the form control values in the wrong
order.

Comment 3

[Johnny Stenback (:jst)]

John, here's the bug about DemoteContainer() messing up form.elements[].

Comment 4

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Comment 5

[Johnny Stenback (:jst)]

The attached patch makes sure HTMLContentSink::DemoteContainer() doesn't mess up
the ordering of form.elements[], the way I ensure that it doesn't change the
ordering is a bit odd but for performance reasons something like this is our
best option, AFAICT.

jkeiser, r=?

Comment 6

[Johnny Stenback (:jst)]

The attached patch makes sure HTMLContentSink::DemoteContainer() doesn't mess up
the ordering of form.elements[], the way I ensure that it doesn't change the
ordering is a bit odd but for performance reasons something like this is our
best option, AFAICT.

jkeiser, r=?

Comment 7

[rpotts (gone)]

Comment on attachment 58672 [details] [diff] [review]
Proposed fix.

sr=[e-mail]

Comment 8

[John Keiser (jkeiser)]

Attachment: Testcase of underlying problem

Comment 9

[John Keiser (jkeiser)]

Attachment: Broader Fix

From discussions offline, adding a check in nsIFormControl not to do anything
at all with setting the form if the form is frozen.  Without doing this, mForm
will be null for a certain period of time even when it should not be.

I am not sure why we're doing the assertions on getter functions though?  Won't
these work just fine if the form is frozen?

Comment 10

[John Keiser (jkeiser)]

Attachment: Actual Patch

Oops, that was another testcase, not a patch.  This is the patch.

Comment 11

[John Keiser (jkeiser)]

(These comments and patch submits were done in Bugzilla, BTW, an avid
DemoteContainer() user, so it's looking fairly peachy.

BTW, the IndexOfControl() assertion is firing all the time on Bugzilla.  Since
it does no harm, I suggest we simply remove the getter assertions.

Comment 12

[John Keiser (jkeiser)]

Attachment: Patch with assertions removed

Comment 13

[Johnny Stenback (:jst)]

jkeiser, I don't see much of a difference between my original patch and your
patches, did you not include all the changes (i.e. nsIFormControl)?

I added the assertions to make sure we know we're not calling any methods that
are not intended to be called while the form is frozen. AFAIK most methods
should work just fine while a form is frozen, but anyone who calls those methods
while the form is frozen is probably doing something wrong. IndexOfFormControl()
might be an exception to that.

Comment 14

[John Keiser (jkeiser)]

Yes, the only difference is in nsGenericHTMLElement really (the SetForm
methods).  I was initially going to change Freeze() and IsFrozen() to
*DemotingContainer() but then I realized that if we are deleting the <form> that
the controls live in are not necessarily the ones being demoted--i.e. you could
have <form><somethingelse><input><div></somethingelse></div></form>, which could
demote <somethingelse> and call SetParent() on the <input>.  Currently I hear we
only demote forms, but the method could conceivably  demote something else. 
Better safe.  So freeze() makes more sense.  I left stuff in nsHTMLFormElement
for safety as well.  If someone else tries to mess with the form while it's
frozen it should not change--it should be consistent.

nsIFormControl will not need to change for this regardless--just SetForm() or
SetParent().  I chose SetForm() in case others try to set the form while it's
demoting.

I'm OK with just deleting the assertion in the IndexOfFormControl() method if
that's probably the only exception.  (Gotta go right now though, I can do it
tonight or r= if you do it today.)

Comment 15

[John Keiser (jkeiser)]

Hmm, thinking about it on the way to work, (Set/Is)DemotingContainer() could be
construed as "we're demoting some container or other right now so please don't
add or remove yourself" rather than "we're demoting this form".  Making the
method be just a flag like that would allow us to get rid of most of the stuff
in nsHTMLFormElement.  I'll tend to it this evening.

Comment 16

[John Keiser (jkeiser)]

Attachment: Simpler Patch

This patch makes it simpler and still works.  Now nsHTMLFormElement does
nothing but store the flag; SetForm in the form controls actually checks it and
trusts that it's not supposed to do anything.

Comment 17

[Johnny Stenback (:jst)]

Let's not let the notion about "demoting" containers creep outside the sink,
call it Freeze/IsFrozen, or something else, but IMO we should stay away from the
"demotecontainer" name. If we really want to call this method demote_something_
lets not put it on nsIForm, put it on the sink and make the code that cares ask
the sink, but I don't think we need to go there.

John, there are two ::SetForm() methods in nsGenericHTML*Element (leaf and
container), you'll need to do them both (i.e make them identical).

Will you make these changes or do you want me to?

Comment 18

[John Keiser (jkeiser)]

Both the leaf and the container are done in the patch, and as far as I can tell,
they are the same.

As to the name ...

We are only going to *use* this for DemoteContainer(), why not be open about it?
 I don't think we want to proliferate this method and encourage other people
using it as workarounds by making it seem general.  If the need really arises,
we can change the name, but that should be a major decision, and changing the
name would highlight the seriousness of the decision to reviewers.  Plus, it
will help people understand the *why* as well as the what, and it will flag it
with a Big Red Flag so that we can delete it later when DemoteContainer() is
(hopefully) replaced by some more sane method.

As a sort-of-unrelated note, I was thinking of an enhancement that would make it
a bit safer--instead of a boolean, pass the currently demoting control to the
form, and the form control can check that and see if *it* is the control in
question.

I think that, if we must use a generic name, we should either:
(a) Make It Functional: keep the name "Freeze()" (or call it FreezeControls())
and use the third or fourth patch here which implements the freeze both on the
form control *and* the form (the name should be consistent--if it really freezes
the form, it should freeze all possible additions to the form)
(b) Make It A Flag: make another windier name like ControlsDontAddThemselves().
 and use the fifth patch, which is simpler but does not necessarily cover all
future cases where people try to change the form array.  Making it a flag
highlights exactly what it's doing.

Comment 19

[John Keiser (jkeiser)]

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

Comment 20

[Johnny Stenback (:jst)]

Duh, yeah, you did change both methods, sorry for not seeing that in the patch
at first glance.

WRT the name, you bring up good points, naming it [G|S]etIsDemotingContainer()
is fine, but I don't think we need to make this bullet proof, i.e. having it be
a boolean is IMO good enough. *Nobody* should ever use this method in any other
situations than when "demoting" a container in the sink, it's not a generic
method. We should comment that well in the interface.

Comment 21

[John Keiser (jkeiser)]

Does this mean the latest patch is ready to go?

I agree, we shouldn't really *need* the IsDemotingContainer(nsIContent*) instead
of (PRBool), I was just thinking it might deal with subtle bugs like moving
<input> around in DemotingContainer() triggering a real remove or add of a
different control (which would fail because DemotingContainer() is set for *all*
controls instead of just the one that is being moved), or threads (which I have
to keep pounding into my head we don't have).  These are fringe problems though,
probably won't happen.  I'm just paranoid :)  I'm happy enough with the current
patch.

Comment 22

[Johnny Stenback (:jst)]

bz, wanna review this one? :-)

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 59025 [details] [diff] [review]
Simpler Patch

>+            // If the child is a form control, cache the form that
>+            // contains it.  After the form control is removed from
>+            // it's container, restore it's form.

Hmm...	This section gives me pause

1)  There is no need to remember and restore the form now that we're
    freezing it, is there?
2)  What happens in the following case:

<form>
<div>
  <input id="one"/>
</div>
<div>
<input id="two">
</form>
</div

a)  The <div> is moved to be a child of the form's parent.  The form
    is _not_ frozen before doing this, since the <div> is not an
    nsIFormControl
b)  The form is frozen (since we now get to the second <input/>
c)  The <input> is moved to the form's parent

Since we eventually want the removal of that first <div> to cause
its descendant controls to reset their form pointers this will
not really work....

If I understand correctly, "container" will _always_ be a form.
Would it be possible to just QI it to nsIForm and freeze it before
moving _any_ children and unfreeze once all the children are moved?
(probably want an assertion here in case QI fails...).

A better (but slower) solution would be to walk up the content tree from 
"container" and freeze the first form we find, if any.

Comment 24

[John Keiser (jkeiser)]

1) "There is no need to remember and restore the form now that we're freezing
it, is there?"

Good point, I totally missed that.  I'll change that tonight.

2) "... we eventually want the removal of that first <div> to cause
its descendant controls to reset their form pointers ..."

We don't want anything resetting the form pointers, I thought ... ATM, we don't
crawl into the DIV, why would we ever want to?

3) "If I understand correctly, "container" will _always_ be a form.  Would it be
possible to just QI it to nsIForm and freeze it before moving _any_ children and
unfreeze once all the children are moved?"

It would certainly be more efficient and cleaner *if* we can make that
assumption.  But I have this thing about function names.  If container will
always be a form, we need to encode that assumption somewhere lest others come
in and start using DemoteContainer() for other stuff.  We could change the
function name to DemoteForm() (which I think isn't a bad idea anyhow) or we
could include a warning in the comments for DemoteContainer().  I'm OK with
that, but unless we have to do it for #2, I'm also OK with what we're doing
now--this operation isn't all that costly.

Finding the parent won't make this work if we ever start demoting other stuff
... for example, if we start demoting <div> and the user does this:
<otherblockelement><div><form><input></otherblockelement>, then the *first* time
it will find the form properly and the content will look like this:
<otherblockelement><div><form></form><input></otherblockelement>, but the
*second* time it won't find the form anymore.  One can imagine other such scenarios.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> We don't want anything resetting the form pointers, I thought

Let me clarify.

Right now, if we have:

<form>
<div><input></div>
</form>

and we removeChild() the <div>, the <input> does not change its form pointer. We
eventually want it to (bug 98487) _except_ when demoting.  Hence my comment. 
With your proposed fix, things would still break when we would fix removeChild()
to do the right thing.

Comment 26

[John Keiser (jkeiser)]

Oh.  You're right then.  Then I think we need to do as you suggested, and *also*
rename DemoteContainer() to DemoteForm() and call it with a form pointer. 
Because DemoteContainer() as it stands will only demote everything correctly
*if* they are forms, and the only way to make a real DemoteContainer() that
worked on non-forms would be to recurse into children and see if there are any
form controls.

Comment 27

[John Keiser (jkeiser)]

Attachment: New Improved Evil-Grin Patch

OK, so now:
- *Demot(e|ing)Container is changed to *Demot(e|ing)Form everywhere.  I have
changed the name of "result" to "rv" in the procedure and given it more verbose
comments.  (The addition of the if made it so I had to move everything over
anyway, so I figure changing the name of a variable won't hurt.)
- I removed all special form control crud from DemoteForm()
- we call form->DemotingContainer(PR_TRUE) *once* at the beginning of
DemoteForm() instead of multiple times as previous.  (Actually it's not
precisely at the beginning.  We actually call it after the form has been
force-added to its parent just in case some script or something happens there.)


So in short, other than everything I haven't changed anything.

Comments, *r=?	I am submitting this patch with it and it fixes the testcase. 
Other examples of heavy form-using DemoteForm()-triggering pages would be
useful, but it looks like we're golden.

Comment 28

[Johnny Stenback (:jst)]

Attachment: Same as above, but diff -wu

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 59502 [details] [diff] [review]
Same as above, but diff -wu

>+nsHTMLFormElement::SetDemotingForm(PRBool aDemotingForm)
>+{
>+  NS_ASSERTION(mDemotingForm != aDemotingForm,
>+               "Bad call to DemotingContainer()!");
DemotingContainer() should be SetDemotingForm there.

>+        // Set mIsDemotingForm here so that the above FlushTags()
>       // calls really flushes the tags.
Make that 'calls' 'call'.

Other than that, r=peterv.

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 59502 [details] [diff] [review]
Same as above, but diff -wu

sr=jst

Comment 31

[Johnny Stenback (:jst)]

Fix checked in, marking FIXED.

Comment 32

[benc]

qa-> default.

Comment 33

[Johnny Stenback (:jst)]

Verified, I'm now able to log into the bank...