Fix SessionStorage

Fix SessionStorage

Apparently the session storage code was pretty horribly broken.  It didn't correctly handle tabs being restored, didn't have the proper lifetime (this was the issue exposed in the bug), and had many leaks.

To fix this, things had to be plumbed fairly differently.  We need to pass session storage in on TabContents creation to ensure that the first RenderView will have the correct session storage id.  When closing a tab, we need to save the session storage with the restoration service.  When restoring a tab, we pass it back into the tab contents class.  When duplicating a tab, we clone the storage.

Lifetimes are now handled by standard reference counting code.  A SessionStorageNamespace object wraps an ID.  When it's instantiated, it allocates an ID.  When it's destroyed, it deletes the ID.  IDs make this process very lightweight (the heavyweight stuff is allocated on first use of SessionStorage) and it seperates the more complex lifetimes of SessionStorage namespaces (where less deterministic shutdown is more tollerable) from the LocalStorage namespace which needs to shutdown very precisely.

BUG=52393
TEST=Set some variable on session storage; close the tab; re-open the tab; the variable should still be set.  You can also run through the repro steps in the bug.

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

[jorlow, 2010-09-03 18:46:24]

This compiles and works.  I'm going to investigate ways to test it (suggestions
very welcome!) and get it working on Windows/Linux on Monday.

But given the fact that this is a top crasher and will likely require a few
iterations, it'd be great if you guys could start looking at it now.

Brett/Darin: you're reviewers because I don't know who else knows enough about
this stuff.  Feel free to delegate.

[jorlow, 2010-09-03 23:21:25]

It'd be really cool if someone could at least do a first pass before you all
head off for the long weekend.  :-)

[jorlow, 2010-09-06 12:51:53]

Joth, maybe you can take a first pass at this?

[brettw, 2010-09-07 17:23:36]

http://codereview.chromium.org/3325012/diff/12001/11023
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/3325012/diff/12001/11023#newcode102
chrome/browser/renderer_host/render_view_host.h:102: // which case
RenderWidgetHost will create a new one.
Can you add a comment here what session storage namespace should be and whether
it can be NULL? (like for testing).

http://codereview.chromium.org/3325012/diff/12001/11041
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/3325012/diff/12001/11041#newcode122
chrome/browser/tab_contents/tab_contents.h:122: // based on an existing tab
contents view.  This can be NULL if not needed.
Can you also comment here about the session storage namespace and whether it can
be NULL? You added this in all the big classes, and people are often using these
for other purposes or testing. Adding extra parameters makes it harder to figure
out how to do this, so I want to be sure all the commonly-used classes are super
clear on how to use them on their own.

I notice you pass NULL in a bunch of places for tests, but it's not clear
looking at some of the usage (like in NavigationController) what a caller's
expectation of null-ness should be. Can you make it super clear in all the
important cases what can be NULL when and what this means?

[jorlow, 2010-09-08 18:34:35]

Done and done.  Plus I think I fixed the unit test failures.  Will re-upload in
a moment.

http://codereview.chromium.org/3325012/diff/12001/11023
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/3325012/diff/12001/11023#newcode102
chrome/browser/renderer_host/render_view_host.h:102: // which case
RenderWidgetHost will create a new one.
On 2010/09/07 17:23:37, brettw wrote:
> Can you add a comment here what session storage namespace should be and
whether
> it can be NULL? (like for testing).

Done.

http://codereview.chromium.org/3325012/diff/12001/11041
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/3325012/diff/12001/11041#newcode122
chrome/browser/tab_contents/tab_contents.h:122: // based on an existing tab
contents view.  This can be NULL if not needed.
On 2010/09/07 17:23:37, brettw wrote:
> Can you also comment here about the session storage namespace and whether it
can
> be NULL? You added this in all the big classes, and people are often using
these
> for other purposes or testing. Adding extra parameters makes it harder to
figure
> out how to do this, so I want to be sure all the commonly-used classes are
super
> clear on how to use them on their own.
> 
> I notice you pass NULL in a bunch of places for tests, but it's not clear
> looking at some of the usage (like in NavigationController) what a caller's
> expectation of null-ness should be. Can you make it super clear in all the
> important cases what can be NULL when and what this means?

Done.

[jorlow, 2010-09-09 15:07:45]

Reminder: this is apparently a top crasher...

[brettw, 2010-09-12 17:05:11]

LGTM with a few minor comments.

I was waiting for Darin to look at this since he has more background on this
than I do. But I guess he's super busy these days. Sorry for the delay.

One thing I'm concerned about: what about when tabs are restored after you exit
and start back up? I assume they should have the same namespace? Does this work
today? It seems like not from my understanding of this, but I could be wrong. If
that's broken, we can solve it in another patch.

http://codereview.chromium.org/3325012/diff/13003/19016
File chrome/browser/in_process_webkit/session_storage_namespace.h (right):

http://codereview.chromium.org/3325012/diff/13003/19016#newcode26
chrome/browser/in_process_webkit/session_storage_namespace.h:26:
SessionStorageNamespace* Clone();
Can you provide a quick comment for this one? Like that it will allocate a new
object and that the refcount will be 0.

http://codereview.chromium.org/3325012/diff/13003/19034
File chrome/browser/tab_contents/navigation_controller.h (right):

http://codereview.chromium.org/3325012/diff/13003/19034#newcode141
chrome/browser/tab_contents/navigation_controller.h:141:
NavigationController(TabContents* tab_contents,
Can you copy the same comment about the namespace from the TabContents
constructor to here?

[jorlow, 2010-09-13 11:30:10]

On 2010/09/12 17:05:11, brettw wrote:
> LGTM with a few minor comments.
> 
> I was waiting for Darin to look at this since he has more background on this
> than I do. But I guess he's super busy these days. Sorry for the delay.
> 
> One thing I'm concerned about: what about when tabs are restored after you
exit
> and start back up? I assume they should have the same namespace? Does this
work
> today? It seems like not from my understanding of this, but I could be wrong.
If
> that's broken, we can solve it in another patch.

Last time Darin and I talked (half a year ago), the thinking was that we should
not write this information to disk, and thus we shouldn't support the restore
after exit use case.  I forget all the details of what went into that decision,
but I know this is what Safari does and I believe it's what firefox does.  There
definitely are sites out there that make this assumption.  And I'm guessing this
is what we do with stuff like session cookies as well?
 
> http://codereview.chromium.org/3325012/diff/13003/19016
> File chrome/browser/in_process_webkit/session_storage_namespace.h (right):
> 
> http://codereview.chromium.org/3325012/diff/13003/19016#newcode26
> chrome/browser/in_process_webkit/session_storage_namespace.h:26:
> SessionStorageNamespace* Clone();
> Can you provide a quick comment for this one? Like that it will allocate a new
> object and that the refcount will be 0.

Good idea.  Done.

> http://codereview.chromium.org/3325012/diff/13003/19034
> File chrome/browser/tab_contents/navigation_controller.h (right):
> 
> http://codereview.chromium.org/3325012/diff/13003/19034#newcode141
> chrome/browser/tab_contents/navigation_controller.h:141:
> NavigationController(TabContents* tab_contents,
> Can you copy the same comment about the namespace from the TabContents
> constructor to here?

Done.

[brettw, 2010-09-13 15:37:45]

On Mon, Sep 13, 2010 at 4:30 AM,  <jorlow@chromium.org> wrote:
> On 2010/09/12 17:05:11, brettw wrote:
>>
>> LGTM with a few minor comments.
>
>> I was waiting for Darin to look at this since he has more background on
>> this
>> than I do. But I guess he's super busy these days. Sorry for the delay.
>
>> One thing I'm concerned about: what about when tabs are restored after you
>
> exit
>>
>> and start back up? I assume they should have the same namespace? Does this
>
> work
>>
>> today? It seems like not from my understanding of this, but I could be
>> wrong.
>
> If
>>
>> that's broken, we can solve it in another patch.
>
> Last time Darin and I talked (half a year ago), the thinking was that we
> should
> not write this information to disk, and thus we shouldn't support the
> restore
> after exit use case.  I forget all the details of what went into that
> decision,
> but I know this is what Safari does and I believe it's what firefox does.
>  There
> definitely are sites out there that make this assumption.  And I'm guessing
> this
> is what we do with stuff like session cookies as well?

Yes, that's fine then.

Brett