Fix SessionStorage confusion between RenderViewHostImpl and NavigationController.

Fix SessionStorage confusion between RenderViewHostImpl and NavigationController.

Both RenderViewHostImpl and NavigationController had SessionStorageNamespace*,
and under some circumstances, they pointed to different
SessionStorageNamespaces* which is wrong.

This also cleans up some storage partition code, since we're not going to have
multiple SessionStorageNamespaces.

For more information, see bug.

BUG=138152
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212218

[marja, 2013-07-15 13:48:44]

jochen, ajwong, ptal.

jochen: everything
ajwong: pls review that the storage partition cleanup is done correctly

(I'll ask for OWNERS reviews later if needed.)

[jochen (gone - plz use gerrit), 2013-07-15 14:07:06]

lgtm

https://codereview.chromium.org/18500005/diff/25001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/18500005/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:415: virtual
SessionStorageNamespace* GetSessionStorageNamespace();
Please add a comment what this method is supposed to do

https://codereview.chromium.org/18500005/diff/25001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/18500005/diff/25001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1363: // Create one
if no one has accessed session storage yet.
that comment is a bit superfluous

[marja, 2013-07-15 14:16:45]

thx for review

https://codereview.chromium.org/18500005/diff/25001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/18500005/diff/25001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:415: virtual
SessionStorageNamespace* GetSessionStorageNamespace();
On 2013/07/15 14:07:06, jochen wrote:
> Please add a comment what this method is supposed to do

Done.

https://codereview.chromium.org/18500005/diff/25001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/18500005/diff/25001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1363: // Create one
if no one has accessed session storage yet.
On 2013/07/15 14:07:06, jochen wrote:
> that comment is a bit superfluous

Done.

[marja, 2013-07-15 14:17:20]

(michaeln, fyi)

[awong, 2013-07-16 18:56:01]

LGTM w/ 1 nit + 2 questions about the status quo implementation.

https://codereview.chromium.org/18500005/diff/56001/chrome/browser/sessions/s...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/18500005/diff/56001/chrome/browser/sessions/s...
chrome/browser/sessions/session_service.cc:1742: // isolated apps which won't
have a default partition.
nite: Remove the TODO.

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1217: if
(!source.session_storage_namespace_.get())
Is it actually possible for the source.session_storage_namespace_ to be NULL?

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1344: if
(!session_storage_namespace)
Does this mean we can never "unset" a session storage namespace?  I'm wondering
when this particular check would get invoked...

[marja, 2013-07-17 09:33:28]

thanks for review!

https://codereview.chromium.org/18500005/diff/56001/chrome/browser/sessions/s...
File chrome/browser/sessions/session_service.cc (right):

https://codereview.chromium.org/18500005/diff/56001/chrome/browser/sessions/s...
chrome/browser/sessions/session_service.cc:1742: // isolated apps which won't
have a default partition.
On 2013/07/16 18:56:01, awong wrote:
> nite: Remove the TODO.

Done.

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1217: if
(!source.session_storage_namespace_.get())
On 2013/07/16 18:56:01, awong wrote:
> Is it actually possible for the source.session_storage_namespace_ to be NULL?

It'll be NULL at least in tests.

And, even outside tests, IMO this makes sense from the copying semantics point
of view, and matches the semantics of the previous implementation (the map
could've been empty).

However, I could be easily be convinced that we should write this code with the
assumption that it'll never be NULL at this point, and mock in the tests. Atm
I'm not sure if that's a better solution than this.

(If somebody feels strongly about this, I'll do a follow-up change.)

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1344: if
(!session_storage_namespace)
On 2013/07/16 18:56:01, awong wrote:
> Does this mean we can never "unset" a session storage namespace?  I'm
wondering
> when this particular check would get invoked...

The CHECK below (which is not a DCHECK but a CHECK) guarantees that there's
nothing in the session_storage_namespace_ when this function is called, so
"unsetting" is not possible even without this check.

[marja, 2013-07-17 09:38:09]

joi: I'm changing some content/public interfaces here, could you review? All the
changes are because of the "can't have multiple session storage namespaces"
cleanup (undoing a partially done feature which would have enabled us to have
multiple of them).

[awong, 2013-07-17 09:44:02]

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1217: if
(!source.session_storage_namespace_.get())
On 2013/07/17 09:33:29, marja wrote:
> On 2013/07/16 18:56:01, awong wrote:
> > Is it actually possible for the source.session_storage_namespace_ to be
NULL?
> 
> It'll be NULL at least in tests.
> 
> And, even outside tests, IMO this makes sense from the copying semantics point
> of view, and matches the semantics of the previous implementation (the map
> could've been empty).
> 
> However, I could be easily be convinced that we should write this code with
the
> assumption that it'll never be NULL at this point, and mock in the tests. Atm
> I'm not sure if that's a better solution than this.
> 
> (If somebody feels strongly about this, I'll do a follow-up change.)

By default, I bias towards not accepting NULL since less permissive APIs are
also less likely to mask a real logic error.

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1344: if
(!session_storage_namespace)
On 2013/07/17 09:33:29, marja wrote:
> On 2013/07/16 18:56:01, awong wrote:
> > Does this mean we can never "unset" a session storage namespace?  I'm
> wondering
> > when this particular check would get invoked...
> 
> The CHECK below (which is not a DCHECK but a CHECK) guarantees that there's
> nothing in the session_storage_namespace_ when this function is called, so
> "unsetting" is not possible even without this check.

I see what you mean by the CHECK, but more than the previous code, it seems odd
here that we just happily accept SetSessionStorageNamespace(NULL) as meaning "do
nothing."

I can totally believe the legacy code path makes this impossible, but in a
vacuum, I would have expected that to CHECK fail or crash.

[awong, 2013-07-17 10:02:53]

BTW, whatever you decide with respect to NULLs still has my LGTM.


On Wed, Jul 17, 2013 at 2:44 AM, <ajwong@chromium.org> wrote:

>
> https://codereview.chromium.**org/18500005/diff/56001/**
>
content/browser/web_contents/**navigation_controller_impl.cc<https://codereview.chromium.org/18500005/diff/56001/content/browser/web_contents/navigation_controller_impl.cc>
> File content/browser/web_contents/**navigation_controller_impl.cc (right):
>
> https://codereview.chromium.**org/18500005/diff/56001/**
>
content/browser/web_contents/**navigation_controller_impl.cc#**newcode1217<https://codereview.chromium.org/18500005/diff/56001/content/browser/web_contents/navigation_controller_impl.cc#newcode1217>
> content/browser/web_contents/**navigation_controller_impl.cc:**1217: if
> (!source.session_storage_**namespace_.get())
> On 2013/07/17 09:33:29, marja wrote:
>
>> On 2013/07/16 18:56:01, awong wrote:
>> > Is it actually possible for the source.session_storage_**namespace_ to
>>
> be NULL?
>
>  It'll be NULL at least in tests.
>>
>
>  And, even outside tests, IMO this makes sense from the copying
>>
> semantics point
>
>> of view, and matches the semantics of the previous implementation (the
>>
> map
>
>> could've been empty).
>>
>
>  However, I could be easily be convinced that we should write this code
>>
> with the
>
>> assumption that it'll never be NULL at this point, and mock in the
>>
> tests. Atm
>
>> I'm not sure if that's a better solution than this.
>>
>
>  (If somebody feels strongly about this, I'll do a follow-up change.)
>>
>
> By default, I bias towards not accepting NULL since less permissive APIs
> are also less likely to mask a real logic error.
>
> https://codereview.chromium.**org/18500005/diff/56001/**
>
content/browser/web_contents/**navigation_controller_impl.cc#**newcode1344<https://codereview.chromium.org/18500005/diff/56001/content/browser/web_contents/navigation_controller_impl.cc#newcode1344>
> content/browser/web_contents/**navigation_controller_impl.cc:**1344: if
> (!session_storage_namespace)
> On 2013/07/17 09:33:29, marja wrote:
>
>> On 2013/07/16 18:56:01, awong wrote:
>> > Does this mean we can never "unset" a session storage namespace?
>>
> I'm
>
>> wondering
>> > when this particular check would get invoked...
>>
>
>  The CHECK below (which is not a DCHECK but a CHECK) guarantees that
>>
> there's
>
>> nothing in the session_storage_namespace_ when this function is
>>
> called, so
>
>> "unsetting" is not possible even without this check.
>>
>
> I see what you mean by the CHECK, but more than the previous code, it
> seems odd here that we just happily accept
> SetSessionStorageNamespace(**NULL) as meaning "do nothing."
>
> I can totally believe the legacy code path makes this impossible, but in
> a vacuum, I would have expected that to CHECK fail or crash.
>
>
https://codereview.chromium.**org/18500005/<https://codereview.chromium.org/1...
>

[marja, 2013-07-17 11:02:59]

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1217: if
(!source.session_storage_namespace_.get())
On 2013/07/17 09:44:08, awong wrote:
> On 2013/07/17 09:33:29, marja wrote:
> > On 2013/07/16 18:56:01, awong wrote:
> > > Is it actually possible for the source.session_storage_namespace_ to be
> NULL?
> > 
> > It'll be NULL at least in tests.
> > 
> > And, even outside tests, IMO this makes sense from the copying semantics
point
> > of view, and matches the semantics of the previous implementation (the map
> > could've been empty).
> > 
> > However, I could be easily be convinced that we should write this code with
> the
> > assumption that it'll never be NULL at this point, and mock in the tests.
Atm
> > I'm not sure if that's a better solution than this.
> > 
> > (If somebody feels strongly about this, I'll do a follow-up change.)
> 
> By default, I bias towards not accepting NULL since less permissive APIs are
> also less likely to mask a real logic error.

Actually, it's not that difficult to mock this in the tests. The
SessionStorageNamespace is created on demand (by GetSessionStorageNamespace), so
I just called that in the unit test before starting the test. In production
code, it's created when a RenderView is created.

Note: I'm making the API less permissive here; the previous code didn't care
here if the map is empty, but now we require a SessionStorageNamespace.

Note 2: The most elegant solution would be to use
source.GetSessionStorageNamespace() here, but I can't, because it's not const.
Grr, const correctness and "create on demand" don't play along nicely.

https://codereview.chromium.org/18500005/diff/56001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1344: if
(!session_storage_namespace)
On 2013/07/17 09:44:08, awong wrote:
> On 2013/07/17 09:33:29, marja wrote:
> > On 2013/07/16 18:56:01, awong wrote:
> > > Does this mean we can never "unset" a session storage namespace?  I'm
> > wondering
> > > when this particular check would get invoked...
> > 
> > The CHECK below (which is not a DCHECK but a CHECK) guarantees that there's
> > nothing in the session_storage_namespace_ when this function is called, so
> > "unsetting" is not possible even without this check.
> 
> I see what you mean by the CHECK, but more than the previous code, it seems
odd
> here that we just happily accept SetSessionStorageNamespace(NULL) as meaning
"do
> nothing."
> 
> I can totally believe the legacy code path makes this impossible, but in a
> vacuum, I would have expected that to CHECK fail or crash.

Offline discussion: that is orthogonal to this change, so not doing anything
about it in this CL.

[Jói, 2013-07-17 13:42:08]

//content/public LGTM.

[commit-bot: I haz the power, 2013-07-17 13:45:00]

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

[commit-bot: I haz the power, 2013-07-17 14:02:22]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[marja, 2013-07-17 14:16:46]

Ahh, I still need OWNERS since OWNERS of src is no longer *.

sky, could you review content/test/*?

[sky, 2013-07-17 15:56:55]

content/test LGTM

[commit-bot: I haz the power, 2013-07-17 20:39:06]

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

[commit-bot: I haz the power, 2013-07-18 02:43:01]

Change committed as 212218

[awong, 2013-07-22 22:35:32]

https://codereview.chromium.org/18500005/diff/87001/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl.cc (right):

https://codereview.chromium.org/18500005/diff/87001/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl.cc:1367:
->GetDOMStorageContext()));
Ah Hah! I think this is the bug.  We should still take in the SiteInstance.

https://codereview.chromium.org/18500005/diff/87001/content/browser/web_conte...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/18500005/diff/87001/content/browser/web_conte...
content/browser/web_contents/render_view_host_manager.cc:86: routing_id,
main_frame_routing_id, false));
Here we still need GetSessionStorageNamespace for a given site_instance