390565 - HTML parser should not move <input type="hidden">

Status: VERIFIED FIXED

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

Description

[Marcelo Simon]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072522 Minefield/3.0a7pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072522 Minefield/3.0a7pre

Since this build I cannot access the internet banking service from my bank everything was ok before.

When I enter my account info it tries to call:
https://bankline.itau.com.br/GRIPNET/bklcgi.exe

and it display a message that the service is not available, but if I use a previous build I can access it.

Reproducible: Always

Steps to Reproduce:
1.go to www.itau.com.br
2.Fill account info ( I can provide this info if this bug is not public )
3.hit ok
Actual Results:  
it display a message that the service is not available

Expected Results:  
it should display a box with my name on it

Comment 1

[Marcelo Simon]

Here's the bonsai for this build
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1185421620&maxdate=1185426359

Comment 2

[Marcelo Simon]

Just to confirm the builds

this one works ->20070725_2047_firefox-3.0a7pre.en-US.win32.zip
since this one don't ->20070725_2206_firefox-3.0a7pre.en-US.win32.zip 

Comment 3

[Martijn Wargers (dead)]

If I understand the page correctly, you login info is not necessary.
I think the bug can also be seen when you fill in random numbers as account info, right?

Comment 4

[Martijn Wargers (dead)]

Attachment: testcase

The more specific url is https://bankline.itau.com.br/lgnet/BanklineItauPF.htm
The attached testcase shows what the problem is in current trunk builds.

During the onsubmit handler the value of the input is changed, but in current trunk builds this isn't posted anymore.

Comment 5

[Martijn Wargers (dead)]

Regression from bug 353415.

Login info is not needed, so this bug can become public now.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So... if I load the testcase, then click the button the URL that's loaded is <https://bugzilla.mozilla.org/attachment.cgi?a=this+should+be+posted>.

That's what I get with older builds too.  Martijn, is that not what you're seeing?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that the page _does_ have the sort of "hidden inputs in a bogus place inside the table" thing going on that bug 353415 would affect.  If they're also depending on posting order (which they might be if they use a hand-rolled exe to handle the post) we might have a problem. :(  But the testcase doesn't have anything like that in it...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Blake, how hard would it be to leave hidden inputs and only hidden inputs (not other kinds of inputs) as kids of <tr> or whatever the page sticks them inside?  If we did that, things would Just Work....

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 274869 [details]
testcase

(In reply to comment #6)
> So... if I load the testcase, then click the button the URL that's loaded is
> <https://bugzilla.mozilla.org/attachment.cgi?a=this+should+be+posted>.
> 
> That's what I get with older builds too.  Martijn, is that not what you're
> seeing?

Sorry, I'm seeing that too now (I thought I didn't, before). So this testcase is just wrong.

Comment 10

[Marcelo Simon]

Since what you thought this bug was about isn't do you need my account info to login to try to diagnose this ?

How can I help to get this fixed ?

Comment 11

[Martijn Wargers (dead)]

(In reply to comment #10)
> Since what you thought this bug was about isn't do you need my account info to
> login to try to diagnose this ?

No, we don't need your account info anymore, thanks for the offer. It's known what the source of issue is.

> How can I help to get this fixed ?

You can't help anymore atm, unless you can code yourself, then you could make a patch.
You can help again with testing afterwards, if a fix has been made.

Comment 12

[Johnny Stenback (:jst)]

Boris, can you work on this regression? If not, please reassign...

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm sort of waiting on Blake to see how easy this is to fix on the parser end.  If it's not, we have to redesign how form.elements works.... :(  And yeah, I guess I can do that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Damon, I think we should block for M10 on this one way or another.  Either to get this fixed, or to back out the various patches we've made to form control storage that have gotten us here (including support for the :default pseudo-class, probably).  I wouldn't feel comfortable with the sort of changes we need here after M10.  In fact I'm not sure how comfortable I am even then, but both going forward and going back are risky.

I'm sort of assuming Blake's not going to be able to find time to work on this at this point, so I'll try to find time to sort out the HTML parser magic myself, I guess.  :(

Comment 15

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

So the problem here is that we're moving <input type=hidden>s placed at bad places in a <table> to before the table? Thus rearranging their order when submitted since we are now better at keeping form.elements in the actual order of the DOM?

If so, this is actually worrying me a bit since this could actually happen to <input type=text> too. I wonder if we can add code to form.elements that keeps parser-moved form controls out of order? That seems like the best and safest solution.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> this could actually happen to <input type=text> 

It could, but unlike with hidden inputs there's visual indication that it's been moved outside the table in that case.  So while hidden inputs interspersed with table parts are common, text inputs in that situation are not....

As I see it, we have 4 main options here:

1)  Leave things as-is, wontfix this bug, evangelize sites that have this
    issue.  This last could be hard, since any site hitting this is not using
    any off-the-shelf CGI library, so would need to actually update their
    server-side code.
2)  Go back to storing form controls out of order in form.elements.  That
    means backing out bug 353415, bug 347165, bug 352980, and the "default
    submit button" part of bug 302186, since this last is pretty much
    impossible to implement efficiently if any mutation requires us to work
    the full elements array (which it would if it were out of order).  There
    might be other patches that would need to come out too; I'd have to
    look pretty carefully at the CVS log to make sure.  It would also make us
    violate the proposed HTML5 spec, but so will solutions 3 and 4, so that's
    not a big deal.  See the thread at <http://lists.w3.org/Archives/Public/public-html/2007Sep/0053.html>.
3)  Keep form.elements in document order, but submit in some other order.  This
    is what Ian proposes in <http://lists.w3.org/Archives/Public/public-html/2007Sep/0128.html>.
4)  Make the parser change I propose in this bug.

In my opinion, 2 is pretty risky due to the number of things we have to make sure we back out correctly, and removes a feature we want.  3 is even riskier because it's hard to get right.  See the responses to Ian in the thread cited above.  4 has the issue comment 15 points out, but I think that this is not likely to bite sites.

Safari has been doing 4 for a while, and they're doing OK with it, apparently.  They feel that this is what the HTML5 spec should say, in fact.  See <http://lists.w3.org/Archives/Public/public-html/2007Sep/0318.html>.

Comment 17

[Mike Schroepfer]

I'm guessing Blake is not likely to get to this before M10 - is there someone else who knows the parser code?  I'm nervous to touch this post b2...

Comment 18

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

Blake, any pointers here would really help as I don't know the innards of the parser code at all :(

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch, kinda

This is ugly.  Real ugly.  So ugly the rest of CNavDTD doesn't invite it to parties.  But it's the best I can think of as far as non-invasive changes go.  I tried doing a separate tag type for hidden inputs, and it's just too much pain I think.  Especially getting the nsHTMLTag -> string mappings to be sane.

The tests do cover the cases we care about, and this seems to work... Blake, what do you think?

Comment 20

[Boris Zbarsky [:bzbarsky]]

I guess I should perhaps take the code I'm adding to CNavDTD::HandleToken and reuse it in DoesRequireBody?  And use the GetAttributeCount() thing instead of just walking till I get to a non-attribute token?

I'd still like to get an overall sanity-check on the patch, but I suspect I'll have to make those changes, right?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: With that cleanup, and including tests for bug 66985

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 290725 [details] [diff] [review]
With that cleanup, and including tests for bug 66985

>Index: parser/htmlparser/src/CNavDTD.cpp
>+// when all the attributes for the input are still in the tokenizer.
>+static PRBool
>+IsHiddenInput(CToken* aToken, nsITokenizer* aTokenizer)
>...
>+  PRInt32 ac = aToken->GetAttributeCount();
>+  NS_ASSERTION(ac <= aTokenizer->GetCount(),
>+               "Not enough tokens in the tokenizer");

I'd rather make this assertion (and the one below) if/returns. CNavDTD does weird things with the tokenizer and things occasionally don't actually match up, though they might be all fixed by this point.

>+  for (PRInt32 i = 0; i < ac; ++i) {
>+    NS_ASSERTION(eHTMLTokenTypes(aTokenizer->GetTokenAt(i)->GetTokenType()) ==
>+                   eToken_attribute, "Unexpected token type");

...especially because if this assertion fails, it's an exploitable crash.

> static PRBool
> DoesRequireBody(CToken* aToken, nsITokenizer* aTokenizer)
> {
>   PRBool result = PR_FALSE;
> 
>   if (aToken) {

Perhaps this isn't the bug to make this change in, but this function is only called from HandleToken, which bails on a null aToken. So you could get rid of this if statement and the |result| variable (or maybe just inline the function into its one us).

>+      PRBool isHiddenInputInsideTableElement = PR_FALSE;
>+      if (aChildTag == eHTMLTag_input &&
>+          FindTagInSet(theParentTag, sTableElements,
>+                       NS_ARRAY_LENGTH(sTableElements))) {

FWIW, if this |FindTagInSet| turns out to be expensive, we could add some data to the element table to help make this decision.

Thanks for taking this on and sorry for not getting to it. I was worried that creating a new element table entry might be a bunch of extra work :-/.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> I'd rather make this assertion (and the one below) if/returns.

I left both assertions, but made us only go up to the PR_MIN of ac and GetTokenCount() when walking attributes, and made us bail out if we get a non-attr token.

> or maybe just inline the function

I did that, since at this point it's just a single boolean conditional, basically.

Checked in, with the tests.

Comment 24

[Marcelo Simon]

I'm the reporter of the bug and can confirm it as verified in the original url of the problem.

Thank you!

Comment 25

[rs.0]

WFM too.
Thanks.