To add more details for people interested in implementing:

components/autofill/core/browser/password_generator.cc has hard-coded calls to base::RandGenerator and base::RandomShuffle. This means that unit tests are also influenced by randomness.

The proposed solution is to:
(1) Allow alternative versions of base::RandGenerator and base::RandomShuffle to be injected in the production code.
(2) Use deterministic alternatives in the unit test to ensure that the "random" decisions are controlled.

To implement (1), one can, e.g., have a global static function pointers (null by default), settable by a function with a name ending with *ForTest(). Once set, the production code would call the functions pointed to, otherwise default to base::RandGenerator and base::RandomShuffle.

In the tests, the deterministic alternative to RandGenerator could be used to ensure that, e.g., in PasswordGeneratorTest.AllCharactersAreGenerated all characters from a given character set are enumerated, without the need to do too many iterations.

Furthermore, injecting the deterministic alternative to RandomShuffle allows to test that RandomShuffle is indeed used (not tested now).

Not all tests depend on the outcome of the random functions. But unless too heavy to set up, the deterministic alternatives should be injected for all tests, to avoid missing a test where they are indeed needed. The best way to do that would be to migrate the unit tests to have a testing::Test-based fixture, which would inject the deterministic versions during construction.

Related discussion on the original CL: https://chromium-review.googlesource.com/c/chromium/src/+/1073587#message-fc2e52ae937a29bfba0898c0d30b7f43eb6ca152

I am happy to review work on this.

I wanna try fixing this !

Sounds good, feel free to send me CLs for review.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48d2aa2c992885fa553c8046e9c3e6f02dcf848b

commit 48d2aa2c992885fa553c8046e9c3e6f02dcf848b
Author: Jerry Lin <wahahab11@gmail.com>
Date: Wed Jun 20 10:25:43 2018

Make unittests of password_generator to be deterministic

The tests in components/autofill/c/b/password_generator_fips181_unittest.cc
use a random source during tests and are thus non-deterministic.

This patch adds a method of PasswordGeneratorFips181 called
SetGeneratorForTest. It accepts a parameter generate_func so
PasswordGeneratorFips181 will use this function to generate password to
achieve deterministic unittesting. Unittest code uses this function to
set generator for test to make test deterministic.

R=vabr@chromium.org

Bug: 847200
Change-Id: If2924ed0e59843336f0e642053eefaca4c353163
Reviewed-on: https://chromium-review.googlesource.com/1084392
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568782}
[modify] https://crrev.com/48d2aa2c992885fa553c8046e9c3e6f02dcf848b/AUTHORS
[modify] https://crrev.com/48d2aa2c992885fa553c8046e9c3e6f02dcf848b/components/autofill/core/browser/password_generator_fips181.cc
[modify] https://crrev.com/48d2aa2c992885fa553c8046e9c3e6f02dcf848b/components/autofill/core/browser/password_generator_fips181.h
[modify] https://crrev.com/48d2aa2c992885fa553c8046e9c3e6f02dcf848b/components/autofill/core/browser/password_generator_fips181_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a44cff8d5aa3a3823d53e046a8046bd6a702e5f9

commit a44cff8d5aa3a3823d53e046a8046bd6a702e5f9
Author: Jerry Lin <wahahab11@gmail.com>
Date: Mon Jun 25 08:56:28 2018

Add fuzzer for PasswordGeneratorFips181.

This patch adds fuzzer for password_generator_fips181_unittest.cc.

R=vabr@chromium.org
Bug: 847200

Change-Id: I83823e2315c459963ff020604272613482f15e78
Reviewed-on: https://chromium-review.googlesource.com/1109596
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569986}
[modify] https://crrev.com/a44cff8d5aa3a3823d53e046a8046bd6a702e5f9/components/autofill/core/browser/BUILD.gn
[add] https://crrev.com/a44cff8d5aa3a3823d53e046a8046bd6a702e5f9/components/autofill/core/browser/password_generator_fips181_fuzzer.cc

Hi Jerry,
Thanks for improving the password_generator_fips181_unittest.cc!

Are you interested in doing the same for password_generator_unittest.cc, or is that available for others?

Thanks for reminding that!

I am very happy to do this.
I will start working on this and send CLs to you when I finish.

Following up on the discussion chromium-dev [1], I'd suggest to following plan, with focus on fuzzing the code:

(1) Write a fuzzer for autofill::GeneratePassword. There is direct support for getting protobuf messages from the fuzzer library [2] and examples to look at (e.g., [3]). This is related to bug 834707 and CLs for this step should reference both 834707 and 847200

The advantage of (1) is that it does not require any modifications to production code.

We can optionally also consider doing the following. However, the trade-off there is slightly worse. The cons include modifying and increasing complexity of the production code:

(2) Introduce a way to inject base::RandGenerator into password_generator.cc, use it to supply fuzzed values instead, and (in a separate CL) also to make the unit tests deterministic and increase their coverage [4].

(3) Similar to (2) but for base::RandomShuffle.



[1] https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-dev/GiN7RhBByWs
[2] https://chromium.googlesource.com/chromium/src/testing/libfuzzer/+/HEAD/libprotobuf-mutator.md
[3] https://chromium.googlesource.com/chromium/src/+/d12baebacc3cbabb5b1fccaccddc37df2ba7acc6/components/password_manager/core/browser/form_parsing/fuzzer/form_parser_proto_generic_fuzzer.cc
[4] https://chromium.googlesource.com/chromium/src/+/197ddf9e9b73e9e080f477352d9b17dfca1df379/components/autofill/core/browser/password_generator.cc#190

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d619893102639fa0f43447c149884063c529e32

commit 0d619893102639fa0f43447c149884063c529e32
Author: Jerry Lin <wahahab11@gmail.com>
Date: Fri Aug 17 14:32:27 2018

Add fuzzer for PasswordGenerator.

This fuzzer passes fuzzed message |PasswordRequirementsSpec| to function |GeneratePassword|.
The random choices inside the function are still random and not replaced by the fuzzed input.

R=vabr@chromium.org

Bug: 847200
Change-Id: I221fe286328d5818bd50282046799e8930079298
Reviewed-on: https://chromium-review.googlesource.com/1120088
Reviewed-by: Jonathan Metzman <metzman@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584059}
[modify] https://crrev.com/0d619893102639fa0f43447c149884063c529e32/components/autofill/core/browser/BUILD.gn
[add] https://crrev.com/0d619893102639fa0f43447c149884063c529e32/components/autofill/core/browser/password_generator_proto_fuzzer.cc
[modify] https://crrev.com/0d619893102639fa0f43447c149884063c529e32/components/password_manager/core/browser/BUILD.gn

vabr going hobby only -> reducing involvement.
Please contact me directly in urgent matters.