chrome/test/data/password/create_form_copy_on_submit.html - Issue 331593008: Only consider PasswordFromManager which HasCompletedMatching when provisionally saving passwords

Side by Side Diff: chrome/test/data/password/create_form_copy_on_submit.html

Issue 331593008: Only consider PasswordFromManager which HasCompletedMatching when provisionally saving passwords (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Test added Created 6 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch | Annotate | Revision Log

« chrome/browser/password_manager/password_manager_browsertest.cc ('K') | « chrome/test/data/password/between_parsing_and_rendering.html ('k') | chrome/test/data/password/form_utils.js » ('j') | chrome/test/data/password/form_utils.js » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!--

	2 This page enables to simulate the following scenario:

	3 On password form submit, the page's JavaScript creates another
	engedy 2014/07/11 17:05:35 nit: Spell it out here more that there is a static nit: Spell it out here more that there is a static form, and a second form is created. vabr (Chromium) 2014/07/11 18:15:47 Done. Show quoted text On 2014/07/11 17:05:35, engedy wrote: > nit: Spell it out here more that there is a static form, and a second form is > created. Done.
	4 form, copies the user's password into it, and submits that.

	5

	6 The issue demonstrated here is that there is very little time

	7 between creating and submitting the second form. As observed in

	8 http://crbug.com/367768, PasswordManager is not able to process
	engedy 2014/07/11 17:05:35 nit: Add one more line here to describe the proble nit: Add one more line here to describe the problem is with not being able to complete matching early enough. vabr (Chromium) 2014/07/11 18:15:47 Done. Show quoted text On 2014/07/11 17:05:35, engedy wrote: > nit: Add one more line here to describe the problem is with not being able to > complete matching early enough. Done.
	9 the form quickly enough in such cases. This test checks that it

	10 still makes use of the first form and works in this scenario.

	11 -->

	12 <html>

	13 <head>

	14 <script src="form_utils.js"></script>

	15 <script>

	16 function preCreatePasswordForm() {

	17 // Remember the filled in password + destroy the old form.

	18 var old_password = document.getElementById('password').value;

	19 document.getElementById('contains-form').innerText = '';

	20 // Spin the message loop to let the deletion settle in.
	engedy 2014/07/11 17:05:35 Could you please describe more why this is needed? Could you please describe more why this is needed? vabr (Chromium) 2014/07/11 18:15:47 Done. The problem is I'm not sure it is needed. It Show quoted text On 2014/07/11 17:05:35, engedy wrote: > Could you please describe more why this is needed? Done. The problem is I'm not sure it is needed. It would probably work without it, but I don't know Blink and V8 well enough to be certain there is not a race between the DOM modification and JS execution -- note that the form elements need to have the same IDs, so it would be bad if we got hold of a wrong version of the elements in the code below. Also, I'm not sure setTimeout actually guarantees that the above does not happen, it's just my attempt to give the system a chance and hope for the best. engedy 2014/07/11 18:26:11 Acknowledged. Show quoted text On 2014/07/11 18:15:47, vabr (Chromium) wrote: > On 2014/07/11 17:05:35, engedy wrote: > > Could you please describe more why this is needed? > > Done. The problem is I'm not sure it is needed. It would probably work without > it, but I don't know Blink and V8 well enough to be certain there is not a race > between the DOM modification and JS execution -- note that the form elements > need to have the same IDs, so it would be bad if we got hold of a wrong version > of the elements in the code below. > Also, I'm not sure setTimeout actually guarantees that the above does not > happen, it's just my attempt to give the system a chance and hope for the best. Acknowledged.
	21 window.setTimeout(createPasswordForm, 0, old_password);

	22 }

	23 function createPasswordForm(old_password) {

	24 // Create and append the new password form. It is almost the

	25 // same as the deleted one, only with a different action.

	26 addForm(createForm());
	engedy 2014/07/11 17:05:36 This might be a terribly bad idea, but perhaps thi This might be a terribly bad idea, but perhaps this method could take the action as an argument, and we could use it to create the first form as well. I am not sure when document.body gets created, though. vabr (Chromium) 2014/07/11 18:15:47 I'm not sure if it is a bad idea, I just don't see Show quoted text On 2014/07/11 17:05:36, engedy wrote: > This might be a terribly bad idea, but perhaps this method could take the action > as an argument, and we could use it to create the first form as well. > > I am not sure when document.body gets created, though. I'm not sure if it is a bad idea, I just don't see why that would be preferable. What were your reasons? I like the static form below, because it is easier to understand it's structure. Having said that, I acknowledge that the code hides the fact that the new form is almost the same. Maybe the createForm function could take an existing form as an argument, and copy that, but maybe that's just overengineering a simple test. WDYT? engedy 2014/07/11 18:26:11 Yes, I wanted to reduce the duplication, but I thi Show quoted text On 2014/07/11 18:15:47, vabr (Chromium) wrote: > On 2014/07/11 17:05:36, engedy wrote: > > This might be a terribly bad idea, but perhaps this method could take the > action > > as an argument, and we could use it to create the first form as well. > > > > I am not sure when document.body gets created, though. > > I'm not sure if it is a bad idea, I just don't see why that would be preferable. > What were your reasons? > I like the static form below, because it is easier to understand it's structure. > Having said that, I acknowledge that the code hides the fact that the new form > is almost the same. Maybe the createForm function could take an existing form as > an argument, and copy that, but maybe that's just overengineering a simple test. > WDYT? Yes, I wanted to reduce the duplication, but I think you are right, let us just keep the static form, it is cleaner.
	27 // Spin the message loop to let the creation settle in.

	28 window.setTimeout(postCreatePasswordForm, 0, old_password);

	29 }

	30 function postCreatePasswordForm(old_password) {

	31 // Copy over the old password + add a dummy username, and submit

	32 // the new form.

	33 document.getElementById('username').value = 'test';

	34 document.getElementById('password').value = old_password;

	35 document.getElementById('submit-button').click();

	36 }

	37 </script>

	38 <title>Test dynamically created password form</title>

	39 </head>

	40 <body>

	41 <div id="contains-form">

	42 <form action="none.html">

	43 Old Form (to visually distinguish it from the form it is replaced with):

	44 <label for="username">Username</label>

	45 <input type="text" id="username" name="username">

	46 <label for="password">Password</label>

	47 <input type="password" id="password" name="password">

	48 <input type="submit" id="submit-button" value="Don't click!">

	49 </form>

	50 <button id="non-form-button" onclick="preCreatePasswordForm();">

	51 Click!

	52 </button>

	53 </div>

	54 </body>

	55 </html>

OLD	NEW