Session restore: Store and restore persistent IDs for sessionStorage.

Session restore: Store and restore persistent IDs for sessionStorage.

Saving the sessionStorage data on disk will be done in a future CL.

BUG=104292
TEST=TBD


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143709

[marja, 2012-06-19 13:55:28]

Hi sky,michaeln,

This CL stores and restores the permanent IDs for sessionStorage in the session
data. In the next CL, I'll store and restore the sessionStorage data. This CL
recreates sessionStorages with a given persistent ID but the data is not yet
saved. (When the data gets saved, it's automatically correctly associated back
when a sessionStorage with the correct ID is created.)

(Draft of the big picture is here:
https://chromiumcodereview.appspot.com/10572015/ )

Could you review the following parts:
sky: chrome/browser/sessions/*
michaeln: all

[sky, 2012-06-19 14:11:59]

How about some tests?

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:605: // Record the association
between the sessionStorage namespace and the
Should that be SessionStorage?

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:622:
content::SessionStorageNamespace* session_storage_namespace =
If we shutdown the session service before the tabcontents will the session
storage leak?

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:862:
pickle.WriteString(session_storage_persistent_id);
Can session_storage_persistent_id be bigger than bytes available?

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1267: pickle->ReadInt(&iter,
&tab_id);
Check return value of these two.

[michaeln, 2012-06-20 04:01:59]

http://codereview.chromium.org/10572015/diff/2001/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/10572015/diff/2001/chrome/browser/sessions/ses...
chrome/browser/sessions/session_service.cc:622:
content::SessionStorageNamespace* session_storage_namespace =
On 2012/06/19 14:11:59, sky wrote:
> If we shutdown the session service before the tabcontents will the session
> storage leak?

The question about order at shutdown is interesting. During shutdown with the
'continue where i left off' setting turned on, are the
NOTIFICATION_TAB_CONTENTS_DESTROYED processed when tabs that will be restored
upon restart are cleaned up? How does the session service ignore state changes
it wants to ignore near the end of time? Does it skip saving SessionCommands
generatred near the end, or does it pull the plug on the notifications such that
no commands are generated near the end?

The call to SetShouldPersist(false) will take effect even if a
TabClosedSessionCommand created by TabClosed is not put in the session restore
journal.

Hopefully you two know more about the session restore stuff than i do :) I'm not
so familiar with this chrome/browser code.

http://codereview.chromium.org/10572015/diff/2001/content/browser/dom_storage...
File content/browser/dom_storage/dom_storage_context_impl.h (right):

http://codereview.chromium.org/10572015/diff/2001/content/browser/dom_storage...
content/browser/dom_storage/dom_storage_context_impl.h:15: class
SessionStorageNamespace;
since this is forward declared in content/public/browser/dom_storage_context.h,
which is included here, this probably doesn't need to be here... the forward
here is needed to declare the override of the method there

http://codereview.chromium.org/10572015/diff/2001/content/public/browser/dom_...
File content/public/browser/dom_storage_context.h (right):

http://codereview.chromium.org/10572015/diff/2001/content/public/browser/dom_...
content/public/browser/dom_storage_context.h:35: virtual
scoped_refptr<SessionStorageNamespace> RecreateSessionStorage(
a comment about what this method does &| its intended use would be nice

http://codereview.chromium.org/10572015/diff/2001/content/public/browser/sess...
File content/public/browser/session_storage_namespace.h (right):

http://codereview.chromium.org/10572015/diff/2001/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:39:
DISALLOW_COPY_AND_ASSIGN(SessionStorageNamespace);
i'm not sure this is meaningful on an abstract class, if it is could you move
the ctor into the protected section so the public interface is slightly less
cluttered, and if it isn't i think the ctor could probably be removed

[marja, 2012-06-20 16:54:32]

Thanks for comments!

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:605: // Record the association
between the sessionStorage namespace and the
On 2012/06/19 14:11:59, sky wrote:
> Should that be SessionStorage?

Done.

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:622:
content::SessionStorageNamespace* session_storage_namespace =
On 2012/06/19 14:11:59, sky wrote:
> If we shutdown the session service before the tabcontents will the session
> storage leak?

It's part of the normal operation that for some tabs, this code won't be
executed, and the "should persist" bool will be true. Those sessionStorages will
be persisted, and they will possibly be restored in the next startup.

When the data is saved, there will also be cleanup code to delete data which was
not used by session restore (e.g., if we crash and don't restore).

Iow, nothing should leak if session_storage_namespace->SetShouldPersist(false);
is not called for some tabs before exit. Was this what you meant?

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:622:
content::SessionStorageNamespace* session_storage_namespace =
On 2012/06/20 04:01:59, michaeln wrote:
> On 2012/06/19 14:11:59, sky wrote:
> > If we shutdown the session service before the tabcontents will the session
> > storage leak?
> 
> The question about order at shutdown is interesting. During shutdown with the
> 'continue where i left off' setting turned on, are the
> NOTIFICATION_TAB_CONTENTS_DESTROYED processed when tabs that will be restored
> upon restart are cleaned up? How does the session service ignore state changes
> it wants to ignore near the end of time? Does it skip saving SessionCommands
> generatred near the end, or does it pull the plug on the notifications such
that
> no commands are generated near the end?
> 
> The call to SetShouldPersist(false) will take effect even if a
> TabClosedSessionCommand created by TabClosed is not put in the session restore
> journal.
> 
> Hopefully you two know more about the session restore stuff than i do :) I'm
not
> so familiar with this chrome/browser code.

Afaics (empirically), when we're exiting the browser, this notification is not
sent for tabs that get closed because of the browser exit. So, those won't get
destroyed, the data will be persisted, and the session restore can work.

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:862:
pickle.WriteString(session_storage_persistent_id);
On 2012/06/19 14:11:59, sky wrote:
> Can session_storage_persistent_id be bigger than bytes available?

Persistent ID is a guid (%08X-%04X-%04X-%04X-%012llX), so shouldn't be a problem
here.

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1267: pickle->ReadInt(&iter,
&tab_id);
On 2012/06/19 14:11:59, sky wrote:
> Check return value of these two.

Done.

https://chromiumcodereview.appspot.com/10572015/diff/2001/content/browser/dom...
File content/browser/dom_storage/dom_storage_context_impl.h (right):

https://chromiumcodereview.appspot.com/10572015/diff/2001/content/browser/dom...
content/browser/dom_storage/dom_storage_context_impl.h:15: class
SessionStorageNamespace;
On 2012/06/20 04:01:59, michaeln wrote:
> since this is forward declared in
content/public/browser/dom_storage_context.h,
> which is included here, this probably doesn't need to be here... the forward
> here is needed to declare the override of the method there

Done.

https://chromiumcodereview.appspot.com/10572015/diff/2001/content/public/brow...
File content/public/browser/dom_storage_context.h (right):

https://chromiumcodereview.appspot.com/10572015/diff/2001/content/public/brow...
content/public/browser/dom_storage_context.h:35: virtual
scoped_refptr<SessionStorageNamespace> RecreateSessionStorage(
On 2012/06/20 04:01:59, michaeln wrote:
> a comment about what this method does &| its intended use would be nice

Done.

https://chromiumcodereview.appspot.com/10572015/diff/2001/content/public/brow...
File content/public/browser/session_storage_namespace.h (right):

https://chromiumcodereview.appspot.com/10572015/diff/2001/content/public/brow...
content/public/browser/session_storage_namespace.h:39:
DISALLOW_COPY_AND_ASSIGN(SessionStorageNamespace);
On 2012/06/20 04:01:59, michaeln wrote:
> i'm not sure this is meaningful on an abstract class, if it is could you move
> the ctor into the protected section so the public interface is slightly less
> cluttered, and if it isn't i think the ctor could probably be removed

Done; I added this because of an earlier code review comment, removed it again.

[jochen (gone - plz use gerrit), 2012-06-20 16:55:34]

http://codereview.chromium.org/10572015/diff/2001/content/public/browser/sess...
File content/public/browser/session_storage_namespace.h (right):

http://codereview.chromium.org/10572015/diff/2001/content/public/browser/sess...
content/public/browser/session_storage_namespace.h:39:
DISALLOW_COPY_AND_ASSIGN(SessionStorageNamespace);
The style guide says to either provide copy constructors, or disallow them. It
doesn't have an exception for abstract classes:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Copy_Constructors

I agree that the meaningfulness is debatable

[michaeln, 2012-06-20 18:09:11]

Modulo whatever is going amiss in the test, this looks pretty good to me.
 
[ RUN      ] SessionRestoreTest.SessionStorage
	0   browser_tests                       0x065577fa 0x0 + 106264570
	1   browser_tests                       0x0658e581 0x0 + 106489217

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:622:
content::SessionStorageNamespace* session_storage_namespace =
> Afaics (empirically), when we're exiting the browser, this notification is not
> sent for tabs that get closed because of the browser exit. So, those won't get
> destroyed, the data will be persisted, and the session restore can work.

Great, this works out well then.

https://chromiumcodereview.appspot.com/10572015/diff/6002/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10572015/diff/6002/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1269: return false;
The comment up top says...
// If the file is corrupt (command with wrong size, or unknown command), we
// still return true and attempt to restore what we we can.
... so should this return true too as is done in the other error conditions?

https://chromiumcodereview.appspot.com/10572015/diff/2002/chrome/browser/sess...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10572015/diff/2002/chrome/browser/sess...
chrome/browser/sessions/session_restore_browsertest.cc:776:
EXPECT_EQ(session_storate_persistent_id,
typo: 'storate' in both std::string locals

[sky, 2012-06-20 21:57:45]

LGTM

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10572015/diff/2001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:862:
pickle.WriteString(session_storage_persistent_id);
On 2012/06/20 16:54:32, marja wrote:
> On 2012/06/19 14:11:59, sky wrote:
> > Can session_storage_persistent_id be bigger than bytes available?
> 
> Persistent ID is a guid (%08X-%04X-%04X-%04X-%012llX), so shouldn't be a
problem
> here.

Please add a comment, and preferably a DCHECK.

[marja, 2012-06-22 10:37:02]

The test is fixed now, too.

https://chromiumcodereview.appspot.com/10572015/diff/6002/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/10572015/diff/6002/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1269: return false;
On 2012/06/20 18:09:11, michaeln wrote:
> The comment up top says...
> // If the file is corrupt (command with wrong size, or unknown command), we
> // still return true and attempt to restore what we we can.
> ... so should this return true too as is done in the other error conditions?

Done.

https://chromiumcodereview.appspot.com/10572015/diff/2002/chrome/browser/sess...
File chrome/browser/sessions/session_restore_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10572015/diff/2002/chrome/browser/sess...
chrome/browser/sessions/session_restore_browsertest.cc:776:
EXPECT_EQ(session_storate_persistent_id,
On 2012/06/20 18:09:11, michaeln wrote:
> typo: 'storate' in both std::string locals

Done.

[michaeln, 2012-06-22 18:00:07]

LGTM2

[commit-bot: I haz the power, 2012-06-22 19:13:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/10572015/20001

[commit-bot: I haz the power, 2012-06-22 19:13:58]

Presubmit check for 10572015-20001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit
    content/public/browser

Presubmit checks took 1.6s to calculate.

[marja, 2012-06-22 19:23:29]

Oops, owners.

brettw, could you review the following:
content/public/browser/*
webkit/tools/test_shell/simple_dom_storage_system.cc

[brettw, 2012-06-22 19:34:54]

content/public and webkit/tools LGTM

[commit-bot: I haz the power, 2012-06-22 19:44:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/10572015/20001

[commit-bot: I haz the power, 2012-06-22 21:20:56]

Change committed as 143709