Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24734007:
Encrypt all stored cookies on selected operating systems. (Closed)
|
Created:
7 years, 2 months ago by bcwhite Modified:
7 years ago Reviewers:
Jói, erikwright (departed), bcwhite1, jam, benm (inactive), rpetterson, Scott Hess - ex-Googler CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEncrypt all stored cookies on selected operating systems.
As part of the goal of protecting private user information, this encrypts the cookie values on operating systems with user-specific crypto APIs and that do not otherwise protect this data.
Performance tests indicate a penalty of about 1ms per cookie (regardless of size) on a Mac and 0.1ms to 0.7ms (depending on the size) under Windows. This will be higher on older hardware but still insignificant.
Encrypted data is binary (with an overhead of 128 bytes on Windows) and binary data must be stored in a BLOB so only one of two fields ("value" or "encrypted_value") will have data with the other being empty. Both values, however, need to be read & written when accessing a cookie because they are marked "non null").
BUG=313323
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234855
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240684
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241004
Patch Set 1 #
Total comments: 9
Patch Set 2 : addressed review comments from shess #Patch Set 3 : create a 'delegate' for cookie crypto and have it set by instantiating method #Patch Set 4 : removed unnecessary build changes #Patch Set 5 : fixed compile on other arch; added test and fixed problems found #Patch Set 6 : fixed perftest #
Total comments: 35
Patch Set 7 : addressed (most) review comments by Erik #
Total comments: 24
Patch Set 8 : addressed review comments by Scott #Patch Set 9 : added new file left out from previous commit #Patch Set 10 : simplify #ifdef for cookie-crypto #Patch Set 11 : use scoped_ptr instead of raw pointers when permanently passing in objects #Patch Set 12 : fixed Android build (missing pointer conversion) #Patch Set 13 : updated new DB schema to be consistent with after-upgrade version #
Total comments: 2
Patch Set 14 : look at actual SQL record to verify no plaintext #
Total comments: 1
Patch Set 15 : check all fields for absent plaintext #Patch Set 16 : scan raw DB file for values to ensure encyrption #
Total comments: 13
Patch Set 17 : add some debug statements to try to find why it's breaking on the try server #Patch Set 18 : addressed review comments by erikwright #Patch Set 19 : need real declaration to instantiate scoped_ptr of an object #Patch Set 20 : need real declaration to instantiate scoped_ptr of an object (android edition) #Patch Set 21 : last upload failed; try again #
Total comments: 1
Patch Set 22 : need real declaration to instantiate scoped_ptr of an object (perftest edition) #Patch Set 23 : add trace of test failing on try-server to see what's going on #Patch Set 24 : more printf debugging #Patch Set 25 : more printf debugging #Patch Set 26 : more printf debugging #Patch Set 27 : fixed add-cookie race condition and removed all extra debug logging #Patch Set 28 : fixed typo #Patch Set 29 : rebased #Patch Set 30 : rebased #Patch Set 31 : disable on Mac because it breaks tests waiting for Keychain (will fix in another CL) #Patch Set 32 : rebased #Patch Set 33 : fix android build #Patch Set 34 : removed left-over code from bad rebase #Patch Set 35 : fix memory leak in test #Messages
Total messages: 110 (0 generated)
Erik, you're the only owner for this code. I'd appreciate your review of the general changes to the cookie store. Encrypting cookies was agreed upon with Justin Schuh as part of the goal of protecting user data. It can't be submitted as-is because of a circular dependency between //components and //content it introduces. I'm working to move the webdata/Encryptor class lower down.
Scott <shess>, I'm told you're the reigning expert on SQLite. Can you comment on any performance issues regarding having two "value" fields, one text and one blob, of which only one has data at any given time?
To put this in context, Brian is adding a BLOB column that will contain the encrypted value of a cookie. For new cookies, the BLOB will be used. For old cookies it will be empty and the existing text column will be used. The alternative would be to go through an awkward process of converting all the existing cookies. Supposedly we can't drop columns so it would entail creating a new table, copying data over, then dropping/renaming. We will have to look at both columns (BLOB and TEXT) while loading from disk to know which one to use. This may be 0 impact if it's inevitable that both are paged in anyway, or maybe nonzero if they are possibly stored in different places on disk. And we must of course assume that the empty TEXT or BLOB still adds some trivial amount of storage. On Fri, Sep 27, 2013 at 2:50 PM, <bcwhite@chromium.org> wrote: > Scott <shess>, I'm told you're the reigning expert on SQLite. Can you > comment > on any performance issues regarding having two "value" fields, one text > and one > blob, of which only one has data at any given time? > > https://codereview.chromium.**org/24734007/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm going OOT tomorrow for a week, I expect to be online somewhat, but probably not super dependable. Anyhow, performance-wise a zero-length string or blob costs 1 byte per row. SQLite stores rows contiguously, with long rows spilling to overflow pages. So the encryption overhead will definitely increase the database size, but having two columns won't. A random organic Cookies file I have on hand has avg(length(value)) as 85 bytes, the rest of the row maybe adds up to another 25-40 bytes, so maybe databases get 100% bigger on disk? https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:370: "encrypted_value BLOB NOT NULL," Put the encrypted_value at the end so that the create-from-scratch and version-migrated schema match. https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:976: std::string value, encrypted_value; // Only one will ever get a value below. I would put this inside the loop. Since the code always does exactly one or the other, the value cannot leak between loop iterations, but that feels sketchy to me. The encryption itself is going to add way way more overhead anyhow. That said, I think it would be even more obvious if you simply put the #if defined around the column 3 and 4 bindings.
>So the encryption overhead will definitely increase the >database size, but having two columns won't. Only 0.01% of our users have a cookie database exceeding 4MB with less than 10% exceeding 1MB. >A random organic Cookies file I have on hand has >avg(length(value)) as 85 bytes, the rest of the row maybe >adds up to another 25-40 bytes, so maybe databases get >100% bigger on disk? I'm not sure if Mac/Linux grow the data size during OS encryption but Windows adds 128 bytes. https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:370: "encrypted_value BLOB NOT NULL," On 2013/09/27 23:07:12, shess wrote: > Put the encrypted_value at the end so that the create-from-scratch and > version-migrated schema match. Done. https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:976: std::string value, encrypted_value; // Only one will ever get a value below. > I would put this inside the loop. Since the code always does exactly one or the > other, the value cannot leak between loop iterations, but that feels sketchy to > me. The encryption itself is going to add way way more overhead anyhow. Sure. I was trying to avoid re-constructing the objects each time but it's hardly an issue. > That said, I think it would be even more obvious if you simply put the #if > defined around the column 3 and 4 bindings. Okay. I'll have to create a sub-block to create the string there (because it's inside a "case" and thus not necessarily initialized). I thought it was easier to read when put at the top.
> Only 0.01% of our users have a cookie database exceeding 4MB with less than 10% > exceeding 1MB. Minor correction. About 1% exceed 1MB.
addressed review comments from shess
create a 'delegate' for cookie crypto and have it set by instantiating method
removed unnecessary build changes
Erik, I've never created a Delegate before. Is this done correctly?
fixed compile on other arch; added test and fixed problems found
fixed perftest
https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:710: std::string value = smt.ColumnString(3); I suppose it's possible that there's a hit for reading even an empty field. Consider only calling ColumnString(3) if the encrypted value is empty. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:305: const int kCompatibleVersionNumber = 5; This needs to be 7. Some of the cookies will have empty value fields, which means it's not backwards compatible. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:714: std::string value = smt.ColumnString(3); How about accessing smt.ColumnString(3) only if encrypted_value is not empty? https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:716: if (!encrypted_value.empty() && crypto_.get()) I think it's a serious error condition if there is an encrypted value and crypto_ is NULL. Should probably DCHECK. In production, the least harmful thing to do would be to read it into memory (with a blank value) but never send it, but I think that reading it into memory (with a blank value) and sending it is much easier and not that much worse. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:861: "ALTER TABLE cookies ADD COLUMN encrypted_value BLOB DEFAULT ''")); IIUC the migration path makes this nullable with default '' while the new path makes it not null? Perhaps after creating the column and giving all rows a value we could alter it to make it not nullable? https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:996: add_smt.BindBlob(4, encrypted_value.data(), This looks like it's refering to memory that will be subsequently freed when encrypted_value goes off the stack. If that's not the case (because BindBlob copies the value) please add a comment to that effect. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:1000: add_smt.BindBlob(4, "", 0); // encrypted_value Presumably NULL could be used in place of '\0' since it is a 0-length blob? https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.h (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.h:54: CookieCryptoDelegate* crypto_delegate); make this a scoped_ptr. That is the modern way to document transfer of ownership. https://codereview.chromium.org/24734007/diff/37001/content/public/browser/co... File content/public/browser/cookie_store_factory.h (right): https://codereview.chromium.org/24734007/diff/37001/content/public/browser/co... content/public/browser/cookie_store_factory.h:21: // This class provides an interface that the persistent cookies store can use Move to content/public/browser/cookie_crypto_delegate.h https://codereview.chromium.org/24734007/diff/37001/content/public/browser/co... content/public/browser/cookie_store_factory.h:41: CookieCryptoDelegate* crypto_delegate); scoped_ptr
https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:710: std::string value = smt.ColumnString(3); On 2013/10/07 20:39:25, erikwright wrote: > I suppose it's possible that there's a hit for reading even an empty field. > Consider only calling ColumnString(3) if the encrypted value is empty. Done. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:305: const int kCompatibleVersionNumber = 5; On 2013/10/07 20:39:25, erikwright wrote: > This needs to be 7. Some of the cookies will have empty value fields, which > means it's not backwards compatible. If an encrypted value was written, the cookie "value" will just be empty but in general the system will continue to work even on a down-grade. Also, cookies that haven't updated to encrypted versions would still have valid values. The system will work at least as well as razing the DB and starting over; I'd call that "backward compatible". https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:716: if (!encrypted_value.empty() && crypto_.get()) On 2013/10/07 20:39:25, erikwright wrote: > I think it's a serious error condition if there is an encrypted value and > crypto_ is NULL. > > Should probably DCHECK. In production, the least harmful thing to do would be to > read it into memory (with a blank value) but never send it, but I think that > reading it into memory (with a blank value) and sending it is much easier and > not that much worse. Done. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:861: "ALTER TABLE cookies ADD COLUMN encrypted_value BLOB DEFAULT ''")); > IIUC the migration path makes this nullable with default '' while the new path > makes it not null? Perhaps after creating the column and giving all rows a value > we could alter it to make it not nullable? The other upgrade blocks don't include "not null" when specifying the now field. I assume this is because all the new entries would have to be NULL. And like the other upgrades, there's never any explicit update to all rows to assign a value. We'd never know when that condition was reached. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:996: add_smt.BindBlob(4, encrypted_value.data(), Excellent catch! I missed that when I moved the variable inside the "for" loop at Scott's suggestion. Done. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:1000: add_smt.BindBlob(4, "", 0); // encrypted_value > Presumably NULL could be used in place of '\0' since it is a 0-length blob? Doing this will try to set the value of the field to NULL but the create statement explicitly says "not null". A "check failure" results. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.h (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.h:54: CookieCryptoDelegate* crypto_delegate); > make this a scoped_ptr. That is the modern way to document transfer of > ownership. Could you provide an example? A search for "takes\ ownership" shows everything accepting a raw pointer. Thanks. https://codereview.chromium.org/24734007/diff/37001/content/public/browser/co... File content/public/browser/cookie_store_factory.h (right): https://codereview.chromium.org/24734007/diff/37001/content/public/browser/co... content/public/browser/cookie_store_factory.h:21: // This class provides an interface that the persistent cookies store can use On 2013/10/07 20:39:25, erikwright wrote: > Move to content/public/browser/cookie_crypto_delegate.h Done.
addressed (most) review comments by Erik
https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.h (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.h:54: CookieCryptoDelegate* crypto_delegate); On 2013/10/08 16:10:25, bcwhite wrote: > > make this a scoped_ptr. That is the modern way to document transfer of > > ownership. > > Could you provide an example? A search for "takes\ ownership" shows everything > accepting a raw pointer. > > Thanks. Precisely. With the new model, comments are no longer required. scoped_ptr<X> is an explicit indication of ownership transfer. https://code.google.com/p/chromium/codesearch#search/&q=scoped_ptr%3C%5Ba-z0-...
More SQLite comment meta-commentary. https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:710: std::string value = smt.ColumnString(3); On 2013/10/08 16:10:25, bcwhite wrote: > On 2013/10/07 20:39:25, erikwright wrote: > > I suppose it's possible that there's a hit for reading even an empty field. > > Consider only calling ColumnString(3) if the encrypted value is empty. > > Done. The big hit is I/O, and how SQLite stores things means that you'll do the I/O through the highest column accessed. So this will only cost a bit of marshaling overhead. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:305: const int kCompatibleVersionNumber = 5; On 2013/10/08 16:10:25, bcwhite wrote: > On 2013/10/07 20:39:25, erikwright wrote: > > This needs to be 7. Some of the cookies will have empty value fields, which > > means it's not backwards compatible. > > If an encrypted value was written, the cookie "value" will just be empty but in > general the system will continue to work even on a down-grade. Also, cookies > that haven't updated to encrypted versions would still have valid values. The > system will work at least as well as razing the DB and starting over; I'd call > that "backward compatible". If the database compatible version is too new, I believe we just leave it alone and go with a clean database which is not persisted. The user will eventually catch up and things will start working again. Not a great outcome, but not horrible. Also consider whether older code will write correct data for newer code. For instance, as presently defined, older code will fail on inserts because it won't be providing an encrypted_value column. As noted below, I think your CREATE actually needs to change, so that won't be a problem. OK, so to successfully work you need to have: - older code statements can successfully operate on newer schema. - older code can successfully interpret the data written by the newer code. - older code writes data which works with the newer code without migration. I think it is possible to have compatible version of 5, but I'm not sure how much effort I'd put into the issue. I don't think we have any data on how often the compatible-version check fails, unfortunately. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:382: "encrypted_value BLOB NOT NULL)", This will result in different schema depending on when a user installed Chrome. Make this path match the ALTER path. Also, the whitespace is embedded in the schema in some cases, so I would prefer not to remove the whitespace. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:861: "ALTER TABLE cookies ADD COLUMN encrypted_value BLOB DEFAULT ''")); On 2013/10/08 16:10:25, bcwhite wrote: > > IIUC the migration path makes this nullable with default '' while the new path > > makes it not null? Perhaps after creating the column and giving all rows a > value > > we could alter it to make it not nullable? > > The other upgrade blocks don't include "not null" when specifying the now field. > I assume this is because all the new entries would have to be NULL. And like > the other upgrades, there's never any explicit update to all rows to assign a > value. We'd never know when that condition was reached. ALTER...ADD changes the schema info, but does not modify the table data. SQLite implicitly assumes NULL for items where the query ran off the end of the present data. Then DEFAULT is interpreted on top of that. ALTER knows all of this implementation detail, you cannot use ALTER...ADD to add a column with a NOT NULL. Unfortunately, ALTER cannot change the definition of an existing column. So it's not possible to later change it to be NOT NULL. If desired you could probably deny new NULL data using a TRIGGER, with DEFAULT handling existing rows which don't contain the data. I think it might also be reasonable to just allow the NULL, and use ColumnType(i) == sql::COLUMN_TYPE_NULL to test it. Is it guaranteed that this column could never be a valid empty value? Though I guess in that case it would fallback to the other column, which would also be empty. If that scenario is valid, it might be worth having a test to prevent problems if someone refactors later. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:996: add_smt.BindBlob(4, encrypted_value.data(), On 2013/10/08 16:10:25, bcwhite wrote: > Excellent catch! I missed that when I moved the variable inside the "for" loop > at Scott's suggestion. > > Done. BindBlob() uses SQLITE_TRANSIENT, which means SQLite will make a copy of the data.
OK, I just did an end-to-end review. IMHO, Erik is more OWNER, so give more weight to my SQL-specific comments, with the others being more along the lines of drive-by commentary. https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl_io_data.cc:432: ); Definitely defer to Erik on this, but ... Would it make sense to have CookieOSCryptoDelegate::Create(), and have it handle returning NULL if the OS doesn't support it? https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:717: DCHECK(encrypted_value.empty() || crypto_.get() != NULL); I can see that this condition corresponds to failures in the following condition, but I have to think hard about it. IMHO putting DCHECK(encrypted_value.empty()) in the else clause would more clearly indicate the invariant, especially if you put a symmetric DCHECK(smt.ColumnString(3).empty()) in the if() case. [I'm willing to be overridden.] https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:736: static_cast<DBCookiePriority>(smt.ColumnInt(12))))); // priority Not a review comment, just a suggestion that you might not have considered: SQL doesn't care what order things are in, so if you want to put encrypted_value at the _end_ of the SELECT statement at column 12, that would reduce the changes in here. It would also reduce the changes to the SELECT statement, too. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:866: "ALTER TABLE cookies ADD COLUMN encrypted_value BLOB DEFAULT ''")); There's really no reason to StringPrintf() this, in fact there's no reason to put it in a std::string. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:877: base::TimeTicks::Now() - start_time); I think it's interesting that we're histogramming time-to-migrate for a trivial schema-only change, but we didn't histogram time-to-migrate for the data migration from v3 to v4. I guess that's local style, but AFAICT these last few migrations should be constant cost. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:967: "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)")); My earlier comment about ordering also applies here - you can put the new column at the end of the list, and just bind it at the new location. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:999: add_smt.BindString(3, std::string()); // value BindCString(3, ""); https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:1002: // immediately makes an internal copy of the data. I think you can trim this to "BindBlob() makes a copy of the blob". The internal SQLite stuff really isn't informative. It occurs to me that statement_unittest.cc maybe should test these assumptions. There is probably code which doesn't realize it depends on them... https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:1007: add_smt.BindBlob(4, "", 0); // encrypted_value With the CREATE change, I think this should be BindNull() and let the DEFAULT handle it. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:140: void InitializeStore(bool crypt, bool restore_old_session_cookies) { VERY easy to get your bools in the wrong order for these two functions. I'll defer to Erik on this, but I wonder if having InitializeStore() vs InitializeCryptedStore() might make sense?
https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:710: std::string value = smt.ColumnString(3); So... Yes or no? Personally I prefer the previous way of always extracting value rather than have an "else" clause. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:305: const int kCompatibleVersionNumber = 5; Yes, older code writes data that is correct for newer code in that records with a "value" but not an "encrypted_value" are handled correctly. However, you're correct that if the database was created with the new version and thus has the "not null" clause for "encrypted_value" then in fact old code will not be able to write to the new database. V6 of the database has a column "priority integer default 1" when upgraded but "priority integer not null default 1" when created from scratch. In the latter case, wouldn't in be not backward compatible with V5 because of the same "not null" clause or does the "default" setting make it okay? If so, I can add a default to newly created tables with "encrypted_value" to make it work. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:382: "encrypted_value BLOB NOT NULL)", > This will result in different schema depending on when a user installed Chrome. > Make this path match the ALTER path. It never will. The existing upgrades add columns without the "not null" clause as does the one I've added. I don't think this can be avoided. > Also, the whitespace is embedded in the schema in some cases, so I would prefer > not to remove the whitespace. Done. I guess I just made it internally consistent right here without even thinking. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.h (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.h:54: CookieCryptoDelegate* crypto_delegate); I've tried this and it doesn't work well. You can't pass a pointer to a derived class or NULL, at least one of which is necessary. https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl_io_data.cc:432: ); > Would it make sense to have CookieOSCryptoDelegate::Create(), and have it handle > returning NULL if the OS doesn't support it? All OS support it. It's only some OS that it's necessary. And actually, Linux doesn't support it but we do call it in order to be consistent with how OAuth tokens are protected. But the condition can certainly be moved there, perhaps "::IfDesired()" instead of "::Create()"? https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:717: DCHECK(encrypted_value.empty() || crypto_.get() != NULL); I like it. Done. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:736: static_cast<DBCookiePriority>(smt.ColumnInt(12))))); // priority I've put "encrypted_value" next to "value" just because they're closely related but if you feel it would be clearer to have it at the end (where it is not created) and keep the other indices the same, I don't mind. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:866: "ALTER TABLE cookies ADD COLUMN encrypted_value BLOB DEFAULT ''")); True. I'd just copied it from above. Done. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:999: add_smt.BindString(3, std::string()); // value On 2013/10/08 17:35:55, shess wrote: > BindCString(3, ""); Done. https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:1002: // immediately makes an internal copy of the data. On 2013/10/08 17:35:55, shess wrote: > I think you can trim this to "BindBlob() makes a copy of the blob". The > internal SQLite stuff really isn't informative. > > It occurs to me that statement_unittest.cc maybe should test these assumptions. > There is probably code which doesn't realize it depends on them... Done.
addressed review comments by Scott
added new file left out from previous commit
https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:305: const int kCompatibleVersionNumber = 5; On 2013/10/08 18:23:36, bcwhite wrote: > Yes, older code writes data that is correct for newer code in that records with > a "value" but not an "encrypted_value" are handled correctly. > > However, you're correct that if the database was created with the new version > and thus has the "not null" clause for "encrypted_value" then in fact old code > will not be able to write to the new database. > > V6 of the database has a column "priority integer default 1" when upgraded but > "priority integer not null default 1" when created from scratch. In the latter > case, wouldn't in be not backward compatible with V5 because of the same "not > null" clause or does the "default" setting make it okay? If so, I can add a > default to newly created tables with "encrypted_value" to make it work. Either way, I'm not sure that it is a good idea to have older code reading the new cookies with (apparently) empty values. This could produce arbitrary, unexpected results from web apps that will suddenly receive cookies that they set but without values. Having the users end up with an in-memory, not persistent cookie store is not great either, mind you. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.h (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.h:54: CookieCryptoDelegate* crypto_delegate); On 2013/10/08 18:23:36, bcwhite wrote: > I've tried this and it doesn't work well. You can't pass a pointer to a derived > class or NULL, at least one of which is necessary. Yes you can, and we do in much code: scoped_ptr<X>() for a NULL pointer. class Y : public X {}; void foobar(scoped_ptr<X> x); void client_code() { scoped_ptr<Y> y(new Y) foobar(y.PassAs<X>()); } https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl_io_data.cc:432: ); On 2013/10/08 18:23:36, bcwhite wrote: > > Would it make sense to have CookieOSCryptoDelegate::Create(), and have it > handle > > returning NULL if the OS doesn't support it? > > All OS support it. It's only some OS that it's necessary. And actually, Linux > doesn't support it but we do call it in order to be consistent with how OAuth > tokens are protected. > > But the condition can certainly be moved there, perhaps "::IfDesired()" instead > of "::Create()"? It makes me a bit ill, but maybe "MaybeCreate()"? https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:877: base::TimeTicks::Now() - start_time); On 2013/10/08 17:35:55, shess wrote: > I think it's interesting that we're histogramming time-to-migrate for a trivial > schema-only change, but we didn't histogram time-to-migrate for the data > migration from v3 to v4. I guess that's local style, but AFAICT these last few > migrations should be constant cost. I'll take a look at how useful this actually is. I'm not sure when/why we started doing it. Probably a younger, greener me asked for it.
https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/1/content/browser/net/sqlite_pe... content/browser/net/sqlite_persistent_cookie_store.cc:710: std::string value = smt.ColumnString(3); On 2013/10/08 18:23:36, bcwhite wrote: > So... Yes or no? Personally I prefer the previous way of always extracting > value rather than have an "else" clause. I don't care either way, so defer to Erik. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:382: "encrypted_value BLOB NOT NULL)", On 2013/10/08 18:23:36, bcwhite wrote: > > This will result in different schema depending on when a user installed > Chrome. > > Make this path match the ALTER path. > > It never will. The existing upgrades add columns without the "not null" clause > as does the one I've added. I don't think this can be avoided. Unfortunately, I didn't review those, or didn't catch it in review. So this case can be better. [Also, changing things now means that people who came in at v6 and migrated to v7 will match people who came in at v7. Earlier upgrades wouldn't apply.] The goal of this is that a substantive change like this has the chance of working differently depending on which upgrade path you came through. So after two or three upgrades, it get challenging to reason about the schema any longer. Better to just have them match, warts and all, then you have more confidence that everything works the same. > > Also, the whitespace is embedded in the schema in some cases, so I would > prefer > > not to remove the whitespace. > > Done. I guess I just made it internally consistent right here without even > thinking. Periodically I'll be doing a fixup which involves reformatting, and inconsistencies in spacing become terribly obvious. It's frustrating to me! https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl_io_data.cc:432: ); On 2013/10/08 18:41:53, erikwright wrote: > On 2013/10/08 18:23:36, bcwhite wrote: > > > Would it make sense to have CookieOSCryptoDelegate::Create(), and have it > > handle > > > returning NULL if the OS doesn't support it? > > > > All OS support it. It's only some OS that it's necessary. And actually, > Linux > > doesn't support it but we do call it in order to be consistent with how OAuth > > tokens are protected. > > > > But the condition can certainly be moved there, perhaps "::IfDesired()" > instead > > of "::Create()"? > > It makes me a bit ill, but maybe "MaybeCreate()"? I don't feel strongly about it. Repeated #ifdef and contorting the call to accomodate is just a bit of a red flag. Heaven forbid we ever add an additional parameter to CreatePersistentCookieStore(). Actually, since it's in the anonymous namespace, it doesn't need to be a static method, it could just be a helper function like CreateCryptoDelegateIfNeeded(). Though IMHO "IfNeeded" implies a level of choice which really isn't present here, like a given Chrome build might sometimes return one, sometimes not. It's more "IfSupported", which seems implied (if you return one, it was supported). https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:736: static_cast<DBCookiePriority>(smt.ColumnInt(12))))); // priority On 2013/10/08 18:23:36, bcwhite wrote: > I've put "encrypted_value" next to "value" just because they're closely related > but if you feel it would be clearer to have it at the end (where it is not > created) and keep the other indices the same, I don't mind. It's all synthetic all the way down, SQLite and the indices don't care what they are or whether they are related. Changing the indices is a mechanical change, if you're happy with doing it, fine, but there's nothing intrinsic to the overall change which requires it. The advantage of not-changing is that you only have to worry about the newly-added index being correct. [Queue and discard discussion of building an inherently less brittle sql::Statement API.] https://codereview.chromium.org/24734007/diff/53001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:877: base::TimeTicks::Now() - start_time); On 2013/10/08 18:41:53, erikwright wrote: > On 2013/10/08 17:35:55, shess wrote: > > I think it's interesting that we're histogramming time-to-migrate for a > trivial > > schema-only change, but we didn't histogram time-to-migrate for the data > > migration from v3 to v4. I guess that's local style, but AFAICT these last > few > > migrations should be constant cost. > > I'll take a look at how useful this actually is. I'm not sure when/why we > started doing it. Probably a younger, greener me asked for it. If it is kept, then obviously a histograms.xml change is in order. Nothing worse than a histogram which is impossible to look at.
simplify #ifdef for cookie-crypto
https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:305: const int kCompatibleVersionNumber = 5; A down-grade would mean sending empty cookie values but would be able to operate correctly with those cookies that hadn't been updated and thus encrypted. Six of one, a half-dozen of the other. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:382: "encrypted_value BLOB NOT NULL)", So... to be sure I understand what you're saying... You'd rather lose the "not null" (and add "default ''") for "encrypted_value" and be different here from all other fields in order to have the same definition of the field regardless of whether it came from a newly created table or an upgraded table. Is that correct? https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:714: std::string value = smt.ColumnString(3); On 2013/10/07 20:39:25, erikwright wrote: > How about accessing smt.ColumnString(3) only if encrypted_value is not empty? Done. https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.h (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.h:54: CookieCryptoDelegate* crypto_delegate); That means creating a scoped_ptr<Y> in the caller just to cast/pass it as a scoped_ptr<X> which in turn passes it again (to a backend) as another scoped_ptr<X> before finally being passed to the member variable. As it is, it just passes "new Crypto" as the parameter. This seems like a horrible (and inefficient) hack, using explicit casts defeating C++'s built-in polymorphism, for something that is effectively a code-comment. You're sure about this change? https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl_io_data.cc:432: ); It makes sense just to get rid of the repeated #ifdef. However, I just wanted to note that it is supported on all platforms. It's just not necessary on some -- ChromeOS, for example, encrypts the entire profile directory so also-encrypting the cookies adds no benefit.
https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.h (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.h:54: CookieCryptoDelegate* crypto_delegate); On 2013/10/08 20:18:09, bcwhite wrote: > That means creating a scoped_ptr<Y> in the caller just to cast/pass it as a > scoped_ptr<X> which in turn passes it again (to a backend) as another > scoped_ptr<X> before finally being passed to the member variable. As it is, it > just passes "new Crypto" as the parameter. > > This seems like a horrible (and inefficient) hack, using explicit casts > defeating C++'s built-in polymorphism, for something that is effectively a > code-comment. You're sure about this change? No. It will look like this: new SQLitePersistentCookieStore( ..., #ifdef ... scoped_ptr<CookieCryptoDelegate>(new CookieCryptoDelegateImpl)); #else scoped_ptr<CookieCryptoDelegate>()); #endif or, if you take the MaybeCreate route: new SQLitePersistentCookieStore( ..., CookieCryptoDelegateImpl::MaybeCreate().PassAs<CookieCryptoDelegate>()); MaybeCreate() would look like: scoped_ptr<CookieCryptoDelegateImpl> CookieCryptoDelegateImpl::MaybeCreate() { #ifdef ... return scoped_ptr<CookieCryptoDelegateImpl>(new CookieCryptoDelegateImpl); #else return scoped_ptr<CookieCryptoDelegateImpl>(); #endif } It's not effectively a code comment. It forces call-sites to explicitly give up ownership, reduces the likelihood of use-after-free, memory leak, and other types of errors, and improves readability by making ownership transfers immediately apparent.
https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.h (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.h:54: CookieCryptoDelegate* crypto_delegate); On 2013/10/08 20:24:35, erikwright wrote: > On 2013/10/08 20:18:09, bcwhite wrote: > > That means creating a scoped_ptr<Y> in the caller just to cast/pass it as a > > scoped_ptr<X> which in turn passes it again (to a backend) as another > > scoped_ptr<X> before finally being passed to the member variable. As it is, > it > > just passes "new Crypto" as the parameter. > > > > This seems like a horrible (and inefficient) hack, using explicit casts > > defeating C++'s built-in polymorphism, for something that is effectively a > > code-comment. You're sure about this change? > > No. It will look like this: > > new SQLitePersistentCookieStore( > ..., > #ifdef ... > scoped_ptr<CookieCryptoDelegate>(new CookieCryptoDelegateImpl)); > #else > scoped_ptr<CookieCryptoDelegate>()); > #endif > > or, if you take the MaybeCreate route: > > new SQLitePersistentCookieStore( > ..., > CookieCryptoDelegateImpl::MaybeCreate().PassAs<CookieCryptoDelegate>()); > > MaybeCreate() would look like: > > scoped_ptr<CookieCryptoDelegateImpl> CookieCryptoDelegateImpl::MaybeCreate() { > #ifdef ... > return scoped_ptr<CookieCryptoDelegateImpl>(new CookieCryptoDelegateImpl); > #else > return scoped_ptr<CookieCryptoDelegateImpl>(); > #endif > } > > It's not effectively a code comment. It forces call-sites to explicitly give up > ownership, reduces the likelihood of use-after-free, memory leak, and other > types of errors, and improves readability by making ownership transfers > immediately apparent. Done.
use scoped_ptr instead of raw pointers when permanently passing in objects
fixed Android build (missing pointer conversion)
https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:382: "encrypted_value BLOB NOT NULL)", On 2013/10/08 20:18:09, bcwhite wrote: > So... to be sure I understand what you're saying... > > You'd rather lose the "not null" (and add "default ''") for "encrypted_value" > and be different here from all other fields in order to have the same definition > of the field regardless of whether it came from a newly created table or an > upgraded table. > > Is that correct? Yes. I want the semantics of the field you are adding to be the same regardless of whether it is a fresh database or a migrated database. The code cannot rely on SQLite enforcing the NOT NULL, so it should not be implied that the code can rely on it. https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl_io_data.cc:432: ); On 2013/10/08 20:18:09, bcwhite wrote: > It makes sense just to get rid of the repeated #ifdef. > > However, I just wanted to note that it is supported on all platforms. It's just > not necessary on some -- ChromeOS, for example, encrypts the entire profile > directory so also-encrypting the cookies adds no benefit. Except to make the code more convoluted! I am not questioning your reasoning, but the comment about the CRYPT_COOKIES definition does not make me think "This is supported everywhere, but is unnecessary on ChromeOS". It makes me think "Some platforms support this, some do not."
updated new DB schema to be consistent with after-upgrade version
https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/37001/content/browser/net/sqlit... content/browser/net/sqlite_persistent_cookie_store.cc:382: "encrypted_value BLOB NOT NULL)", Done. I just wanted to be sure I understood your request. https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/24734007/diff/53001/chrome/browser/profiles/p... chrome/browser/profiles/profile_impl_io_data.cc:432: ); Comment improved.
lgtm https://codereview.chromium.org/24734007/diff/105001/content/browser/net/sqli... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/24734007/diff/105001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:562: EXPECT_EQ("something", cookie_other->Value()); I'm somewhat hesitant to suggest this ... but I wonder how hard it would be to grep the underlying file for one of the values? Since this would be pretty implementation-specific, it would probably want a positive test to verify the technique, either searching for a cookie name string, or searching for a cookie value in the not-encrypted case. I don't think that should block. I'm just trying to think of how to enforce that the part about encrypting is actually happening.
https://codereview.chromium.org/24734007/diff/105001/content/browser/net/sqli... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/24734007/diff/105001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:562: EXPECT_EQ("something", cookie_other->Value()); I could open the SQL DB directly and fetch the record then scan the encrypted field to make sure the plaintext version isn't in there somewhere.
look at actual SQL record to verify no plaintext
I'm not sure whether this makes me happy, either. Maybe Erik could +1 or -1 whether it's even a good idea at all to check at this level. --- Why I suggested "grep the underlying file" is that I see the goal as making sure that the implementation doesn't start storing cookies unencrypted somewhere. The above catches the case of whether the existing schema operates as expected, but wouldn't catch if the implementation changed how things are stored. It's possible I misunderstand the goal, though. By "grep the file", I mean something like "Read the file to a string", and then call find() on that. I think the files are small enough that that's feasible, but as I noted, it's pretty implementation-specific. For instance, if we started using WAL mode, it would fail because the data would possibly be in a different file (which is why I suggested testing a positive-match case, also, which would then fail under WAL mode). Or if things were converted to leveldb. So I'm not sure how much effort should be put into detecting this kind of thing in the first place. https://codereview.chromium.org/24734007/diff/117001/content/browser/net/sqli... File content/browser/net/sqlite_persistent_cookie_store_unittest.cc (right): https://codereview.chromium.org/24734007/diff/117001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store_unittest.cc:581: } Doing this entirely at the SQLite level, it might be more general to do "SELECT * FROM cookies", and then for each result, loop over smt.ColumnCount() and check each column.
> Why I suggested "grep the underlying file" is that I see the goal as making sure > that the implementation doesn't start storing cookies unencrypted somewhere. I considered that but it the upgrade case, the old plaintext versions will likely continue to exist in "freed" blocks within the file, waiting to be overwritten by later stored data. https://codereview.chromium.org/24734007/diff/117001/content/browser/net/sqli... > content/browser/net/sqlite_persistent_cookie_store_unittest.cc:581: } > Doing this entirely at the SQLite level, it might be more general to do "SELECT > * FROM cookies", and then for each result, loop over smt.ColumnCount() and check > each column. I can do that.
On 2013/10/15 19:34:35, bcwhite wrote: > > Why I suggested "grep the underlying file" is that I see the goal as making > sure > > that the implementation doesn't start storing cookies unencrypted somewhere. > > I considered that but it the upgrade case, the old plaintext versions will > likely continue to exist in "freed" blocks within the file, waiting to be > overwritten by later stored data. This is certainly possible for old values which are being replaced. But if you turned on encryption and then stored "complicated_value", this should not be possible.
check all fields for absent plaintext
> This is certainly possible for old values which are being replaced. But if you > turned on encryption and then stored "complicated_value", this should not be > possible. Unless the internal representation is not raw. SQLite could (theoretically) compress the strings, for example.
On 2013/10/15 20:08:55, bcwhite wrote: > > This is certainly possible for old values which are being replaced. But if > you > > turned on encryption and then stored "complicated_value", this should not be > > possible. > > Unless the internal representation is not raw. SQLite could (theoretically) > compress the strings, for example. It should not be possible to see "complicated_value" in the raw file if it were compressed, either. The in-theory issue is why I suggested that it would need a positive test. Like "first_value" _does_ appear in the file when encryption is not on, but "encrypted_value" does _not_ appear when encryption is on. One will break if someone stops encrypting, the other will break if the file itself changes form (becomes encrypted, or compressed, or something like that). [I agree, I'm debating angels on a pinhead, here. I think this code is unlikely to accidentally be refactored away. So I'd only want to test it if it's not too hard, and relatively reliable.]
scan raw DB file for values to ensure encyrption
> The in-theory issue is why I suggested that it would need a positive test. Like > "first_value" _does_ appear in the file when encryption is not on, but > "encrypted_value" does _not_ appear when encryption is on. One will break if > someone stops encrypting, the other will break if the file itself changes form > (becomes encrypted, or compressed, or something like that). Done.
add some debug statements to try to find why it's breaking on the try server
LGTM, that's exactly the kind of thing I meant. Though it might be worthwhile using constants for the two instances of the items to prevent false positives due to typos (especially in the encrypted case, in the not-encrypted case the test should fail and take care of it).
https://codereview.chromium.org/24734007/diff/137001/android_webview/browser/... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/24734007/diff/137001/android_webview/browser/... android_webview/browser/aw_browser_context.cc:25: #include "content/public/browser/cookie_crypto_delegate.h" use forward decl instead. https://codereview.chromium.org/24734007/diff/137001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/24734007/diff/137001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_service.cc:40: #include "content/public/browser/cookie_crypto_delegate.h" forward decl https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store.cc:287: // all new values written will be encrypted on selected operating systems. Add backwards-compatibility statement. i.e., new rows read by old clients will lead to what behaviour? insert attempts from old clients will lead to what behaviour, including after coming back up to version 7 client? https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store.cc:718: DCHECK(value.empty()); This DCHECK serves no purpose; there is no assignment to value since its declaration. https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... File content/browser/net/sqlite_persistent_cookie_store_perftest.cc (right): https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store_perftest.cc:16: #include "content/public/browser/cookie_crypto_delegate.h" forward-decl https://codereview.chromium.org/24734007/diff/137001/content/public/browser/c... File content/public/browser/cookie_crypto_delegate.h (right): https://codereview.chromium.org/24734007/diff/137001/content/public/browser/c... content/public/browser/cookie_crypto_delegate.h:10: // This class provides an interface that the persistent cookies store can use // Implements encryption and decryption for the persistent cookie store.
addressed review comments by erikwright
https://codereview.chromium.org/24734007/diff/137001/android_webview/browser/... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/24734007/diff/137001/android_webview/browser/... android_webview/browser/aw_browser_context.cc:25: #include "content/public/browser/cookie_crypto_delegate.h" On 2013/10/22 23:58:14, erikwright wrote: > use forward decl instead. Done. https://codereview.chromium.org/24734007/diff/137001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/safe_browsing_service.cc (right): https://codereview.chromium.org/24734007/diff/137001/chrome/browser/safe_brow... chrome/browser/safe_browsing/safe_browsing_service.cc:40: #include "content/public/browser/cookie_crypto_delegate.h" On 2013/10/22 23:58:14, erikwright wrote: > forward decl Done. https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... File content/browser/net/sqlite_persistent_cookie_store.cc (right): https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store.cc:287: // all new values written will be encrypted on selected operating systems. On 2013/10/22 23:58:14, erikwright wrote: > Add backwards-compatibility statement. > > i.e., new rows read by old clients will lead to what behaviour? insert attempts > from old clients will lead to what behaviour, including after coming back up to > version 7 client? Done. https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store.cc:718: DCHECK(value.empty()); On 2013/10/22 23:58:14, erikwright wrote: > This DCHECK serves no purpose; there is no assignment to value since its > declaration. Right. It used to be loaded from ColumnString(3) but no longer. Removed. https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... File content/browser/net/sqlite_persistent_cookie_store_perftest.cc (right): https://codereview.chromium.org/24734007/diff/137001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store_perftest.cc:16: #include "content/public/browser/cookie_crypto_delegate.h" On 2013/10/22 23:58:14, erikwright wrote: > forward-decl Done. https://codereview.chromium.org/24734007/diff/137001/content/public/browser/c... File content/public/browser/cookie_crypto_delegate.h (right): https://codereview.chromium.org/24734007/diff/137001/content/public/browser/c... content/public/browser/cookie_crypto_delegate.h:10: // This class provides an interface that the persistent cookies store can use On 2013/10/22 23:58:14, erikwright wrote: > // Implements encryption and decryption for the persistent cookie store. Done.
need real declaration to instantiate scoped_ptr of an object
need real declaration to instantiate scoped_ptr of an object (android edition)
LGTM. Please revert to full include in the perftest. https://codereview.chromium.org/24734007/diff/137001/android_webview/browser/... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/24734007/diff/137001/android_webview/browser/... android_webview/browser/aw_browser_context.cc:25: #include "content/public/browser/cookie_crypto_delegate.h" On 2013/10/23 19:36:23, bcwhite wrote: > On 2013/10/22 23:58:14, erikwright wrote: > > use forward decl instead. > > Done. Sorry, I should have realized that you needed a full type there. https://codereview.chromium.org/24734007/diff/551001/content/browser/net/sqli... File content/browser/net/sqlite_persistent_cookie_store_perftest.cc (right): https://codereview.chromium.org/24734007/diff/551001/content/browser/net/sqli... content/browser/net/sqlite_persistent_cookie_store_perftest.cc:22: class CookieCryptoDelegate; This will need to be a full type too.
need real declaration to instantiate scoped_ptr of an object (perftest edition)
add trace of test failing on try-server to see what's going on
more printf debugging
more printf debugging
more printf debugging
fixed add-cookie race condition and removed all extra debug logging
fixed typo
+joi (OWNER for content/public) +rlp (OWNER for chrome/browser/profiles) +benm (OWNER for android_webview)
-me, +jam who has asked to review any new interfaces in //content/public.
android_webview lgtm
before I review, why is this bug and the linked design doc private?
Identity management and sign-out have privacy implications. The raw plans and thoughts have not been made public so as to avoid... ummm... misperceptions. -- Brian On Tue, Oct 29, 2013 at 1:24 PM, <jam@chromium.org> wrote: > before I review, why is this bug and the linked design doc private? > > https://codereview.chromium.**org/24734007/<https://codereview.chromium.org/2... > -- Brian bcwhite@google.com ----------------------------------------------------------------------------------------- *Treat someone as they are and they will remain that way. Treat someone as they can be and they will become that way. * To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/29 18:37:35, bcwhite_google.com wrote: > Identity management and sign-out have privacy implications. The raw plans > and thoughts have not been made public so as to avoid... ummm... > misperceptions. I'm not sure I buy this. Someone could just use the switch to see where we're headed. It seems that if there are misconceptions, we can clarify them when they arise. I'm not sure why we should use the possibility of confusion to justify moving some stuff to private while the code is public.
On 2013/10/29 19:12:15, jam wrote: > On 2013/10/29 18:37:35, http://bcwhite_google.com wrote: > > Identity management and sign-out have privacy implications. The raw plans > > and thoughts have not been made public so as to avoid... ummm... > > misperceptions. > > I'm not sure I buy this. Someone could just use the switch to see where we're > headed. It seems that if there are misconceptions, we can clarify them when they > arise. I'm not sure why we should use the possibility of confusion to justify > moving some stuff to private while the code is public. To explain myself more: if the design doc is internal and shouldn't be exposed, then is there a sanitized one available for external consumption? If not, then the bug should contain a brief summary. Right now the bug contains no information other than a link to a design doc that's private. The bug should contain more information, and there's no reason to make it private.
> To explain myself more: if the design doc is internal and shouldn't be > exposed, > then is there a sanitized one available for external consumption? If not, > then > the bug should contain a brief summary. Right now the bug contains no > information other than a link to a design doc that's private. The bug > should > contain more information, and there's no reason to make it private. > I brought this up in our weekly meeting and we came to the same conclusion. Brian bcwhite@google.com ----------------------------------------------------------------------------------------- *Treat someone as they are and they will remain that way. Treat someone as they can be and they will become that way. * To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/29 19:29:21, bcwhite_google.com wrote: > > To explain myself more: if the design doc is internal and shouldn't be > > exposed, > > then is there a sanitized one available for external consumption? If not, > > then > > the bug should contain a brief summary. Right now the bug contains no > > information other than a link to a design doc that's private. The bug > > should > > contain more information, and there's no reason to make it private. > > > > I brought this up in our weekly meeting and we came to the same conclusion. What conclusion? I'm curious why anyone thinks that bug, as is, should be private? Sorry to harp on this, but recently there's been too many things that are private that don't need to be. Can you please cc me on an email discussion if one exists? If not, who can I email about this? Now regarding the layering, it's not clear to me that this needs to be something that content asks its embedder for. src/components/webdata/encryptor/ has no dependencies other than src/crypto. Since it's used by multiple modules now (content, chrome, components), why not move that directory to src/crypto/encryptor?
lgtm profiles LGTM
I looked into moving webdata/Encryptor to under //crypto such but it was nixed by the security folks because, despite it's name, it's not actually encryption but like obfuscation because anybody with access to the OS account can recover the protected information with no need to know the actual encryption key. They want to key //crypto as pure, strong crypto.
On 2013/10/30 14:31:59, bcwhite wrote: > I looked into moving webdata/Encryptor to under //crypto such but it was nixed > by the security folks because, despite it's name, it's not actually encryption > but like obfuscation because anybody with access to the OS account can recover > the protected information with no need to know the actual encryption key. They > want to key //crypto as pure, strong crypto. I see. Is that discussion public? The issue here is that in general, when we have multiple top level modules (i.e. chrome, content) depending on some piece of code, we move it lower down. Usually that means base. But in this case, since this code depends on crypto that's not possible. We don't really use the content embedder API to get to generic code.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1191001
Retried try job too often on win_rel for step(s) browser_tests, content_unittests, sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1191001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1191001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1191001
Failed to apply patch for content/browser/net/sqlite_persistent_cookie_store.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/browser/net/sqlite_persistent_cookie_store.cc
Hunk #1 succeeded at 26 (offset -1 lines).
Hunk #2 succeeded at 74 (offset -1 lines).
Hunk #3 succeeded at 87 (offset -1 lines).
Hunk #4 succeeded at 271 (offset -1 lines).
Hunk #5 succeeded at 282 (offset -1 lines).
Hunk #6 succeeded at 305 (offset -1 lines).
Hunk #7 succeeded at 382 (offset -1 lines).
Hunk #8 succeeded at 692 (offset -1 lines).
Hunk #9 succeeded at 715 (offset -1 lines).
Hunk #10 succeeded at 859 (offset -1 lines).
Hunk #11 succeeded at 963 (offset -1 lines).
Hunk #12 succeeded at 996 (offset -1 lines).
Hunk #13 succeeded at 1201 (offset -1 lines).
Hunk #14 succeeded at 1253 (offset -1 lines).
Hunk #15 FAILED at 1264.
1 out of 15 hunks FAILED -- saving rejects to file
content/browser/net/sqlite_persistent_cookie_store.cc.rej
Patch: content/browser/net/sqlite_persistent_cookie_store.cc
Index: content/browser/net/sqlite_persistent_cookie_store.cc
diff --git a/content/browser/net/sqlite_persistent_cookie_store.cc
b/content/browser/net/sqlite_persistent_cookie_store.cc
index
517a8c18a1646d10af600c125a9e1b00d754d606..305674e652ce830f82a19c7ef5df9c74b797b562
100644
--- a/content/browser/net/sqlite_persistent_cookie_store.cc
+++ b/content/browser/net/sqlite_persistent_cookie_store.cc
@@ -27,6 +27,7 @@
#include "base/threading/sequenced_worker_pool.h"
#include "base/time/time.h"
#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/cookie_crypto_delegate.h"
#include "content/public/browser/cookie_store_factory.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "net/cookies/canonical_cookie.h"
@@ -74,7 +75,8 @@ class SQLitePersistentCookieStore::Backend
const scoped_refptr<base::SequencedTaskRunner>& client_task_runner,
const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
bool restore_old_session_cookies,
- quota::SpecialStoragePolicy* special_storage_policy)
+ quota::SpecialStoragePolicy* special_storage_policy,
+ scoped_ptr<CookieCryptoDelegate> crypto_delegate)
: path_(path),
num_pending_(0),
force_keep_session_state_(false),
@@ -86,7 +88,8 @@ class SQLitePersistentCookieStore::Backend
client_task_runner_(client_task_runner),
background_task_runner_(background_task_runner),
num_priority_waiting_(0),
- total_priority_requests_(0) {}
+ total_priority_requests_(0),
+ crypto_(crypto_delegate.Pass()) {}
// Creates or loads the SQLite database.
void Load(const LoadedCallback& loaded_callback);
@@ -269,6 +272,9 @@ class SQLitePersistentCookieStore::Backend
// The cumulative duration of time when |num_priority_waiting_| was greater
// than 1.
base::TimeDelta priority_wait_duration_;
+ // Class with functions that do cryptographic operations (for protecting
+ // cookies stored persistently).
+ scoped_ptr<CookieCryptoDelegate> crypto_;
DISALLOW_COPY_AND_ASSIGN(Backend);
};
@@ -277,6 +283,13 @@ namespace {
// Version number of the database.
//
+// Version 7 adds encrypted values. Old values will continue to be used but
+// all new values written will be encrypted on selected operating systems. New
+// records read by old clients will simply get an empty cookie value while old
+// records read by new clients will continue to operate with the unencrypted
+// version. New and old clients alike will always write/update records with
+// what they support.
+//
// Version 6 adds cookie priorities. This allows developers to influence the
// order in which cookies are evicted in order to meet domain cookie limits.
//
@@ -293,7 +306,7 @@ namespace {
// Version 3 updated the database to include the last access time, so we can
// expire them in decreasing order of use when we've reached the maximum
// number of cookies.
-const int kCurrentVersionNumber = 6;
+const int kCurrentVersionNumber = 7;
const int kCompatibleVersionNumber = 5;
// Possible values for the 'priority' column.
@@ -370,7 +383,8 @@ bool InitTable(sql::Connection* db) {
"last_access_utc INTEGER NOT NULL, "
"has_expires INTEGER NOT NULL DEFAULT 1, "
"persistent INTEGER NOT NULL DEFAULT 1,"
- "priority INTEGER NOT NULL DEFAULT %d)",
+ "priority INTEGER NOT NULL DEFAULT %d,"
+ "encrypted_value BLOB DEFAULT '')",
CookiePriorityToDBCookiePriority(net::COOKIE_PRIORITY_DEFAULT)));
if (!db->Execute(stmt.c_str()))
return false;
@@ -679,15 +693,16 @@ bool
SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
if (restore_old_session_cookies_) {
smt.Assign(db_->GetCachedStatement(
SQL_FROM_HERE,
- "SELECT creation_utc, host_key, name, value, path, expires_utc, "
- "secure, httponly, last_access_utc, has_expires, persistent, priority "
- "FROM cookies WHERE host_key = ?"));
+ "SELECT creation_utc, host_key, name, value, encrypted_value, path, "
+ "expires_utc, secure, httponly, last_access_utc, has_expires, "
+ "persistent, priority FROM cookies WHERE host_key = ?"));
} else {
smt.Assign(db_->GetCachedStatement(
SQL_FROM_HERE,
- "SELECT creation_utc, host_key, name, value, path, expires_utc, "
- "secure, httponly, last_access_utc, has_expires, persistent, priority "
- "FROM cookies WHERE host_key = ? AND persistent = 1"));
+ "SELECT creation_utc, host_key, name, value, encrypted_value, path, "
+ "expires_utc, secure, httponly, last_access_utc, has_expires, "
+ "persistent, priority FROM cookies WHERE host_key = ? "
+ "AND persistent = 1"));
}
if (!smt.is_valid()) {
smt.Clear(); // Disconnect smt_ref from db_.
@@ -701,20 +716,28 @@ bool
SQLitePersistentCookieStore::Backend::LoadCookiesForDomains(
for (; it != domains.end(); ++it) {
smt.BindString(0, *it);
while (smt.Step()) {
+ std::string value;
+ std::string encrypted_value = smt.ColumnString(4);
+ if (!encrypted_value.empty() && crypto_.get()) {
+ crypto_->DecryptString(encrypted_value, &value);
+ } else {
+ DCHECK(encrypted_value.empty());
+ value = smt.ColumnString(3);
+ }
scoped_ptr<net::CanonicalCookie> cc(new net::CanonicalCookie(
// The "source" URL is not used with persisted cookies.
GURL(), // Source
smt.ColumnString(2), // name
- smt.ColumnString(3), // value
+ value, // value
smt.ColumnString(1), // domain
- smt.ColumnString(4), // path
+ smt.ColumnString(5), // path
Time::FromInternalValue(smt.ColumnInt64(0)), // creation_utc
- Time::FromInternalValue(smt.ColumnInt64(5)), // expires_utc
- Time::FromInternalValue(smt.ColumnInt64(8)), // last_access_utc
- smt.ColumnInt(6) != 0, // secure
- smt.ColumnInt(7) != 0, // httponly
+ Time::FromInternalValue(smt.ColumnInt64(6)), // expires_utc
+ Time::FromInternalValue(smt.ColumnInt64(9)), // last_access_utc
+ smt.ColumnInt(7) != 0, // secure
+ smt.ColumnInt(8) != 0, // httponly
DBCookiePriorityToCookiePriority(
- static_cast<DBCookiePriority>(smt.ColumnInt(11))))); // priority
+ static_cast<DBCookiePriority>(smt.ColumnInt(12))))); // priority
DLOG_IF(WARNING,
cc->CreationDate() > Time::Now()) << L"CreationDate too recent";
cookies_per_origin_[CookieOrigin(cc->Domain(), cc->IsSecure())]++;
@@ -837,6 +860,26 @@ bool
SQLitePersistentCookieStore::Backend::EnsureDatabaseVersion() {
base::TimeTicks::Now() - start_time);
}
+ if (cur_version == 6) {
+ const base::TimeTicks start_time = base::TimeTicks::Now();
+ sql::Transaction transaction(db_.get());
+ if (!transaction.Begin())
+ return false;
+ // Alter the table to add empty "encrypted value" column.
+ if (!db_->Execute("ALTER TABLE cookies "
+ "ADD COLUMN encrypted_value BLOB DEFAULT ''")) {
+ LOG(WARNING) << "Unable to update cookie database to version 7.";
+ return false;
+ }
+ ++cur_version;
+ meta_table_.SetVersionNumber(cur_version);
+ meta_table_.SetCompatibleVersionNumber(
+ std::min(cur_version, kCompatibleVersionNumber));
+ transaction.Commit();
+ UMA_HISTOGRAM_TIMES("Cookie.TimeDatabaseMigrationToV7",
+ base::TimeTicks::Now() - start_time);
+ }
+
// Put future migration cases here.
if (cur_version < kCurrentVersionNumber) {
@@ -921,10 +964,10 @@ void SQLitePersistentCookieStore::Backend::Commit() {
return;
sql::Statement add_smt(db_->GetCachedStatement(SQL_FROM_HERE,
- "INSERT INTO cookies (creation_utc, host_key, name, value, path, "
- "expires_utc, secure, httponly, last_access_utc, has_expires, "
- "persistent, priority) "
- "VALUES (?,?,?,?,?,?,?,?,?,?,?,?)"));
+ "INSERT INTO cookies (creation_utc, host_key, name, value, "
+ "encrypted_value, path, expires_utc, secure, httponly, last_access_utc, "
+ "has_expires, persistent, priority) "
+ "VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)"));
if (!add_smt.is_valid())
return;
@@ -954,16 +997,26 @@ void SQLitePersistentCookieStore::Backend::Commit() {
add_smt.BindInt64(0, po->cc().CreationDate().ToInte…
(message too large)
rebased
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1801001
Failed to apply patch for android_webview/browser/aw_browser_context.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file android_webview/browser/aw_browser_context.cc
Hunk #2 FAILED at 144.
1 out of 2 hunks FAILED -- saving rejects to file
android_webview/browser/aw_browser_context.cc.rej
Patch: android_webview/browser/aw_browser_context.cc
Index: android_webview/browser/aw_browser_context.cc
diff --git a/android_webview/browser/aw_browser_context.cc
b/android_webview/browser/aw_browser_context.cc
index
ca3a5b8aefd52a319a0e9dd3b5adeec3836d80d9..028cbc804d0ba9f13071a7413c10faf69c022bbb
100644
--- a/android_webview/browser/aw_browser_context.cc
+++ b/android_webview/browser/aw_browser_context.cc
@@ -22,6 +22,7 @@
#include "components/user_prefs/user_prefs.h"
#include "components/visitedlink/browser/visitedlink_master.h"
#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/cookie_crypto_delegate.h"
#include "content/public/browser/cookie_store_factory.h"
#include "content/public/browser/resource_context.h"
#include "content/public/browser/storage_partition.h"
@@ -143,7 +144,8 @@ void AwBrowserContext::PreMainMessageLoopRun() {
true,
NULL,
NULL,
- background_task_runner);
+ background_task_runner,
+ scoped_ptr<content::CookieCryptoDelegate>());
cookie_store_->GetCookieMonster()->SetPersistSessionCookies(true);
url_request_context_getter_ =
rebased
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1941001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1941001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1941001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1941001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1941001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1941001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/1941001
disable on Mac because it breaks tests waiting for Keychain (will fix in another CL)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/2761001
Message was sent while issue was closed.
Change committed as 234855
Message was sent while issue was closed.
rebased
fix android build
removed left-over code from bad rebase
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/2981001
Message was sent while issue was closed.
Change committed as 240684
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/98603012/ by bcwhite@chromium.org. The reason for reverting is: buildbot failure in Chromium Memory FYI on Linux Tests (valgrind)(1).
fix memory leak in test
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/3001001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bcwhite@chromium.org/24734007/3001001
Message was sent while issue was closed.
Change committed as 241004 |
