Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1271053002:
[PasswordManager] Fix Flaky BrowserTest related to dynamically created (Closed)
|
Created:
5 years, 4 months ago by xunlu Modified:
4 years, 6 months ago CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[PasswordManager] Fix Flaky BrowserTest related to dynamically created
password form
For browserTests that dynamically create password form, add
PasswordStoreObserve to wait for response from PasswordStore before submitting
the form.
BUG=301547
Patch Set 1 : #
Total comments: 16
Patch Set 2 : comments #Patch Set 3 : Rebase #
Total comments: 1
Messages
Total messages: 22 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
xunlu@chromium.org changed reviewers: + gcasto@chromium.org
vabr@chromium.org changed reviewers: + vabr@chromium.org
Thanks, Xunlu, for fixing the broken tests! Your CL LGTM, as long as you are happy to address the comments below as suggested. Cheers, Vaclav https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:237: // Create password form. nit: This comment is not necessary, you named the string variable well enough, so it is clear what it does. (Also below.) https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:242: // Make sure PasswordStore::GetLogins() returned before submitting the form. This comment can be misunderstood. The observer sadly cannot observe the GetLogins issued by the PasswordFormManager for the added form, but only its own. It is a clever hack to use a response to a second GetLogins as an approximation for knowing that the first one has been processed, but we need to document this in case something goes wrong. Namely, it is still possible that the message about creating the form in the renderer gets to the browser later than the observer issues its GetLogin. In that case, the rest of the test might still be running too early. Therefore I suggest the following comment instead: // Issue an artificial PasswordStore::GetLogins() request and wait until the store returns a response. Hopefully, at that point, the PasswordFormManager created for the new form above will have received and processed the store's response as well. (Also below.) https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:243: password_manager::TestPasswordStore* password_store = (1) You do not need TestPasswordStore interface, just use PasswordStore. (2) Please keep the store in a scoped_refptr: scoped_refptr<PasswordStore> password_store = PasswordStoreFactory::GetForProfile(...); (Also below.) https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_test_base.cc (right): https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.cc:71: form, password_manager::PasswordStore::ALLOW_PROMPT, this); Please use DISALLOW_PROMPT instead, to mitigate the risk of getting stuck because of displaying a user-facing prompt to unlock the keychain. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.h:12: #include "components/password_manager/core/browser/test_password_store.h" You do not need this. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.h:49: class PasswordStoreObserver : public password_manager::PasswordStoreConsumer { I suggest against naming the class *Observer. It confusingly hints at PasswordStore::Observer, which it does not use. It also does not really observe the store (it does not see anything else than the response to its own GetLogins request). My suggestion: DummyStoreConsumer because it expresses more clearly what it inherits from, what it does, and that it does not care about the actual response from the store. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.h:51: explicit PasswordStoreObserver(password_manager::TestPasswordStore*); Please change TestPasswordStore references to PasswordStore in all of the class code. You are not using anything not contained in the base PasswordStore interface. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.h:59: void Wait(); The name is too short and vague. Also, it seems strange to pass the passwords store to the constructor and only use it in Wait(). I would suggest: (1) Get rid of test_password_store_ member variable, do not pass a store in the constructor. (2) Instead, pass the store as an argument of the current Wait() method, and do not store that argument anywhere. (3) Rename Wait to RequestLoginsAndWait.
Patchset #2 (id:100001) has been deleted
https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:237: // Create password form. On 2015/08/05 08:37:35, vabr (Chromium) wrote: > nit: This comment is not necessary, you named the string variable well enough, > so it is clear what it does. (Also below.) Done. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:242: // Make sure PasswordStore::GetLogins() returned before submitting the form. On 2015/08/05 08:37:35, vabr (Chromium) wrote: > This comment can be misunderstood. The observer sadly cannot observe the > GetLogins issued by the PasswordFormManager for the added form, but only its > own. It is a clever hack to use a response to a second GetLogins as an > approximation for knowing that the first one has been processed, but we need to > document this in case something goes wrong. Namely, it is still possible that > the message about creating the form in the renderer gets to the browser later > than the observer issues its GetLogin. In that case, the rest of the test might > still be running too early. Therefore I suggest the following comment instead: > > // Issue an artificial PasswordStore::GetLogins() request and wait until the > store returns a response. Hopefully, at that point, the PasswordFormManager > created for the new form above will have received and processed the store's > response as well. > > (Also below.) Your version is much more clear, thanks! Done https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:243: password_manager::TestPasswordStore* password_store = On 2015/08/05 08:37:35, vabr (Chromium) wrote: > (1) You do not need TestPasswordStore interface, just use PasswordStore. > (2) Please keep the store in a scoped_refptr: > scoped_refptr<PasswordStore> password_store = > PasswordStoreFactory::GetForProfile(...); > > (Also below.) Done. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_test_base.cc (right): https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.cc:71: form, password_manager::PasswordStore::ALLOW_PROMPT, this); On 2015/08/05 08:37:35, vabr (Chromium) wrote: > Please use DISALLOW_PROMPT instead, to mitigate the risk of getting stuck > because of displaying a user-facing prompt to unlock the keychain. Done. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.h:12: #include "components/password_manager/core/browser/test_password_store.h" On 2015/08/05 08:37:35, vabr (Chromium) wrote: > You do not need this. Done. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.h:49: class PasswordStoreObserver : public password_manager::PasswordStoreConsumer { On 2015/08/05 08:37:35, vabr (Chromium) wrote: > I suggest against naming the class *Observer. It confusingly hints at > PasswordStore::Observer, which it does not use. It also does not really observe > the store (it does not see anything else than the response to its own GetLogins > request). > > My suggestion: DummyStoreConsumer > because it expresses more clearly what it inherits from, what it does, and that > it does not care about the actual response from the store. Done. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.h:51: explicit PasswordStoreObserver(password_manager::TestPasswordStore*); On 2015/08/05 08:37:35, vabr (Chromium) wrote: > Please change TestPasswordStore references to PasswordStore in all of the class > code. You are not using anything not contained in the base PasswordStore > interface. Done. https://codereview.chromium.org/1271053002/diff/80001/chrome/browser/password... chrome/browser/password_manager/password_manager_test_base.h:59: void Wait(); On 2015/08/05 08:37:35, vabr (Chromium) wrote: > The name is too short and vague. Also, it seems strange to pass the passwords > store to the constructor and only use it in Wait(). I would suggest: > (1) Get rid of test_password_store_ member variable, do not pass a store in the > constructor. > (2) Instead, pass the store as an argument of the current Wait() method, and do > not store that argument anywhere. > (3) Rename Wait to RequestLoginsAndWait. Done.
LGTM, thanks for addressing all my comments! Vaclav
The CQ bit was checked by xunlu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1271053002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1271053002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_10.10_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_10.1...)
On 2015/08/06 08:12:11, vabr (Chromium) wrote: > LGTM, thanks for addressing all my comments! > Vaclav Hey Vaclav, When I was testing the fix on my local machine today, I found that the test would still flake( like 1 in 1000 times). Therefore, this fix will not completely fix the issue but only reduce the probability of it happening. So do you think we should enable these tests?
On 2015/08/06 23:55:24, xunlu wrote: > On 2015/08/06 08:12:11, vabr (Chromium) wrote: > > LGTM, thanks for addressing all my comments! > > Vaclav > > Hey Vaclav, > > When I was testing the fix on my local machine today, I found that the test > would still flake( like 1 in 1000 times). Therefore, this fix will not > completely fix the issue but only reduce the probability of it happening. > > So do you think we should enable these tests? Thanks, Xunlu, for the detailed testing. I added one more comment to potentially help with the flakes. If it does not help, we should still land your CL, as it improves the situation. If you want, you can leave the tests enabled, but occasionally observe the trybots and waterfall (http://build.chromium.org/p/chromium/console?reload=60) for the next couple of days. If you see the tests flake again, then disable them (without removing your other changes). Cheers, Vaclav
https://codereview.chromium.org/1271053002/diff/140001/chrome/browser/passwor... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/1271053002/diff/140001/chrome/browser/passwor... chrome/browser/password_manager/password_manager_browsertest.cc:260: Would inserting base::RunLoop run_loop; run_loop.RunUntilIdle(); help with the flakes you observe locally?
On 2015/08/07 07:47:59, vabr (Chromium) wrote: > https://codereview.chromium.org/1271053002/diff/140001/chrome/browser/passwor... > File chrome/browser/password_manager/password_manager_browsertest.cc (right): > > https://codereview.chromium.org/1271053002/diff/140001/chrome/browser/passwor... > chrome/browser/password_manager/password_manager_browsertest.cc:260: > Would inserting > base::RunLoop run_loop; > run_loop.RunUntilIdle(); > help with the flakes you observe locally? This didn't seem to help. Still seeing 1 or 2 failures in 1000 runs.
On 2015/08/14 22:40:30, xunlu wrote: > On 2015/08/07 07:47:59, vabr (Chromium) wrote: > > > https://codereview.chromium.org/1271053002/diff/140001/chrome/browser/passwor... > > File chrome/browser/password_manager/password_manager_browsertest.cc (right): > > > > > https://codereview.chromium.org/1271053002/diff/140001/chrome/browser/passwor... > > chrome/browser/password_manager/password_manager_browsertest.cc:260: > > Would inserting > > base::RunLoop run_loop; > > run_loop.RunUntilIdle(); > > help with the flakes you observe locally? > > This didn't seem to help. Still seeing 1 or 2 failures in 1000 runs. Thanks for checking, Xun. In that case you can either try to land and be prepared to disable the tests if they flake again (as recommended in https://codereview.chromium.org/1271053002/#msg17), or you can try to trace down the issue further, by seeing whether in the failing cases the message AutofillHostMsg_PasswordFormsParsed (sent for the dynamically created form) is received by the browser code after DummyStoreConsumer::RequestLoginsAndWait is called. If that's the case, there should be a way to wait with using the dummy store consumer after the message is received, and that should fix the failures. If it is not the case, then I'm not sure what happens. Cheers, Vaclav
On 2015/08/17 08:17:52, vabr (Chromium) wrote: > On 2015/08/14 22:40:30, xunlu wrote: > > On 2015/08/07 07:47:59, vabr (Chromium) wrote: > > > > > > https://codereview.chromium.org/1271053002/diff/140001/chrome/browser/passwor... > > > File chrome/browser/password_manager/password_manager_browsertest.cc > (right): > > > > > > > > > https://codereview.chromium.org/1271053002/diff/140001/chrome/browser/passwor... > > > chrome/browser/password_manager/password_manager_browsertest.cc:260: > > > Would inserting > > > base::RunLoop run_loop; > > > run_loop.RunUntilIdle(); > > > help with the flakes you observe locally? > > > > This didn't seem to help. Still seeing 1 or 2 failures in 1000 runs. > > Thanks for checking, Xun. > > In that case you can either try to land and be prepared to disable the tests if > they flake again (as recommended in > https://codereview.chromium.org/1271053002/#msg17), or you can try to trace down > the issue further, by seeing whether in the failing cases the message > AutofillHostMsg_PasswordFormsParsed (sent for the dynamically created form) is > received by the browser code after DummyStoreConsumer::RequestLoginsAndWait is > called. If that's the case, there should be a way to wait with using the dummy > store consumer after the message is received, and that should fix the failures. > If it is not the case, then I'm not sure what happens. > > Cheers, > Vaclav Hi Xun, Unless you plan to work on this, would you mind closing the CL and noting the CL URL in the bug, so that we know which ideas have been tried out the next time we have a look at that issue? Cheers, Vaclav
Description was changed from ========== [PasswordManager] Fix Flaky BrowserTest related to dynamically created password form For browserTests that dynamically create password form, add PasswordStoreObserve to wait for response from PasswordStore before submitting the form. BUG=301547 ========== to ========== [PasswordManager] Fix Flaky BrowserTest related to dynamically created password form For browserTests that dynamically create password form, add PasswordStoreObserve to wait for response from PasswordStore before submitting the form. BUG=301547 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
