Destroy the notification database when corruption occurs.

Destroy the notification database when corruption occurs.

When corruption occurs in the notification database, destroy the
entire database, which means that the database will be re-created
during the next invocation. If the corruption occurs while opening
the database, try re-opening it immediately so that the operation
can continue.

Design document:
  http://goo.gl/TciXVp

BUG=447628
Committed: https://crrev.com/72f559a45853c2191377386aa117a0c48c6e8be8
Cr-Commit-Position: refs/heads/master@{#322709}

[Peter Beverloo, 2015-03-19 21:33:15]

peter@chromium.org changed reviewers:
+ cmumford@chromium.org, johnme@chromium.org

[Peter Beverloo, 2015-03-19 21:33:15]

+johnme for notifications
+cmumford

Christopher, you mentioned in an earlier CL[1] that LevelDB has the ability to
try to repair a database, where I replied that it wouldn't provide a good user
experience if we'd do that for notifications. (We'd leave behind non-functional
notifications.)

This CL implements the blow-away-on-corruption one layer higher than the
NotificationDatabase. If corruption occurs whilst opening the database, it will
be destroyed and re-opened immediately after, otherwise it'll be closed just
before marking an operation as failed (unless data is being removed).

Is this behavior too harsh? I also remove all left-over files in the database's
directory.

Of course, UMA will give us insight in how frequently this is happening in real
life. Hopefully it'll be rare.

[1] https://codereview.chromium.org/988163003

[cmumford, 2015-03-19 22:12:28]

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:28:
FAIL_OPERATION,
I can't really think of a time where you'd want to *not* delete a corrupted
database. The only thing that comes to mine are: 1) utility (diagnostic) apps,
or 2) working with a read-only filesystem.

If you don't currently intend to pass in FAIL_OPERATION then I'd just do away
with the enum and make the delete on corrupt db implicit.

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:292:
OpenDatabase(success_closure,
If I'm reading this correctly the user of this function can't know if the db was
deleted, and data was lost. I'll trust your judgement if that's OK.

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:298:
database_.reset();
Do you want to reset this ptr above before calling OpenDatabase a second time? I
think it'll work fine, but might be clearer that way.

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:303: void
PlatformNotificationContextImpl::DestroyDatabase() {
You should return a status code here, and check it above. You will have
antivirus software, etc. holding files open, preventing delete from succeeding,
and you'll want to catch that.

[Peter Beverloo, 2015-03-20 14:31:27]

Thank you for the review!

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:28:
FAIL_OPERATION,
On 2015/03/19 22:12:27, cmumford wrote:
> I can't really think of a time where you'd want to *not* delete a corrupted
> database. The only thing that comes to mine are: 1) utility (diagnostic) apps,
> or 2) working with a read-only filesystem.
> 
> If you don't currently intend to pass in FAIL_OPERATION then I'd just do away
> with the enum and make the delete on corrupt db implicit.

Done.

Note that this CL used this to avoid getting in an infinite loop when opening a
database that's corrupted. I've instead passed a boolean named
|delete_on_corruption|.

(I work a lot in Blink, where enums are preferred over booleans.)

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:292:
OpenDatabase(success_closure,
On 2015/03/19 22:12:27, cmumford wrote:
> If I'm reading this correctly the user of this function can't know if the db
was
> deleted, and data was lost. I'll trust your judgement if that's OK.

Indeed. The TODO on line 317 covers this - when the database gets destroyed, or
notifications get removed due to a Service Worker unregistering, the context
will take care of closing the necessary notifications with the
PlatformNotificationService.

This hasn't been implemented yet, hence the TODO.

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:298:
database_.reset();
On 2015/03/19 22:12:27, cmumford wrote:
> Do you want to reset this ptr above before calling OpenDatabase a second time?
I
> think it'll work fine, but might be clearer that way.

Currently DestroyDatabase() calls NotificationDatabase::Destroy() on corruption
-- we only know whether corruption occurred after the database has been opened.
It then resets |database_| on line 309.

The reset on line 298 is for the "corruption or failure, but don't destroy the
database" case.

https://codereview.chromium.org/1024463006/diff/20001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:303: void
PlatformNotificationContextImpl::DestroyDatabase() {
On 2015/03/19 22:12:27, cmumford wrote:
> You should return a status code here, and check it above. You will have
> antivirus software, etc. holding files open, preventing delete from
succeeding,
> and you'll want to catch that.

That's an interesting case, I hadn't considered that. How should we track this?
UMA?

If we can't open the database, NotificationDatabase::Destroy() fails and
deleting the files fail as well, we'd end up invoking the failure closure in
OpenDatabase().

[johnme, 2015-03-20 15:29:07]

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:182: // Blow
away the database if reading data failed due to corruption. Following
s/reading/deleting/

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_unittest.cc:297:
base::RunLoop().RunUntilIdle();
As in https://codereview.chromium.org/1014703007/, this smells flaky. I'll wait
and see what you reply there.

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_unittest.cc:349:
EXPECT_TRUE(IsDirectoryEmpty(database_dir.path()));
It's not immediately obvious that a read operation would write anything to disk.
Please add a corresponding EXPECT_FALSE(IsDirectoryEmpty(...)) above the
OnStorageWiped call.

[cmumford, 2015-03-20 17:36:52]

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:186:
DestroyDatabase();
Well I guess you will have times where DestroyDatabase fails, but that will
almost surely leave a partial leveldb folder, so the notification data is likely
destroyed, so I guess calling this success is OK.

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:281: false
/* delete_on_corruption */);
I see your point about booleans - I prefer the relatively self-commenting nature
of enums myself.

What I was thinking is just to have
PlatformNotificationContextImpl::OpenDatabase do an open (if corrupt, delete the
folder (fail if that fails), and then open a second time. The caller (in this
simple example) wouldn't know that the db was wacked, but you could have a third
success_corruption_restored closure if you wanted that.

This way you could avoid recursion and have a simpler (at least to me) flow.

[Peter Beverloo, 2015-03-20 18:55:38]

All done :-).

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_impl.cc
(right):

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:182: // Blow
away the database if reading data failed due to corruption. Following
On 2015/03/20 15:29:06, johnme wrote:
> s/reading/deleting/

Done.

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_impl.cc:281: false
/* delete_on_corruption */);
On 2015/03/20 17:36:52, cmumford wrote:
> I see your point about booleans - I prefer the relatively self-commenting
nature
> of enums myself.
> 
> What I was thinking is just to have
> PlatformNotificationContextImpl::OpenDatabase do an open (if corrupt, delete
the
> folder (fail if that fails), and then open a second time. The caller (in this
> simple example) wouldn't know that the db was wacked, but you could have a
third
> success_corruption_restored closure if you wanted that.
> 
> This way you could avoid recursion and have a simpler (at least to me) flow.

That's a great suggestion - thank you once again! :-) It also allows us to track
the ability to open a database after it has been blown away due to corruption.

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context_unittest.cc
(right):

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_unittest.cc:297:
base::RunLoop().RunUntilIdle();
On 2015/03/20 15:29:06, johnme wrote:
> As in https://codereview.chromium.org/1014703007/, this smells flaky. I'll
wait
> and see what you reply there.

As replied there - it's not flaky because they're the same thread :).

https://codereview.chromium.org/1024463006/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context_unittest.cc:349:
EXPECT_TRUE(IsDirectoryEmpty(database_dir.path()));
On 2015/03/20 15:29:06, johnme wrote:
> It's not immediately obvious that a read operation would write anything to
disk.
> Please add a corresponding EXPECT_FALSE(IsDirectoryEmpty(...)) above the
> OnStorageWiped call.

Done.

[cmumford, 2015-03-20 19:34:23]

lgtm

[johnme, 2015-03-23 18:34:44]

lgtm

[Peter Beverloo, 2015-03-28 20:49:34]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-28 20:49:35]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/1024463006/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-28 20:49:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1024463006/80001

[commit-bot: I haz the power, 2015-03-28 21:36:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-28 21:37:33]

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