Bug 65611 – Add firstPartyOrigin parameter to WebStorageNamespace::createStorageArea

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit Misc.

[Marja Hölttä]

In Chrome, we'd like to grant scripts the right to use permanent or session-only localStorage based on their origin and the first-party origin of the page.

For this, we'd need to modify the signature of WebStorageNamespace::createStorageArea():

Now:
virtual WebStorageArea* createStorageArea(const WebString& origin) = 0;

We'd like to have:
virtual WebStorageArea* createStorageArea(const WebString& origin, const WebString& firstPartyOrigin) = 0;

I'd like to carry out the modifications in the following order:

1) Modifications in WebKit
- Add the two-parameter version of WebStorageNamespace::createStorageArea, provide an implementation (in WebStoragenamespaceImpl.cpp) which just delegates to the one-parameter version.
- Provide an implementation to the one-parameter version of WebStorageNamespace::createStorageArea which just delegates to the two-parameter version. This way it's enough if a subclass implements either the one-parameter version or the two-parameter version.
- Add the second parameter to StorageNamespace::storageArea(), StorageNamespaceImpl::storageArea(), StorageNamespaceProxy::storageArea() and WebStorageNamespaceImpl::createStorageArea().
- Modify DOMWindow to use the two-parameter version of storageArea().

2) Modifications in Chromium
- Modify the RendererWebStorageNamespaceImpl (inherits WebStorageNamespace) to overwrite the two-parameter version of createStorageArea instead of the one-parameter version.

3) Modifications in WebKit
- Remove the one-parameter version of WebStorageNamespace::createStorageArea, make the two-parameter version pure virtual and remove the implementation.

[Marja Hölttä]

Created attachment 102770 [details]
Proposed patch for the first round of WebKit modifications

[Marja Hölttä]

Created attachment 102773 [details]
Proposed patch for the first round of WebKit modifications

[Marja Hölttä]

Created attachment 102779 [details]
Patch

[WebKit Review Bot]

Attachment 102779 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marja Hölttä]

Created attachment 102781 [details]
Patch

[Darin Adler]

Comment on attachment 102781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102781&action=review

> Source/WebCore/storage/StorageNamespaceImpl.h:49
> -        virtual PassRefPtr<StorageArea> storageArea(PassRefPtr<SecurityOrigin>);
> +        virtual PassRefPtr<StorageArea> storageArea(PassRefPtr<SecurityOrigin> prpOrigin, PassRefPtr<SecurityOrigin> prpFirstPartyOrigin);

The names here should not have a prp prefix.

Comment on attachment 102781 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102781&action=review

As the patch touches WebKit/chromium, you'll need to add [e-mail] as reviewer

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

this line should come first in the changelog entry

> Source/WebKit/chromium/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

same here

[Marja Hölttä]

Created attachment 102880 [details]
Patch

[Marja Hölttä]

Created attachment 102881 [details]
Patch

[Marja Hölttä]

Darin Fischer: could you review this patch, please? It modifies WebKit/chromium.

[Darin Fisher (:fishd, Google)]

Comment on attachment 102881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102881&action=review

> Source/WebKit/chromium/public/WebStorageNamespace.h:71
> +    virtual WebStorageArea* createStorageArea(const WebString& origin, const WebString& firstPartyOrigin);

i think it would make sense to switch over to passing WebSecurityOrigin instead of WebString.
it is often better to expose WebSecurityOrigin instead of raw strings.  when createStorageArea
was first created, we probably didn't yet have WebSecurityOrigin or the discipline to pass it
around instead of raw strings.

[Marja Hölttä]

Created attachment 103080 [details]
Patch.

[Marja Hölttä]

It turned out this is not needed, after all. Resolving as invalid.