Do not try to open DB in SessionStorageDatabase::DeleteNamespace()

Do not try to open DB in SessionStorageDatabase::DeleteNamespace()

DeleteNamespace() is called during shutdown (in a call chain: DOMStorageSession::~DOMStorageSession() -> DOMStorageContextImpl::DeleteSessionNamespace() -> SessionStorageDatabase::DeleteNamespace()).

Trying to open DB during shutdown causes unnecessary disk access, and it results in slowing down Chrome shutdown.
On Chrome OS, this is observed as a crash because Chrome gets aborted when it cannot finish shutdown quickly. (http://crbug.com/418627)

BUG=462500
Committed: https://crrev.com/36f040abc634e7c3bb41357c423b566d53c2072a
Cr-Commit-Position: refs/heads/master@{#318840}

[hashimoto, 2015-03-02 10:16:46]

hashimoto@chromium.org changed reviewers:
+ marja@chromium.org, michaeln@chromium.org

[hashimoto, 2015-03-02 10:16:47]

Could anyone (michaeln@ or marja@) look at this?

IIUC there is no need to open DB for DeleteNamespace() if it's not opened
already. (i.e. no data is written for the specified namespace)

Also, it seems DOMStorageContextImpl::StartScavengingUnusedSessionStorage() is
responsible to call DeleteNamespace() later even if the namespace is not deleted
during shutdown.
Anyways, no code should rely on shutdown performing clean up correctly as it can
be aborted, the device can be disconnected from the power supply, or chrome can
crash at any moment.

[marja, 2015-03-02 10:39:04]

On 2015/03/02 10:16:47, hashimoto wrote:
> Could anyone (michaeln@ or marja@) look at this?
> 
> IIUC there is no need to open DB for DeleteNamespace() if it's not opened
> already. (i.e. no data is written for the specified namespace)
> 
> Also, it seems DOMStorageContextImpl::StartScavengingUnusedSessionStorage() is
> responsible to call DeleteNamespace() later even if the namespace is not
deleted
> during shutdown.
> Anyways, no code should rely on shutdown performing clean up correctly as it
can
> be aborted, the device can be disconnected from the power supply, or chrome
can
> crash at any moment.

Hmm, I'm not sure I understand why this is supposed to work.

It can be that we start up, and then we scavenge (before we have used session
storage at all), and that's a perfectly valid course of events. Why wouldn't we
want to open the DB at that point? It can be that the data was written not on
this run, but on a previous run, and we should scavenge it. As you said, we
cannot assume that the shutdown is clean, so there can be some leftover data
from the previous run.

[hashimoto, 2015-03-02 10:47:55]

On 2015/03/02 10:39:04, marja wrote:
> On 2015/03/02 10:16:47, hashimoto wrote:
> > Could anyone (michaeln@ or marja@) look at this?
> > 
> > IIUC there is no need to open DB for DeleteNamespace() if it's not opened
> > already. (i.e. no data is written for the specified namespace)
> > 
> > Also, it seems DOMStorageContextImpl::StartScavengingUnusedSessionStorage()
is
> > responsible to call DeleteNamespace() later even if the namespace is not
> deleted
> > during shutdown.
> > Anyways, no code should rely on shutdown performing clean up correctly as it
> can
> > be aborted, the device can be disconnected from the power supply, or chrome
> can
> > crash at any moment.
> 
> Hmm, I'm not sure I understand why this is supposed to work.
> 
> It can be that we start up, and then we scavenge (before we have used session
> storage at all), and that's a perfectly valid course of events. Why wouldn't
we
> want to open the DB at that point? It can be that the data was written not on
> this run, but on a previous run, and we should scavenge it. As you said, we
> cannot assume that the shutdown is clean, so there can be some leftover data
> from the previous run.

DeleteNamespace() is called by only two functions:
- DOMStorageContextImpl::DeleteSessionNamespace(): This function is called only
by DOMStorageSession::~DOMStorageSession for cleanup purpose.
- DOMStorageContextImpl::DeleteNextUnusedNamespaceInCommitSequence(): This
function is run asynchronously by FindUnusedNamespacesInCommitSequence() for
scavenging.

For scavenging, IIUC we can expect the DB to be already opened as
FindUnusedNamespacesInCommitSequence() calls
SessionStorageDatabase::ReadNamespacesAndOrigins() which calls LazyOpen().

[marja, 2015-03-02 10:54:39]

Ah, that's true, for scavenging it should be open already. Now that I re-read
your message I see what you meant.

So... this only targets the DOMStorageSession::~DOMStorageSession use case. And
the namespace is *not* opened when we create it, only when we write data into
it, is that correct?

(It would be good to have comments into code to explain *why* opening is not
needed in DeleteNamespace.)

[hashimoto, 2015-03-02 11:17:21]

On 2015/03/02 10:54:39, marja wrote:
> Ah, that's true, for scavenging it should be open already. Now that I re-read
> your message I see what you meant.
> 
> So... this only targets the DOMStorageSession::~DOMStorageSession use case.
And
> the namespace is *not* opened when we create it, only when we write data into
> it, is that correct?
DOMStorageSession's ctor calls DOMStorageContextImpl::CreateSessionNamespace(),
but it seems it doesn't result in opening the DB.
> 
> (It would be good to have comments into code to explain *why* opening is not
> needed in DeleteNamespace.)
Sounds good. Added comments.
Any suggestion would be welcome.

[marja, 2015-03-02 11:19:02]

LGTM, but can you double-check w/ michaeln@ that this is OK.

(It has been a long while since I've last touched the code, which is probably
evident from my I-don't-quite-remember-how-this-works comments :)

[michaeln, 2015-03-02 18:57:51]

lgtm 2

[hashimoto, 2015-03-03 04:01:54]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2015-03-03 04:02:21]

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

[commit-bot: I haz the power, 2015-03-03 05:07:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-03 05:08:03]

Patchset 2 (id:??) landed as
https://crrev.com/36f040abc634e7c3bb41357c423b566d53c2072a
Cr-Commit-Position: refs/heads/master@{#318840}