Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2019333008:
[Offline Pages] Fix crash due to table inconsistency. (Closed)
|
Created:
4 years, 6 months ago by romax Modified:
4 years, 6 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Fix crash due to table inconsistency.
Adding logic to add a column if there wasn't one when upgrading.
BUG=615549
Committed: https://crrev.com/161706f2098084c856c3a9537a3e072aa5fd852f
Cr-Commit-Position: refs/heads/master@{#397872}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Replacing old table if schema is changed. And regression test. #Patch Set 3 : fixing build. #Patch Set 4 : fix win build #
Total comments: 11
Patch Set 5 : comments from fgorski@ #
Total comments: 17
Patch Set 6 : comments from jianli@ #
Messages
Total messages: 17 (5 generated)
romax@chromium.org changed reviewers: + jianli@chromium.org
PTAL and please let me know how I can repro/verify this crash is fixed...
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
romax@, I think this code will put the column at the end and it would be better to have it together with other integer fields. I think it is OK to blow the old table away and create a new one if the old one is missing a column. https://codereview.chromium.org/2019333008/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2019333008/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:93: "ALTER TABLE" OFFLINE_PAGES_TABLE_NAME I think you need a space after TABLE https://codereview.chromium.org/2019333008/diff/1/components/offline_pages/of... components/offline_pages/offline_page_metadata_store_sql.cc:94: "ADD COLUMN expiration_time INTEGER NOT NULL DEFAULT 0"); and before ADD
Please make sure you test if the crash is fixed. Also add a test if you can.
On 2016/06/01 16:52:04, fgorski wrote:
> romax@, I think this code will put the column at the end and it would be
better
> to have it together with other integer fields.
We should not put the column at the end because it will make column index
OP_EXPIRATION_TIME stops working. We should create a new table and fill it with
the old data, like:
ALTER TABLE {tableName} RENAME TO TempOldTable;
CREATE TABLE {tableName} (...);
INSERT INTO {tableName} (...) SELECT ... FROM TempOldTable;
DROP TABLE TempOldTable;
>
> I think it is OK to blow the old table away and create a new one if the old
one
> is missing a column.
>
>
https://codereview.chromium.org/2019333008/diff/1/components/offline_pages/of...
> File components/offline_pages/offline_page_metadata_store_sql.cc (right):
>
>
https://codereview.chromium.org/2019333008/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_metadata_store_sql.cc:93: "ALTER TABLE"
> OFFLINE_PAGES_TABLE_NAME
> I think you need a space after TABLE
>
>
https://codereview.chromium.org/2019333008/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_metadata_store_sql.cc:94: "ADD COLUMN
> expiration_time INTEGER NOT NULL DEFAULT 0");
> and before ADD
Updating, will recreate the table if columns are inconsistent, also adding a regression test. There are to-dos, since we might need a plan for database schema upgrade for future and pull out the SQL-related tests out of impl_tests.
https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:39: // TODO(romax): move it to sql_unittests. good point. Another test that we should include in upgrades is working with existing data, which should also find its way to extracted tests. https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:40: void BuildBadTable(const base::FilePath& file) { How about BuildStoreV1 (and then look at the comment below) https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:70: virtual OfflinePageMetadataStore* BuildBadStore( Can we call it: BuildV1Store() or something indicating that it was valid at some point but is no longer? Actually it seems that you are doing the upgrade in this method. How about: BuildStoreWithUpgrade() https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:208: std::unique_ptr<OfflinePageMetadataStore> store(this->BuildBadStore()); can you omit this->? https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:91: if (!db->Execute( because you are actually doing the upgrade, could you add a test with at least one entry?
Addressed comments from Filip, PTAL. https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:39: // TODO(romax): move it to sql_unittests. On 2016/06/03 18:17:16, fgorski wrote: > good point. > > Another test that we should include in upgrades is working with existing data, > which should also find its way to extracted tests. i've combined this case with the newly added test. https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:40: void BuildBadTable(const base::FilePath& file) { On 2016/06/03 18:17:16, fgorski wrote: > How about BuildStoreV1 (and then look at the comment below) Done. https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:70: virtual OfflinePageMetadataStore* BuildBadStore( On 2016/06/03 18:17:16, fgorski wrote: > Can we call it: BuildV1Store() or something indicating that it was valid at some > point but is no longer? > > Actually it seems that you are doing the upgrade in this method. How about: > BuildStoreWithUpgrade() Done. https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:208: std::unique_ptr<OfflinePageMetadataStore> store(this->BuildBadStore()); On 2016/06/03 18:17:16, fgorski wrote: > can you omit this->? no, since TYPED_TEST is a derived template class so accessing member should use this-> explicitly. https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:91: if (!db->Execute( On 2016/06/03 18:17:16, fgorski wrote: > because you are actually doing the upgrade, could you add a test with at least > one entry? Done.
lgtm This implementation works and I like it. I made an extra comment about table name and I would like to hear from you and Jian about that before you land thou. We can chat in the office. https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2019333008/diff/60001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:208: std::unique_ptr<OfflinePageMetadataStore> store(this->BuildBadStore()); On 2016/06/03 19:36:22, romax wrote: > On 2016/06/03 18:17:16, fgorski wrote: > > can you omit this->? > > no, since TYPED_TEST is a derived template class so accessing member should use > this-> explicitly. Acknowledged. Thanks for checking. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:29: #define OFFLINE_PAGES_TABLE_V1 "offlinepages_v1" wow. I didn't realize we are versioning the table name, that means you probably don't need to use temp table in the upgrade, don't you think?
lgtm https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:29: #define OFFLINE_PAGES_TABLE_V1 "offlinepages_v1" On 2016/06/03 20:04:17, fgorski wrote: > wow. I didn't realize we are versioning the table name, that means you probably > don't need to use temp table in the upgrade, don't you think? We can certainly do this, but using define in our sqllite code make versing transition very difficult. So I am fine with leaving as it is unless we want to refactor the logic. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:42: void BuildTestStoreV1(const base::FilePath& file) { BuildTestStoreV1 => BuildTestStoreWithOutdatedSchema https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:87: statement.BindString(9, path_string); Doing "statement.BindString(9, kFilePath.MaybeAsASCII())" should be good enough to avoid all these #if. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:190: std::unique_ptr<OfflinePageMetadataStore> BuildStoreV1WithUpgrade(); BuildStoreV1WithUpgrade => BuildStoreWithUpgradeFromOutdatedSchema https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:242: store->AddOrUpdateOfflinePage( Please also set expiration_time before calling AddOrUpdateOfflinePage and then read it back after 2nd load. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:258: EXPECT_EQ(2U, this->offline_pages_.size()); Change to use ASSERT_EQ. Please also add some basic validations for 2 entries, like: EXPECT_EQ(..., this->offline_pages_[0].offline_id); EXPECT_EQ(base::Time(), this->offline_pages_[0].expiration_time); EXPECT_EQ(..., this->offline_pages_[1].offline_id); EXPECT_EQ(..., this->offline_pages_[1].expiration_time); https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:29: const char kOfflinePagesColumns[] = Could you please comment that new column should be added to the end of the list below in order to avoid complicated table upgrade? https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:115: if (db->DoesTableExist(kOfflinePagesTable.table_name)) { I suggest to reorder all these if checks to make the code easier to understand and get rid of 2nd call of Commit. if (!db->DoesTableExist(kOfflinePagesTable.table_name)) { if (!CreateTable(db, kOfflinePagesTable) return false; } if (db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time")) return true; if (!RefreshColumns(db)) return false; return transaction.Commit(); }
addressed comments from jianli@, will commit once all try bots passed. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_impl_unittest.cc (right): https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:29: #define OFFLINE_PAGES_TABLE_V1 "offlinepages_v1" On 2016/06/03 21:12:18, jianli wrote: > On 2016/06/03 20:04:17, fgorski wrote: > > wow. I didn't realize we are versioning the table name, that means you > probably > > don't need to use temp table in the upgrade, don't you think? > > We can certainly do this, but using define in our sqllite code make versing > transition very difficult. So I am fine with leaving as it is unless we want to > refactor the logic. yes they'll be in a near future plan. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:42: void BuildTestStoreV1(const base::FilePath& file) { On 2016/06/03 21:12:18, jianli wrote: > BuildTestStoreV1 => BuildTestStoreWithOutdatedSchema Done. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:87: statement.BindString(9, path_string); On 2016/06/03 21:12:18, jianli wrote: > Doing "statement.BindString(9, kFilePath.MaybeAsASCII())" should be good enough > to avoid all these #if. Done. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:190: std::unique_ptr<OfflinePageMetadataStore> BuildStoreV1WithUpgrade(); On 2016/06/03 21:12:18, jianli wrote: > BuildStoreV1WithUpgrade => BuildStoreWithUpgradeFromOutdatedSchema Done. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:242: store->AddOrUpdateOfflinePage( On 2016/06/03 21:12:18, jianli wrote: > Please also set expiration_time before calling AddOrUpdateOfflinePage and then > read it back after 2nd load. Done. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_impl_unittest.cc:258: EXPECT_EQ(2U, this->offline_pages_.size()); On 2016/06/03 21:12:18, jianli wrote: > Change to use ASSERT_EQ. > > Please also add some basic validations for 2 entries, like: > EXPECT_EQ(..., this->offline_pages_[0].offline_id); > EXPECT_EQ(base::Time(), this->offline_pages_[0].expiration_time); > EXPECT_EQ(..., this->offline_pages_[1].offline_id); > EXPECT_EQ(..., this->offline_pages_[1].expiration_time); Done. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... File components/offline_pages/offline_page_metadata_store_sql.cc (right): https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:29: const char kOfflinePagesColumns[] = On 2016/06/03 21:12:18, jianli wrote: > Could you please comment that new column should be added to the end of the list > below in order to avoid complicated table upgrade? Done. https://codereview.chromium.org/2019333008/diff/80001/components/offline_page... components/offline_pages/offline_page_metadata_store_sql.cc:115: if (db->DoesTableExist(kOfflinePagesTable.table_name)) { On 2016/06/03 21:12:18, jianli wrote: > I suggest to reorder all these if checks to make the code easier to understand > and get rid of 2nd call of Commit. > if (!db->DoesTableExist(kOfflinePagesTable.table_name)) { > if (!CreateTable(db, kOfflinePagesTable) > return false; > } > > if (db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time")) > return true; > > if (!RefreshColumns(db)) > return false; > > return transaction.Commit(); > } emm I dont think we can return true since it may come from the create table above and it would rollback without committing the transaction. But i've changed the order. Thanks for the catch!
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2019333008/#ps100001 (title: "comments from jianli@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2019333008/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Fix crash due to table inconsistency. Adding logic to add a column if there wasn't one when upgrading. BUG=615549 ========== to ========== [Offline Pages] Fix crash due to table inconsistency. Adding logic to add a column if there wasn't one when upgrading. BUG=615549 Committed: https://crrev.com/161706f2098084c856c3a9537a3e072aa5fd852f Cr-Commit-Position: refs/heads/master@{#397872} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/161706f2098084c856c3a9537a3e072aa5fd852f Cr-Commit-Position: refs/heads/master@{#397872} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
