Wipe out offline page data on clearing cookie and site data

Wipe out offline page data on clearing cookie and site data

BUG=491352
Committed: https://crrev.com/23248b4cbc2dd78b1673a90a3624bb4cfff7e447
Cr-Commit-Position: refs/heads/master@{#356442}

[jianli, 2015-10-22 23:57:25]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org, mathp@chromium.org, michaeln@chromium.org

[jianli, 2015-10-22 23:57:25]

fgorski for offline pages
michaeln for browsing data
mathp for leveldb proto

[Mathieu, 2015-10-23 18:14:47]

lgtm for leveldb_proto

[michaeln, 2015-10-23 20:16:56]

bdr lgtm

[fgorski, 2015-10-23 20:55:03]

Jian Li,

Could you change all the yyyCompleted methods to yyyDone? Based on:
https://code.google.com/p/chromium/codesearch#search/&q=Done%20file:%5Esrc/co...
vs.
https://code.google.com/p/chromium/codesearch#search/&q=Completed%20file:%5Es...

Thanks.
Filip

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:123:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);
does it make sense to database_.reset() here? to not have it hang in memory?

Otherwise, if we expect the caller to properly clean up by calling Reset, that
should be clearly documented.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:143:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);
It would likely make sense to distinguish this case from the previous one and
hence turn this UMA into an enum pointing to where it failed.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:151: if
(!OfflinePageItemFromEntry(entry, &item)) {
Explain why you are changing this. This sounds too aggressive. This will make
the whole DB load fail if one of the entries is busted.

We could drop that entry instead of failing completely.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:231:
weak_ptr_factory_.GetWeakPtr(),
nit: git cl format is your friend.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:227:
bookmark_ids.push_back(id_page_pair.second.bookmark_id);
bookmark_ids.push_back(id_page_pair.first);

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:574:
weak_ptr_factory_.GetWeakPtr(),
nit: alignment is off

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:579: const base::Closure&
callback, bool success) {
can you more explicitly ignore success here?

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:597: const
std::vector<OfflinePageItem>& offline_pages) {
offline_pages.clear(); should probably go here as well.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:171: void ClearAll(const
base::Closure& callback);
DeleteAllPages -- to be consistent with the naming above.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:913: EXPECT_EQ(1UL,
GetStore()->offline_pages().size());
come on :) have at least 2 items in the store here ;)

[jianli, 2015-10-26 21:42:44]

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:123:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);
On 2015/10/23 20:55:03, fgorski wrote:
> does it make sense to database_.reset() here? to not have it hang in memory?
> 
> Otherwise, if we expect the caller to properly clean up by calling Reset, that
> should be clearly documented.

Done.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:143:
UMA_HISTOGRAM_BOOLEAN("OfflinePages.LoadSuccess", false);
On 2015/10/23 20:55:03, fgorski wrote:
> It would likely make sense to distinguish this case from the previous one and
> hence turn this UMA into an enum pointing to where it failed.

Done.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:151: if
(!OfflinePageItemFromEntry(entry, &item)) {
On 2015/10/23 20:55:03, fgorski wrote:
> Explain why you are changing this. This sounds too aggressive. This will make
> the whole DB load fail if one of the entries is busted.
> 
> We could drop that entry instead of failing completely.

I don't think it is safe to allow partial load when certain data were corrupted
unless we add checksum to validate each entry. If some bad thing happens, we
should not trust the store data at all and we should rebuild it.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:231:
weak_ptr_factory_.GetWeakPtr(),
On 2015/10/23 20:55:03, fgorski wrote:
> nit: git cl format is your friend.

Done.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:227:
bookmark_ids.push_back(id_page_pair.second.bookmark_id);
On 2015/10/23 20:55:03, fgorski wrote:
> bookmark_ids.push_back(id_page_pair.first);

Done.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:574:
weak_ptr_factory_.GetWeakPtr(),
On 2015/10/23 20:55:03, fgorski wrote:
> nit: alignment is off 

Done.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:579: const base::Closure&
callback, bool success) {
On 2015/10/23 20:55:03, fgorski wrote:
> can you more explicitly ignore success here?

Done.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:597: const
std::vector<OfflinePageItem>& offline_pages) {
On 2015/10/23 20:55:03, fgorski wrote:
> offline_pages.clear(); should probably go here as well.

Done.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:171: void ClearAll(const
base::Closure& callback);
On 2015/10/23 20:55:03, fgorski wrote:
> DeleteAllPages -- to be consistent with the naming above.

I think semantically ClearAll is a bit different from DeleteAllPages since the
former also means to destroy the whole store while the latter does not need
this. I rather use ClearAll to mean a special wipe out like this.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:913: EXPECT_EQ(1UL,
GetStore()->offline_pages().size());
On 2015/10/23 20:55:03, fgorski wrote:
> come on :) have at least 2 items in the store here ;)

Done.

[jianli, 2015-10-26 22:54:01]

jianli@chromium.org changed reviewers:
+ rkaplow@chromium.org

[jianli, 2015-10-26 22:54:01]

rkaplow for new UMAs.

[rkaplow, 2015-10-26 23:03:27]

lgtm

[fgorski, 2015-10-27 20:13:09]

Hey, you didn't address my Done vs. Complete comment earlier. Please see my last
message.

Other than that things look good.

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1420003004/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:171: void ClearAll(const
base::Closure& callback);
On 2015/10/26 21:42:44, jianli wrote:
> On 2015/10/23 20:55:03, fgorski wrote:
> > DeleteAllPages -- to be consistent with the naming above.
> 
> I think semantically ClearAll is a bit different from DeleteAllPages since the
> former also means to destroy the whole store while the latter does not need
> this. I rather use ClearAll to mean a special wipe out like this.

Acknowledged.

https://codereview.chromium.org/1420003004/diff/50017/components/offline_page...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1420003004/diff/50017/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:122:
database_.reset();
should we connect db reset and callback into one function to make sure it
happens together upon load failure?

RunLoadCallback(status, callback, result) {
if (status != LOAD_SUCCEDED) {
  database_.reset();
  result.clear();
}
callback.Run(status, result);
}

This is to ensure we never hang on to the database if it failed to load.

https://codereview.chromium.org/1420003004/diff/50017/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1420003004/diff/50017/components/offline_page...
components/offline_pages/offline_page_model.cc:593: DCHECK(success);
DCHECK makes sense :)

[jianli, 2015-10-27 21:55:04]

Also renamed methods ended with Completed to Done.

https://codereview.chromium.org/1420003004/diff/50017/components/offline_page...
File components/offline_pages/offline_page_metadata_store_impl.cc (right):

https://codereview.chromium.org/1420003004/diff/50017/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:122:
database_.reset();
On 2015/10/27 20:13:09, fgorski wrote:
> should we connect db reset and callback into one function to make sure it
> happens together upon load failure?
> 
> RunLoadCallback(status, callback, result) {
> if (status != LOAD_SUCCEDED) {
>   database_.reset();
>   result.clear();
> }
> callback.Run(status, result);
> }
> 
> This is to ensure we never hang on to the database if it failed to load. 

Done.

[jianli, 2015-10-27 21:55:06]

Also renamed methods ended with Completed to Done.

[fgorski, 2015-10-27 22:14:52]

lgtm

[fgorski, 2015-10-27 22:14:53]

lgtm

[jianli, 2015-10-27 22:15:40]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-10-27 22:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, mathp@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1420003004/#ps70001
(title: "Address more feedback")

[commit-bot: I haz the power, 2015-10-27 22:17:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420003004/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420003004/70001

[commit-bot: I haz the power, 2015-10-27 23:56:42]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-10-27 23:58:04]

Patchset 5 (id:??) landed as
https://crrev.com/23248b4cbc2dd78b1673a90a3624bb4cfff7e447
Cr-Commit-Position: refs/heads/master@{#356442}

[commit-bot: I haz the power, 2015-10-27 23:58:07]

Failed to apply the patch.

[Nico, 2015-10-28 03:24:23]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-10-28 03:24:23]

this breaks the clang/win build:

In file included from
..\..\components\offline_pages\offline_page_metadata_store_impl_unittest.cc:14:
..\..\components/leveldb_proto/proto_database_impl.h(102,6) : error: unused
function 'InitFromTaskRunner' [-Werror,-Wunused-function]
void InitFromTaskRunner(LevelDB* database, const base::FilePath& database_dir,
     ^
..\..\components/leveldb_proto/proto_database_impl.h(110,6) : error: unused
function 'DestroyFromTaskRunner' [-Werror,-Wunused-function]
void DestroyFromTaskRunner(const base::FilePath& database_dir, bool* success) {
     ^
2 errors generated.

Can you take a look?