Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
999073002:
Integrate serving affiliation-based matches into the PasswordStore. (Closed)
DescriptionIntegrate serving affiliation-based matches into the PasswordStore.
BUG=437865
Committed: https://crrev.com/a6bf1b23de42b96dfcc4b0b6d62d3696bb51b31a
Cr-Commit-Position: refs/heads/master@{#320959}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments from mkwst@. #
Total comments: 10
Patch Set 3 : Addressed comments from vabr@. #Patch Set 4 : Rebase. #Patch Set 5 : Add missing include. #Patch Set 6 : Add more useful debug output to see what is wrong on Mac. #Patch Set 7 : Fix operator== and operator<< for PasswordForm, update tests. #Patch Set 8 : Fix failures on Mac. #Patch Set 9 : Reduce scope of CL, test utils are fixed in a separate CL. #Messages
Total messages: 36 (13 generated)
engedy@chromium.org changed reviewers: + vabr@chromium.org
Hi Vaclav, could you please take a look at this?
engedy@chromium.org changed reviewers: + mkwst@chromium.org - vabr@chromium.org
Reassigning to Mike, who volunteered to take a look at this. Thanks. :) @Mike, as always, please start with the tests. The rest of the code should be conveniently boring here too.
LGTM % comments. https://codereview.chromium.org/999073002/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliated_match_helper.cc (right): https://codereview.chromium.org/999073002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliated_match_helper.cc:93: password_store_->AddObserver(this); Can you ASSERT(password_store_) here, since you're no longer doing it in the constructor? https://codereview.chromium.org/999073002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/999073002/diff/1/components/password_manager/... components/password_manager/core/browser/password_store.cc:297: more_results.weak_clear(); Why weak_clear? Doesn't `insert` transfer ownership to `results` in a way that would let you dump `more_results` entirely here? https://codereview.chromium.org/999073002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/999073002/diff/1/components/password_manager/... components/password_manager/core/browser/password_store_unittest.cc:358: // but not the credential for the unaffiliated Android application. Nit: I'd suggest splitting this out into a separate test rather than having two scenarios in a single test.
https://codereview.chromium.org/999073002/diff/1/components/password_manager/... File components/password_manager/core/browser/affiliated_match_helper.cc (right): https://codereview.chromium.org/999073002/diff/1/components/password_manager/... components/password_manager/core/browser/affiliated_match_helper.cc:93: password_store_->AddObserver(this); On 2015/03/12 09:23:09, Mike West (OOO) wrote: > Can you ASSERT(password_store_) here, since you're no longer doing it in the > constructor? I have added the 2 checks to Initialize(). https://codereview.chromium.org/999073002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/999073002/diff/1/components/password_manager/... components/password_manager/core/browser/password_store.cc:297: more_results.weak_clear(); On 2015/03/12 09:23:09, Mike West (OOO) wrote: > Why weak_clear? Doesn't `insert` transfer ownership to `results` in a way that > would let you dump `more_results` entirely here? Nope. Ownership is retained after the previous line, so this is the established standard way to do it. It is ugly, but I am told that we are looking into dumping ScopedVector soon and then we will no longer have these problems. https://codereview.chromium.org/999073002/diff/1/components/password_manager/... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/999073002/diff/1/components/password_manager/... components/password_manager/core/browser/password_store_unittest.cc:358: // but not the credential for the unaffiliated Android application. On 2015/03/12 09:23:09, Mike West (OOO) wrote: > Nit: I'd suggest splitting this out into a separate test rather than having two > scenarios in a single test. Done.
Ok. Land it.
engedy@chromium.org changed reviewers: + jochen@chromium.org
@Jochen, could you please review changes to chrome_constants.*, that is, adding a new file to the profile directory?
On 2015/03/12 at 10:03:43, engedy wrote: > @Jochen, could you please review changes to chrome_constants.*, that is, adding a new file to the profile directory? Hrm. I thought that was per-file OWNERed to *, but it apparently isn't. Is that intentional?
vabr@chromium.org changed reviewers: + vabr@chromium.org
LGTM with some minor comments. Also, a bug report to the way you switched reviewers: if you remove a reviewer, and announce that in an e-mail, please make sure that that reviewer stays at least in the Cc of that e-mail, otherwise they won't get the message until they actually navigate to the review. :) Cheers, Vaclav https://codereview.chromium.org/999073002/diff/20001/components/password_mana... File components/password_manager/core/browser/affiliated_match_helper.h (right): https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/affiliated_match_helper.h:109: PasswordStore* password_store_; Please make this PasswordStore* const password_store_; to make apparent that this is only assigned to during construction. Also please comment on this being expected to be not null, except in tests which don't initialise the helper. https://codereview.chromium.org/999073002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/password_store.cc:290: autofill::PasswordForm android_form; nit: You can drop autofill:: due to line 21. https://codereview.chromium.org/999073002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/password_store.h:205: // Note: subclasses should implement FillMatchingLogins() instead, which is So do we still have legitimate reasons to keep this virtual? Code search tells me that it is no longer overridden, except for in PasswordStoreWin. Is that a TODO to remove that last override, or something which is expected to last in the near future? In each case, a comment explaining what are actual good reasons to override this as opposed to overriding FillMatchingLogins would be helpful. https://codereview.chromium.org/999073002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/password_store_unittest.cc:57: // Expects that GetAffiliatedAndroidRealms() will be called with the nit: Mentioning the callback sounds a little confusing, because the method itself does not do anything with callbacks. What about changing "Expects...invokes" to "Make the helper expect...invoke"? https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/password_store_unittest.cc:273: "https://two.example.com/origin", Why not introduce kTestWebOrigin2, when you already have that for realms?
what's the story for deleting data from this database? will it grow for ever? what if the user wants to clear browsing data? Will this leak information about visited sites? What when the user clears the history? how will you make sure that writes finish on shutdown? how will you deal with a corrupted database? how will you cope with failed writes?
@Vaclav, please take another look. https://codereview.chromium.org/999073002/diff/20001/components/password_mana... File components/password_manager/core/browser/affiliated_match_helper.h (right): https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/affiliated_match_helper.h:109: PasswordStore* password_store_; On 2015/03/12 13:54:12, vabr (Chromium) wrote: > Please make this > PasswordStore* const password_store_; > to make apparent that this is only assigned to during construction. > Also please comment on this being expected to be not null, except in tests which > don't initialise the helper. Done. https://codereview.chromium.org/999073002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_store.cc (right): https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/password_store.cc:290: autofill::PasswordForm android_form; On 2015/03/12 13:54:12, vabr (Chromium) wrote: > nit: You can drop autofill:: due to line 21. Done. https://codereview.chromium.org/999073002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_store.h (right): https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/password_store.h:205: // Note: subclasses should implement FillMatchingLogins() instead, which is On 2015/03/12 13:54:12, vabr (Chromium) wrote: > So do we still have legitimate reasons to keep this virtual? Code search tells > me that it is no longer overridden, except for in PasswordStoreWin. Is that a > TODO to remove that last override, or something which is expected to last in the > near future? In each case, a comment explaining what are actual good reasons to > override this as opposed to overriding FillMatchingLogins would be helpful. Done. https://codereview.chromium.org/999073002/diff/20001/components/password_mana... File components/password_manager/core/browser/password_store_unittest.cc (right): https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/password_store_unittest.cc:57: // Expects that GetAffiliatedAndroidRealms() will be called with the On 2015/03/12 13:54:12, vabr (Chromium) wrote: > nit: Mentioning the callback sounds a little confusing, because the method > itself does not do anything with callbacks. What about changing > "Expects...invokes" to "Make the helper expect...invoke"? I have rephrased, please take a look. https://codereview.chromium.org/999073002/diff/20001/components/password_mana... components/password_manager/core/browser/password_store_unittest.cc:273: "https://two.example.com/origin", On 2015/03/12 13:54:12, vabr (Chromium) wrote: > Why not introduce kTestWebOrigin2, when you already have that for realms? Done.
Thanks, your response to my comments LGTM. Once you put together answers to Jochen's questions, could you consider making that a design doc, and link it also from the bug? Cheers, Vaclav
i'd be less nervous about this if the new feature was behind a flag, or is it?
Sorry for the delay. > i'd be less nervous about this if the new feature was behind a flag, or is it? Yes, it is behind a flag. If not explicitly enabled, no new code is exercised (other than checking whether or not the feature is enabled) and no new files are created. > what's the story for deleting data from this database? will it grow for ever? > what if the user wants to clear browsing data? Will this leak information about > visited sites? What when the user clears the history? No information about visited web sites are stored, therefore clearing history is N/A. Instead, the database can be thought of as a cache that stores additional meta-information corresponding to some credentials stored in the password manager. More precisely, it caches previously fetched affiliation information, keyed by applications, currently for Android applications for which the user has credentials stored. For any given application, the affiliation data itself is public information and the same for all users. In terms of size, it is on the order of 1-2 kB per application. New data is therefore only added to the cache if credentials are stored for new applications. Existing data in the cache becomes stale after some time, after which it gets refetched, in which case the old data is overwritten. The details of deleting no longer relevant data is TBD, but will happen either when corresponding credentials are deleted and/or when the user chooses to clear 'Cache' as part of 'Clear browsing data'. (This will obviously need to be decided and implemented before dropping the flag.) All in all, the database contains only derived data, and can be regenerated at any time at cost of some fetches being made to a web service. > how will you make sure that writes finish on shutdown? There are no writes explicitly triggered on shutdown. The SQLite DB gets sqlite_close()'d when the DB thread message loop gets pumped on a normal shutdown. We are doing the same as for the rest of SQLite databases, therefore I *assume*, but have not verified, that this does the right thing. During the "fast" shutdown flows, this might not happen, so we might lose data with if it was fetched shortly before shutdown. But once again, this is a cache that stores only derived data, so we do not particularly care about losing some. Insofar as corrupting the DB is concerned, I am told (https://codereview.chromium.org/797983003/#msg17) that barring a power loss, it is quite difficult to corrupt an SQLite DB (but see next section). > how will you deal with a corrupted database? On encountering, after start-up, SQL errors defined as catastrophic (SQLITE_CORRUPT, SQLITE_NOTADB, meaning that recovery from these states is highly unlikely), the DB will be razed, closed, and "poisoned" until the next shut-down/start-up cycle, meaning that all subsequent operations will silently fail. If such an error is still present/encountered on start-up, the DB will be razed and the empty DB tried to be opened again. If that again fails, the DB again gets into a poisoned state. Note that the two errors above represent only a fraction of all errors, and do not include errors like SQLITE_IOERR_WRITE, SQLITE_FULL, or SQLITE_READONLY. In these cases, operations will just silently fail (the DB will not get razed), expecting that things might get better with time, so the layer above the database will need to tolerate this (see next section). Recovery is not necessary as the DB is only a cache that contains derived data, and can be regenerated at any time. > how will you cope with failed writes? The layer above the database -- namely, the AffiliationBackend -- will be fault tolerant in the sense that it will expect that any read/write can fail. As a result of such failure, it will operate with reduced functionality, or not at all; but fail in a safe manner (i.e. report that nothing is affiliated with anything, and will not do anything terrible otherwise). I am planning to add unit tests that explicitly test these scenarios. (Currently, I *think* that the implementation can tolerate this already, but this is not yet tested, which will be needed before removing the flag). One concern that came up was that the AffiliationBackend can be requested to preload data into this cache DB and keep it fresh by periodic refetches. In case the DB is not writable, the AffiliationBackend might be fooled into thinking, after each otherwise successful fetch, that the data is still not fresh and needs fetching. This could lead to an infinite flood of requests. To prevent this, the last fetch time is stored in memory, and no refetches will be made if one has taken place recently, regardless of what is in the database. There is also a time stamp stored next to each piece of cached data in the database, so in the case the database is rendered read-only at some point (due to write errors), the cached data will be correctly recognized as stale and ignored after some time.
The last paragraph should read: "There is also a stamp stored next to each piece of cached data in the database, *indicating when that piece of data was fetched*..." So in other words, the last fetch time is stored redundantly: both in the database, and also in memory.
well then.. lgtm
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, vabr@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/999073002/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/999073002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, jochen@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/999073002/#ps80001 (title: "Add missing include.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/999073002/80001
Patchset #6 (id:100001) has been deleted
Patchset #9 (id:170001) has been deleted
@Vaclav, could you PTAL at the delta between Patch Set 4 and 9? Only the unittest is changed, I have factored out fixing the test utils into https://codereview.chromium.org/1018613002/, which must land before this, otherwise the tests are not really verifying too much.
Sounds reasonable, LGTM.
The CQ bit was checked by engedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/999073002/#ps190001 (title: "Reduce scope of CL, test utils are fixed in a separate CL.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/999073002/190001
Message was sent while issue was closed.
Committed patchset #9 (id:190001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a6bf1b23de42b96dfcc4b0b6d62d3696bb51b31a Cr-Commit-Position: refs/heads/master@{#320959} |
