350785 - Autocomplete / Form Manager stores element data even if Autocomplete is "off"

Status: VERIFIED FIXED

(Toolkit :: Form Manager, defect)

Description

[Anthony Baratta]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

Autocomplete / Form Manager stores element data even if Autocomplete is "off".



Reproducible: Always

Steps to Reproduce:
With Autocomplete set to "off", the Form Manager is still storing the value of the form element in the element "history".

Create a page with a single element called "testOne". And a submit button. Set the form action to "get". Set testOne Autocomplete = "off". Copy page one to page two and remove the autocomplete element.

Navigate to page one, enter text in testOne, submit form. Navigate to page two, use down arrrow to activate element history and you should see previous element value from page one.

Actual Results:  
Previous Page One element testOne history data shows in page two testOne History.

Expected Results:  
No history data should appear on pageTwo testOne element.

I don't know how severe this is, but it is disconcerting. Could someone use the element history and JS to grab data that shouldn't be there?

Comment 1

[Gervase Markham [:gerv]]

*** This bug has been marked as a duplicate of 198419 ***

Comment 2

[Phil Ringnalda (:philor)]

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

Comment 3

[Phil Ringnalda (:philor)]

Pretty sure wallet and satchel are still quite forked, so it can't be a duplicate of something in that "Core" component that deals with code in /extensions/. Sigh.

Bug 201850 gave form autocomplete the sense to not *fill to* forms with autocomplete="off", but not the sense to not *remember from* them.

Comment 4

[Phil Ringnalda (:philor)]

Attachment: testcase

To test: submit all five forms, then delete the default value in the fifth input, and type a q.

Expected: only quux5 is offered for autocomplete.
Current actual: all five are autocompleted.

Comment 5

[Phil Ringnalda (:philor)]

Attachment: trunk patch

Trunk version:

- copies over the autocomplete code from nsFormFillController.cpp to not store autocomplete="off" inputs
- cleans up some Aviary-landing of bug 256079 in the choice of string functions in nsFormFillController.cpp
- fixes the maybe-unreported bug that we don't store form data in an <input type="TEXT">, only in type="text".

Not quite sure how much of that I can sneak into a 1.8.1.1 version. I only fixed nsStorageFormHistory.cpp, since I don't see any way to build nsFormHistory.cpp on the trunk any more; I'll mark this as "depends" on bug 340871, which will make sure nobody will build the unfixed one.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 241925 [details] [diff] [review]
trunk patch

Sorry, I don't think I'll have time for this (and I'm no longer a Toolkit peer).

Comment 7

[Phil Ringnalda (:philor)]

Comment on attachment 241925 [details] [diff] [review]
trunk patch

Whee, review-shopping. Not sure who wouldn't fear having to own satchel if they review in it.

Comment 8

[Benjamin Smedberg]

Comment on attachment 241925 [details] [diff] [review]
trunk patch

I'm worried about performance. nsIContent.AttrValueIs would be a performance win over string handling on nsIDOMHtmlInputElement.

If you think that's overkill, we should at least avoid creating NS_LITERAL_STRINGs in a loop. Make them NS_NAMED_LITERAL_STRING before the loop:

NS_NAMED_LITERAL_STRING(kAutoComplete, "autocomplete");
for (;;) {
  ...
  inputElt->GetAttribute(kAutoComplete, autocomplete);
};

Comment 9

[Phil Ringnalda (:philor)]

Attachment: trunk v.2

It's entirely possible I don't know how to properly use AttrValueIs, or maybe PR_Now(), but I'm getting nothing from it: for 1000 inputs (with the name-values already stored, since most of the time and pain is there), I get ~100ms either way. For reasonably sized forms, that's well below my ability to notice, but either way since dveditz wants this for 1.8.1.1, I'd like to land something I can port to the branch first, and optimize it in a new bug if AttrValueIs really is an optimization.

However, that first patch was completely insane, checking the form for every input. Unreasonable as it seems to me, we, IE, and Opera agree that in a <form autocomplete="off">, nothing on an <input> can turn it back on, and only one of Microsoft's two "spec" articles says anything that makes Safari seem even possibly right to let <input autocomplete="on"> turn it back on, so we can just bail early.

Comment 10

[Benjamin Smedberg]

Comment on attachment 244011 [details] [diff] [review]
trunk v.2

I'd like unit tests for this as well.

Comment 11

[Phil Ringnalda (:philor)]

Anyone who can point me toward finding the values in the autocomplete popup from JS, for tests, feel free to speak up.

Comment 12

[Robert Sayre]

I'm willing to help get this test in the tree.(In reply to comment #11)
> Anyone who can point me toward finding the values in the autocomplete popup
> from JS, for tests, feel free to speak up.

This one looks tricky, so we'll figure it out together. It may be that we don't have the test infrastructure we need just yet, but we should still have the test ready to go in the bug.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 244011 [details] [diff] [review]
trunk v.2

r=mano

Comment 14

[Phil Ringnalda (:philor)]

Trunk:
toolkit/components/satchel/src/nsFormFillController.cpp 1.78
toolkit/components/satchel/src/nsStorageFormHistory.cpp 1.13

Comment 15

[Phil Ringnalda (:philor)]

Attachment: 1.8.1 v.1

1.8.1 branch version, same stuff in a different file, and minus the unrelated unprivacy bit to remember type="TEXT".

Comment 16

[Asaf Romano (gone)]

Comment on attachment 244863 [details] [diff] [review]
1.8.1 v.1

I just realized both patches check the autocomplete attribute even if this is not a text-type input, please optimize this on trunk as well.

Comment 17

[Daniel Veditz [:dveditz]]

Not blocking, but if you get through reviews we'll look at approving for 1.8.1 branch, maybe for 1.8.0 branch.

Comment 18

[Phil Ringnalda (:philor)]

Attachment: Trunk bail early v.1

Comment 19

[Phil Ringnalda (:philor)]

Attachment: 1.8.1 v.2

Comment 20

[Asaf Romano (gone)]

Comment on attachment 245307 [details] [diff] [review]
Trunk bail early v.1

nit: add an empty line after the continue statement before checkin.

r=mano.

Comment 21

[Asaf Romano (gone)]

Comment on attachment 245314 [details] [diff] [review]
1.8.1 v.2

ditto

Comment 22

[Phil Ringnalda (:philor)]

Trunk w/blank line: toolkit/components/satchel/src/nsStorageFormHistory.cpp 1.14

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 245314 [details] [diff] [review]
1.8.1 v.2

approved for 1.8 branch, a=dveditz for drivers

Comment 24

[Phil Ringnalda (:philor)]

1.8branch: toolkit/components/satchel/src/nsFormHistory.cpp 1.26.4.8

Comment 25

[Hasham]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1pre) Gecko/2008062306 GranParadiso/3.0.1pre

Verified. With autocomplete off, input items are not remembered.

Comment 26

[Robert Sayre]

dolske> philor: good news, we totally test that.