Adding session-only localStorage.

Adding session-only LocalStorage. Data can now be stored in the
permanent LocalStorage or in the session-only LocalStorage, depending
on the content settings for the origin.

Draft -- missing the usage of first party origin when checking the "enforce session only" setting

BUG=NONE
TEST=DOMStorageBrowserTest.ChangingSettings, DOMStorageBrowserTest.SessionOnly

[michaeln, 2011-07-27 18:53:22]

Oh boy... you're really going down this path of per-host session-only storage
model. I think this could prove greatly complicate things, so i'm somewhat wary
of this endeavor.

Probably a good choice to start with LocalStorage since it's conceptually less
complicated than some of the others, and supports an in-memory-only backing
store, and is also not integrated with the unified quota system.

[jochen (gone - plz use gerrit), 2011-07-29 08:01:05]

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_context.cc:284: return
resource_context_;
you can inline such small getters

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_namespace.cc (right):

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_namespace.cc:21: namespace {
why not make these two functions method of DOMStorageNamespace?

DOMStorageNamespace would need to be ref counted then, but that's better anyway,
so you can guarantee it doesn't go away while you post tasks

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_namespace.h (right):

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_namespace.h:31: class Delegate {
instead of using this delegate, you should use base/callback.h

[michaeln, 2011-07-30 16:49:24]

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_message_filter.cc:117: void
DOMStorageMessageFilter::OverrideThreadForMessage(
This additional thread context switching (from IO to WEBKIT, ,back to IO, back
to WEBKIT, then back to IO) is kind of going in circles. The point of the
OverrideThreadForMessage stuff was to simplify and streamline handling of
messages on other threads. But now that the DOMStorageHostMsg_StorageAreaId does
require processing on the IO thread, it probably would be cleaner and more
straightforward to not override the thread for that particular message.

void DOMStorageMessageFilter::OnStorageAreaId(int64 namespace_id,
                                              const string16& origin,
                                              IPC::Message* reply_msg) {
  // Initiate handling on the IO thread to get the content setting value.
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
  bool content_setting_value = // get setting for this origin
  // Do the bulk of the work on the webkit thread.
  PostTask(OnStorageAreaIdWebKitThread,
           namespace_id, origin, content_setting_value , reply_msg);
}

void DOMStorageMessageFilter::OnStorageAreaIdWebKitThread(
                                              int64 namespace_id,
                                              const string16& origin,
                                              bool content_setting_value ,
                                              IPC::Message* reply_msg) {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));
  DOMStorageNamespace* storage_namespace =
      Context()->GetStorageNamespace(namespace_id, content_setting_value ,
true);
  ...
  DOMStorageHostMsg_StorageAreaId::WriteReplyParams(reply_msg, area_id);
  Send(reply_msg);
}

wdyt?

[marja, 2011-08-01 08:36:32]

Marking some of the smaller changes done.

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_context.cc:284: return
resource_context_;
On 2011/07/29 08:01:06, jochen wrote:
> you can inline such small getters

Done.

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_namespace.cc (right):

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_namespace.cc:21: namespace {
On 2011/07/29 08:01:06, jochen wrote:
> why not make these two functions method of DOMStorageNamespace?
> 
> DOMStorageNamespace would need to be ref counted then, but that's better
anyway,
> so you can guarantee it doesn't go away while you post tasks

Done.

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_namespace.h (right):

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_namespace.h:31: class Delegate {
On 2011/07/29 08:01:06, jochen wrote:
> instead of using this delegate, you should use base/callback.h

Done.

(This will change again, though, after I've done the modifications michaeln
suggested.)

[marja, 2011-08-01 09:41:27]

More changes "Done".

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/7480041/diff/4001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_message_filter.cc:117: void
DOMStorageMessageFilter::OverrideThreadForMessage(
On 2011/07/30 16:49:24, michaeln wrote:
> This additional thread context switching (from IO to WEBKIT, ,back to IO, back
> to WEBKIT, then back to IO) is kind of going in circles. The point of the
> OverrideThreadForMessage stuff was to simplify and streamline handling of
> messages on other threads. But now that the DOMStorageHostMsg_StorageAreaId
does
> require processing on the IO thread, it probably would be cleaner and more
> straightforward to not override the thread for that particular message.
> 
> void DOMStorageMessageFilter::OnStorageAreaId(int64 namespace_id,
>                                               const string16& origin,
>                                               IPC::Message* reply_msg) {
>   // Initiate handling on the IO thread to get the content setting value.
>   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
>   bool content_setting_value = // get setting for this origin
>   // Do the bulk of the work on the webkit thread.
>   PostTask(OnStorageAreaIdWebKitThread,
>            namespace_id, origin, content_setting_value , reply_msg);
> }
> 
> void DOMStorageMessageFilter::OnStorageAreaIdWebKitThread(
>                                               int64 namespace_id,
>                                               const string16& origin,
>                                               bool content_setting_value ,
>                                               IPC::Message* reply_msg) {
>   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));
>   DOMStorageNamespace* storage_namespace =
>       Context()->GetStorageNamespace(namespace_id, content_setting_value ,
> true);
>   ...
>   DOMStorageHostMsg_StorageAreaId::WriteReplyParams(reply_msg, area_id);
>   Send(reply_msg);
> }
> 
> wdyt?

Done.

Thanks for the tip, this made the code a lot simpler.

[michaeln, 2011-08-02 19:47:41]

http://codereview.chromium.org/7480041/diff/9001/content/browser/mock_resourc...
File content/browser/mock_resource_context.cc (left):

http://codereview.chromium.org/7480041/diff/9001/content/browser/mock_resourc...
content/browser/mock_resource_context.cc:21:
set_request_context(test_request_context_);
why does the setter not work anymore?

http://codereview.chromium.org/7480041/diff/16001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7480041/diff/16001/content/browser/content_bro...
content/browser/content_browser_client.h:151: virtual bool
AllowPermanentStorage(
I'm a little worried about naming, this usage of "permanent" is easily confused
with our different usage of "persistent".

I don't know if i this would be a good idea, but maybe instead of expressing
this in terms of "allowing" the usual thing (which is permanent storage), phrase
this in terms of forcing the unusual thing (which is session-only-ness).

EnforceSessionOnlyStorage(...);

So cookies, localstorage, etc are allowed (per other methods here)... but then
forced to be session-only.

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_message_filter.cc:108: ;
this looks odd, maybe put an early return here instead and then there's no need
for the addition of the else clause

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_message_filter.cc:123: //
FIXME(marja): Get the first party URL
nit: TODO(marja) instead of FIXME. One of those style differences between
working in chromium vs webkit repositories.

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_namespace.cc (right):

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_namespace.cc:47:
session_only_local_storage_.reset(
can this be more lazily created, i imagine most users won't have this related
setting turned on

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_namespace.cc:76: // the
DOMStorageArea was created.)
This blurs the lines between the sessionOnly and longerLived worlds quite a bit.
With the sessionOnly setting on, sometimes you get the longLived namespace, and
vice versa. Is that really needed? What would happen if this was more clear
cut...

if (enforce_session_only)
  return session_only_thingy;
else
  return long_lived_thingy;

[marja, 2011-08-03 07:09:51]

http://codereview.chromium.org/7480041/diff/9001/content/browser/mock_resourc...
File content/browser/mock_resource_context.cc (left):

http://codereview.chromium.org/7480041/diff/9001/content/browser/mock_resourc...
content/browser/mock_resource_context.cc:21:
set_request_context(test_request_context_);
On 2011/08/02 19:47:41, michaeln wrote:
> why does the setter not work anymore?

ResourceContext::ResourceContext DCHECKs we're on the UI thread, and
ResourceContext::set_request_context DCHECKs we're on the IO thread. This
function calls them both. It would work if every test did the "create the UI
thread and the IO thread with the same message loop" thing. After my changes,
this MockResourceContext gets created more often than before (because
WebKitContext is given a ResourceContext). So I thought it would be nicer to
modify this MockResourceContext to be more useful in a wider range of tests
(also those which don't do the "create the UI thread and the IO thread with the
same message loop" thing).

[marja, 2011-08-03 09:14:15]

Thanks for the comments!

http://codereview.chromium.org/7480041/diff/16001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7480041/diff/16001/content/browser/content_bro...
content/browser/content_browser_client.h:151: virtual bool
AllowPermanentStorage(
On 2011/08/02 19:47:41, michaeln wrote:
> I'm a little worried about naming, this usage of "permanent" is easily
confused
> with our different usage of "persistent".
> 
> I don't know if i this would be a good idea, but maybe instead of expressing
> this in terms of "allowing" the usual thing (which is permanent storage),
phrase
> this in terms of forcing the unusual thing (which is session-only-ness).
> 
> EnforceSessionOnlyStorage(...);
> 
> So cookies, localstorage, etc are allowed (per other methods here)... but then
> forced to be session-only.

Done.

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_message_filter.cc:108: ;
On 2011/08/02 19:47:41, michaeln wrote:
> this looks odd, maybe put an early return here instead and then there's no
need
> for the addition of the else clause

Done.

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_message_filter.cc:123: //
FIXME(marja): Get the first party URL
On 2011/08/02 19:47:41, michaeln wrote:
> nit: TODO(marja) instead of FIXME. One of those style differences between
> working in chromium vs webkit repositories.

Done.

Though, my intention was to get this FIXME fixed before committing the cl. :)

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_namespace.cc (right):

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_namespace.cc:47:
session_only_local_storage_.reset(
On 2011/08/02 19:47:41, michaeln wrote:
> can this be more lazily created, i imagine most users won't have this related
> setting turned on

Done.

http://codereview.chromium.org/7480041/diff/16001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_namespace.cc:76: // the
DOMStorageArea was created.)
On 2011/08/02 19:47:41, michaeln wrote:
> This blurs the lines between the sessionOnly and longerLived worlds quite a
bit.
> With the sessionOnly setting on, sometimes you get the longLived namespace,
and
> vice versa. Is that really needed? What would happen if this was more clear
> cut...
> 
> if (enforce_session_only)
>   return session_only_thingy;
> else
>   return long_lived_thingy;

I guess that would confuse some web pages, if the user alters the setting when
the page is open. E.g., first the page puts something to the localStorage, and
it goes to the permanent storage. Then the user alters the setting so that only
session-only storage is allowed (for this origin). Then a different frame on the
page tries to read back from the localStorage, but it can no longer find the
data, since the data is in the permanent storage, but that piece of code would
direct it to the session-only storage. Is this too far-fetched? :) If the page
just reads data from the localStorage 2 times, GetStorageArea is called only
once. So everything would work unless the page has a construct which gets
GetStorageArea called multiple times (at least frames, not sure when else it
would be), or if 2 open pages use the same localStorage.

In the current version of the cl, real session-only-ness is only achieved if the
user clears the storage data after restricting some origins to session only
storage, and restarts the browser. The same after modifying the setting the
other way around (session only -> permanent). If a page has already put data
into the localStorage, and the user changes the content setting, the data can
still be retrieved by all users of the same localStorage. In your version it can
only be retrieved by those users of the that localStorage who have already got
the storage area id earlier. Not sure which is better.

[michaeln, 2011-08-04 20:21:51]

The code looks pretty good, but really i do wonder about the semantics involved
wrt blurring the lines.

It's unclear which namespace will be used upon switching the setting. That might
lead to unexpected results. Suppose you switch it on (sessionOnly). Then a page
writes a bunch of new stuff to localStorage, depending it may go into the real
storage on disk and not be deleted on session end. Somebody would probably
consider that a bug.

Conversely, suppose you switch if off. Subsequent writes may  go to the bit
bucket on session end. Ditto bug.

I'm wondering if a different approach to this feature set has been considered?
Write it all to disk as usual, clean up on session end per the current settings
and that time. That approach would probably be easier to take for fileSystem,
appcache, sqlDatabase, indexedDatabase systems too.

Also, we could add a 'SpecialStoragePolicy' method to determine whether
'sessionOnlyNess' applies for an origin. Is the backing for for those setting
accessible in a thread-safe way?

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_context.h:112: const
content::ResourceContext& GetResourceContext() const {
would prefer unix hacker style for this simple getter...
   resource_context()
... but see comment elsewhere about maybe not needing this data member in this
class

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_message_filter.cc:125:
Context()->GetResourceContext());
I think this is the only place you actually need access to the ResourceContext
instance. if that's right, instead of adding the new data member to
DOMStorageContext consider adding it to the DOMStorageMessageFilter class.

The reasons being:

* Thread safety. The ResourceContext class can only be used on the IO thread but
the DOMStorage stuff is almost exclusively used on the WEBKIT thread. Having
that data member there is a little landmine'ish.

* I think you could shrink the CL a little, less plumbing of that value around
in ctors.

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_namespace.cc (right):

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_namespace.cc:143: return true;
maybe simplify as
return (type == LOCAL) && !path.isEmpty();

[marja, 2011-08-05 13:15:53]

Thanks for the comments, the ResourceContext& change indeed streamlined the cl.
Marking small changes as "Done".

{mirandac,sky}: Afaics, this cl no longer touches the parts which would've
needed your reviews for the OWNERS check, so I'll remove you from the reviewer
list. sky, you're an OWNER of content/browser (but so is darin), so feel free to
add yourself back if you have time to review this.

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_context.h:112: const
content::ResourceContext& GetResourceContext() const {
On 2011/08/04 20:21:51, michaeln wrote:
> would prefer unix hacker style for this simple getter...
>    resource_context()
> ... but see comment elsewhere about maybe not needing this data member in this
> class

Done. Or rather, "Gone".

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_message_filter.cc:125:
Context()->GetResourceContext());
On 2011/08/04 20:21:51, michaeln wrote:
> I think this is the only place you actually need access to the ResourceContext
> instance. if that's right, instead of adding the new data member to
> DOMStorageContext consider adding it to the DOMStorageMessageFilter class.
> 
> The reasons being:
> 
> * Thread safety. The ResourceContext class can only be used on the IO thread
but
> the DOMStorage stuff is almost exclusively used on the WEBKIT thread. Having
> that data member there is a little landmine'ish.
> 
> * I think you could shrink the CL a little, less plumbing of that value around
> in ctors.

Done.

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
File content/browser/in_process_webkit/dom_storage_namespace.cc (right):

http://codereview.chromium.org/7480041/diff/25001/content/browser/in_process_...
content/browser/in_process_webkit/dom_storage_namespace.cc:143: return true;
On 2011/08/04 20:21:51, michaeln wrote:
> maybe simplify as
> return (type == LOCAL) && !path.isEmpty();

Done.