Closed Bug 350785 Opened 14 years ago Closed 13 years ago

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

Categories

(Toolkit :: Form Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: Anthony, Assigned: philor)

References

Details

(Keywords: fixed1.8.1.1, privacy, Whiteboard: [sg:want P2])

Attachments

(4 files, 2 obsolete files)

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?

*** This bug has been marked as a duplicate of 198419 ***
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
*** Bug 351700 has been marked as a duplicate of this bug. ***
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.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8.1.1?
Keywords: privacy
Whiteboard: [sg:want P2]
Component: Form Manager → Satchel
OS: Windows XP → All
Product: Firefox → Toolkit
QA Contact: form.manager → satchel
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha1
Version: unspecified → Trunk
Attached file 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.
Assignee: nobody → philringnalda
Status: NEW → ASSIGNED
Attached patch trunk patch (obsolete) — Splinter Review
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.
Attachment #241925 - Flags: first-review?(bryner)
Depends on: 340871
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).
Attachment #241925 - Flags: first-review?(bryner)
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.
Attachment #241925 - Flags: first-review?(benjamin)
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);
};
Attachment #241925 - Flags: first-review?(benjamin) → first-review-
Attached patch trunk v.2Splinter Review
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.
Attachment #241925 - Attachment is obsolete: true
Attachment #244011 - Flags: first-review?(benjamin)
Comment on attachment 244011 [details] [diff] [review]
trunk v.2

I'd like unit tests for this as well.
Attachment #244011 - Flags: second-review?(mano)
Attachment #244011 - Flags: first-review?(benjamin)
Attachment #244011 - Flags: first-review+
Anyone who can point me toward finding the values in the autocomplete popup from JS, for tests, feel free to speak up.
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 on attachment 244011 [details] [diff] [review]
trunk v.2

r=mano
Attachment #244011 - Flags: second-review?(mano) → second-review+
Trunk:
toolkit/components/satchel/src/nsFormFillController.cpp 1.78
toolkit/components/satchel/src/nsStorageFormHistory.cpp 1.13
Status: ASSIGNED → RESOLVED
Closed: 13 years ago13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Flags: blocking1.8.0.9?
Attached patch 1.8.1 v.1 (obsolete) — Splinter Review
1.8.1 branch version, same stuff in a different file, and minus the unrelated unprivacy bit to remember type="TEXT".
Attachment #244863 - Flags: first-review?(mano)
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.
Attachment #244863 - Flags: first-review?(mano) → first-review-
Not blocking, but if you get through reviews we'll look at approving for 1.8.1 branch, maybe for 1.8.0 branch.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9-
Attachment #245307 - Flags: first-review?(mano)
Attached patch 1.8.1 v.2Splinter Review
Attachment #244863 - Attachment is obsolete: true
Attachment #245314 - Flags: first-review?(mano)
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.
Attachment #245307 - Flags: first-review?(mano) → first-review+
Comment on attachment 245314 [details] [diff] [review]
1.8.1 v.2

ditto
Attachment #245314 - Flags: first-review?(mano) → first-review+
Attachment #245314 - Flags: approval1.8.1.1?
Trunk w/blank line: toolkit/components/satchel/src/nsStorageFormHistory.cpp 1.14
Comment on attachment 245314 [details] [diff] [review]
1.8.1 v.2

approved for 1.8 branch, a=dveditz for drivers
Attachment #245314 - Flags: approval1.8.1.1? → approval1.8.1.1+
1.8branch: toolkit/components/satchel/src/nsFormHistory.cpp 1.26.4.8
Keywords: fixed1.8.1.1
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.1-
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.
Status: RESOLVED → VERIFIED
Component: Satchel → Form Manager
dolske> philor: good news, we totally test that.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.