Restore sessionStorage when chrome restarts.

Restore sessionStorage when chrome restarts.

Write sessionStorage always to disk (like localStorage). When restoring tabs
after a restart or a crash, associate the sessionStorages with the correct tabs.

BUG=104292
TEST=TBD

[jochen (gone - plz use gerrit), 2011-12-13 13:47:32]

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_service.h:437: typedef std::map<int64,
please add a comment what is mapped to what here

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_context.cc:287: // We cannot use
derive the directory name for the sessionStorage databases
remove "use"

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
File content/browser/in_process_webkit/dom_storage_namespace.h (right):

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_namespace.h:93: struct
DOMStorageNamespaceCreatedDetails {
could you add two constructors (one with and one without parameters) and a
destructor?

http://codereview.chromium.org/8929007/diff/1/content/public/browser/notifica...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/8929007/diff/1/content/public/browser/notifica...
content/public/browser/notification_types.h:425: // Sent when a
DOMStorageNamespace is created.
please say what the Source and the Details will be.

Is it send of all namespaces or just for session storage ones?

[michaeln, 2011-12-13 20:40:03]

Under what circumstances are tabs/sessions restored from disk? I'm not clear on
what the use-cases / requirements are.

As mentioned in our email thread, I'm wondering if this is worth the effort or
not. Not sure its better to always incur the disk cost for the infrequent
'restart for upgrade' use case. In that 'restart' case, we shutdown a little
faster... but in the normal case we run a little slower, most especially in the
the normal startup/shutdown sequence where we have to go out and delete it at
some point (where does that happen?). Not sure i buy that tradeoff.

There are tentative plans to work on DOMStorage in Q1 to eliminate our
dependency on webcore/WEBKIT_THREAD. This new persistent session storage feature
adds a bunch of complexity. If we must do some flavor of 'persistent session
storage', it might be nice to do that after we've eliminated our dependency on
webcore/WEBKIT_THREAD.

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_restore.cc:696: // Don't schedule a load for the
selected tab, as ShowBrowser() will
Does this mean the navigation to the top most url on the stack has already
happened (or is happening concurrently)? If so, is there a race here between the
page creating a new empty sessionStorage namespace and the logic under
RestoreTab to reanimate an save one?

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_restore.cc:745: base::Unretained(this),
dom_storage_context, namespace_id,
what ensures 'this' is still valid when the task executes?

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_service.cc:654: ScheduleCommand(new
SessionCommand(kCommandSessionStorageCreated,
I'm not at all familiar with this SessionService class or its base. Can you tell
me about what it means to 'schedule a command' and wassup with the ResetCommand
stuff?

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_context.cc:175: dir_path =
data_path_.Append(kSessionStorageDirectory)
in what circumstances is data_path_ empty, i'm guessing for incognito?

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_context.cc:289: // directory won't
be created before we restore the session.
can you say more about the latter part of the comment?

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_context.cc:292:
data_path_.Append(kSessionStorageDirectory), "", &dir_path);
where/when are these directories deleted?

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
File content/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_message_filter.cc:143:
base::Bind(&DOMStorageMessageFilter::NotifyOnUIThread, this,
Is the intent to only send this notification for sessionStorage namespaces or
for localStorage namespaces too? Looks like its doing the latter, but the last
param makes it seem like the intent is the former.

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
File content/browser/in_process_webkit/dom_storage_namespace.cc (right):

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_namespace.cc:110: // we want to
save it on disk.
So we won't be using the SessionStorage stuff in webcore at all anymore. This is
likely to confuse folks.

[michaeln, 2011-12-15 23:15:08]

So based in the CPS meeting... sounds like the restore feature s/b targeting the
most general use case where a normal browser start effectively restores the
session state from the prior run.

[marja, 2012-01-11 15:17:53]

I'm finally resuming this sessionStorage work, here are some old comments. (Not
all problems you've pointed out are resolved yet & this CL needs more work.)

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_service.cc:654: ScheduleCommand(new
SessionCommand(kCommandSessionStorageCreated,
On 2011/12/13 20:40:03, michaeln wrote:
> I'm not at all familiar with this SessionService class or its base. Can you
tell
> me about what it means to 'schedule a command' and wassup with the
ResetCommand
> stuff?

ScheduleCommand puts the command into a queue and schedules it to be written to
the disk later. When the session is restored, these commands are then read from
the disk and the session is constructed based on them.

ScheduleReset builds the set of commands again based on the current windows,
tabs, and navigations, and when they are eventually written to the disk, the
previous commands are discarded.

And this CL has a problem that it doesn't take the ScheduleReset into account.
Yet.

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_service.h:437: typedef std::map<int64,
On 2011/12/13 13:47:32, jochen wrote:
> please add a comment what is mapped to what here

Done.

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_context.cc:175: dir_path =
data_path_.Append(kSessionStorageDirectory)
On 2011/12/13 20:40:03, michaeln wrote:
> in what circumstances is data_path_ empty, i'm guessing for incognito?

Yes.

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_context.cc:287: // We cannot use
derive the directory name for the sessionStorage databases
On 2011/12/13 13:47:32, jochen wrote:
> remove "use"

Done.

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_context.cc:289: // directory won't
be created before we restore the session.
On 2011/12/13 20:40:03, michaeln wrote:
> can you say more about the latter part of the comment?

This was inaccurate, changed it to "There is no guarantee that the same
namespace ID won't be reused before we restore the session."

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_context.cc:292:
data_path_.Append(kSessionStorageDirectory), "", &dir_path);
On 2011/12/13 20:40:03, michaeln wrote:
> where/when are these directories deleted?

The deletion code is not yet in this CL, but the directories will be deleted
during a clean shutdown if we know we're not going to restore the session on the
next startup, and we'd also need to delete the stray ones after a crash if the
user doesn't restore the session...

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
File content/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_message_filter.cc:143:
base::Bind(&DOMStorageMessageFilter::NotifyOnUIThread, this,
On 2011/12/13 20:40:03, michaeln wrote:
> Is the intent to only send this notification for sessionStorage namespaces or
> for localStorage namespaces too? Looks like its doing the latter, but the last
> param makes it seem like the intent is the former.

Fixed this.

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
File content/browser/in_process_webkit/dom_storage_namespace.h (right):

http://codereview.chromium.org/8929007/diff/1/content/browser/in_process_webk...
content/browser/in_process_webkit/dom_storage_namespace.h:93: struct
DOMStorageNamespaceCreatedDetails {
On 2011/12/13 13:47:32, jochen wrote:
> could you add two constructors (one with and one without parameters) and a
> destructor?

Done.

http://codereview.chromium.org/8929007/diff/1/content/public/browser/notifica...
File content/public/browser/notification_types.h (right):

http://codereview.chromium.org/8929007/diff/1/content/public/browser/notifica...
content/public/browser/notification_types.h:425: // Sent when a
DOMStorageNamespace is created.
On 2011/12/13 13:47:32, jochen wrote:
> please say what the Source and the Details will be.
> 
> Is it send of all namespaces or just for session storage ones?

Done, and fixed this so that it's only sent for session storage namespaces, and
changed the name accordingly.

[marja, 2012-01-17 16:21:49]

Hi,

I uploaded a new version which (hopefully) tackles:
- ScheduleResets in SessionService
- Cloning sessionStorages
- Purging sessionStorages
- Deleting sessionStorage db's (this is not quite right yet, needs some more
work).

{jochen, michaeln}, can you have a look if this is going to the right direction?
It's getting rather complicated, wdyt? I begin to doubt my approach.. Otoh, I
don't know of any simpler approach (possibly after the DOM refactoring). Do you
have any ideas?

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_restore.cc:696: // Don't schedule a load for the
selected tab, as ShowBrowser() will
On 2011/12/13 20:40:03, michaeln wrote:
> Does this mean the navigation to the top most url on the stack has already
> happened (or is happening concurrently)? If so, is there a race here between
the
> page creating a new empty sessionStorage namespace and the logic under
> RestoreTab to reanimate an save one?

Afaics, both callers of this func, SessionRestoreImpl::RestoreForeignSession and
SessionRestoreImpl::ProcessSessionWindows call ShowBrowser after calling this
function. I don't understand why the comment says "will already have done"
instead of "will do".

http://codereview.chromium.org/8929007/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_restore.cc:745: base::Unretained(this),
dom_storage_context, namespace_id,
On 2011/12/13 20:40:03, michaeln wrote:
> what ensures 'this' is still valid when the task executes?

This is fixed now; WebKitContext is refcounted.

[michaeln, 2012-01-17 21:25:13]

http://codereview.chromium.org/8929007/diff/29001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/8929007/diff/29001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_context.cc:350: // have a simple
function for doing this.
Not sure this approach to cloning session storage works? See
StorageAreaSync.cpp. I think this approach relies on synchronous characteristics
that webcore's localstorage system does not have.

* the files being copied are accessed on a background thread so at the time
these file copies occur they may be in some intermediary state

* key/value mutations are not immediately written to an underlying file on disk,
the current webcore impl has a one second timer.

[marja, 2012-01-19 12:43:42]

http://codereview.chromium.org/8929007/diff/29001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/8929007/diff/29001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_context.cc:350: // have a simple
function for doing this.
On 2012/01/17 21:25:13, michaeln wrote:
> Not sure this approach to cloning session storage works? See
> StorageAreaSync.cpp. I think this approach relies on synchronous
characteristics
> that webcore's localstorage system does not have.
> 
> * the files being copied are accessed on a background thread so at the time
> these file copies occur they may be in some intermediary state
> 
> * key/value mutations are not immediately written to an underlying file on
disk,
> the current webcore impl has a one second timer.

Ah, true, this was broken, thanks for pointing it out. I uploaded a new version
which reads the data from DOMStorageAreas with GetItem and sets it to another
DOMStorageArea with SetItem; how does that look?