Bug 61581

Created attachment 95072 [details] Patch

Adam, no idea whether you're the right person to review. Feel free to punt to somebody else

Comment on attachment 95072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95072&action=review > Source/WebCore/storage/StorageArea.h:58 > + virtual bool canAccessStorage(Frame* sourceFrame) const = 0; Typically we don't like to use Frames for security checks because it's slightly loose as to which Document is in the Frame. Instead, we prefer to use Document or (ideally) SecurityOrigin. In this case, it seems like StorageArea is mainly using Frame as its context object, so that might be something to fix in the future rather than in this patch. More generally, it seems like this work should be done at the WebKit layer (not the WebCore layer) since you're just asking the embedder whether this is allowed.

(In reply to comment #3) > (From update of attachment 95072 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95072&action=review > > > Source/WebCore/storage/StorageArea.h:58 > > + virtual bool canAccessStorage(Frame* sourceFrame) const = 0; > > Typically we don't like to use Frames for security checks because it's slightly loose as to which Document is in the Frame. Instead, we prefer to use Document or (ideally) SecurityOrigin. In this case, it seems like StorageArea is mainly using Frame as its context object, so that might be something to fix in the future rather than in this patch. > > More generally, it seems like this work should be done at the WebKit layer (not the WebCore layer) since you're just asking the embedder whether this is allowed. I tried that before, but that meant that I need to add the Frame to each method in StorageArea, and it seemed worse that moving the check to Storage

> I tried that before, but that meant that I need to add the Frame to each method in StorageArea, and it seemed worse that moving the check to Storage How does the storage area know which storage to interact with? That's what you should use to do the security check.

Looks like most of them take a Frame anyway to check quota.

Created attachment 95087 [details] Patch

Comment on attachment 95072 [details] Patch Attachment 95072 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8740237

Comment on attachment 95087 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95087&action=review Generally looks good. I'll look in more detail when I get to a real computer. > Source/WebCore/ChangeLog:7 > + I would add more explanation to the changelog explaining why we're implement this, especially why this calls out to the embedder.

What's the testing plan for this code?

Created attachment 95127 [details] Patch

Comment on attachment 95127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95127&action=review This looks great. > LayoutTests/platform/chromium/permissionclient/storage-permission.html:22 > + if (window.layoutTestController && layoutTestController.setStorageAllowed) > + layoutTestController.setStorageAllowed(true); You might want to log a message that this test requires layoutTestController.setStorageAllowed so can't be run in a browser (most LayoutTests can be run in a browser) > Source/WebCore/storage/StorageAreaImpl.h:49 > + virtual String key(unsigned index, Frame* sourceFrame) const; Usually we pass the context object as the first argument, but I see that you're just following established convention in this interface. When we change these to take a Document instead of a Frame, we should probably move the parameter to the first position. > Source/WebKit/chromium/public/WebView.h:101 > + virtual WebPermissionClient* permissionClient() = 0; Changes to the public WebKit API require fishd approval. > Source/WebKit/chromium/src/WebViewImpl.cpp:299 > + extra blank line? > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1838 > +void LayoutTestController::setStorageAllowed(const CppArgumentList& arguments, CppVariant* result) I can't believe we're still using this ancient CPPObject interface!

Created attachment 95133 [details] Patch

(In reply to comment #12) > (From update of attachment 95127 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95127&action=review > > This looks great. > > > LayoutTests/platform/chromium/permissionclient/storage-permission.html:22 > > + if (window.layoutTestController && layoutTestController.setStorageAllowed) > > + layoutTestController.setStorageAllowed(true); > > You might want to log a message that this test requires layoutTestController.setStorageAllowed so can't be run in a browser (most LayoutTests can be run in a browser) Done. > > > Source/WebCore/storage/StorageAreaImpl.h:49 > > + virtual String key(unsigned index, Frame* sourceFrame) const; > > Usually we pass the context object as the first argument, but I see that you're just following established convention in this interface. When we change these to take a Document instead of a Frame, we should probably move the parameter to the first position. I added a FIXME comment for that > > > Source/WebKit/chromium/public/WebView.h:101 > > + virtual WebPermissionClient* permissionClient() = 0; > > Changes to the public WebKit API require fishd approval. I dropped that change. Instead, I'm now maintaining a global instance as part of TestShell > > > Source/WebKit/chromium/src/WebViewImpl.cpp:299 > > + > > extra blank line? > > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1838 > > +void LayoutTestController::setStorageAllowed(const CppArgumentList& arguments, CppVariant* result) > > I can't believe we're still using this ancient CPPObject interface!

Comment on attachment 95133 [details] Patch i'll first have to rebase after 61583 has landed

Comment on attachment 95133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95133&action=review > Tools/DumpRenderTree/chromium/WebPermissions.h:43 > + // Sets the policy whether to allow storage or not. > + void setStorageAllowed(bool storageAllowed) { m_storageAllowed = storageAllowed; } Does this state leak from test to test now that this client is global? Maybe we need to call a reset() method on it when we transition between tests?

Created attachment 95191 [details] Patch

(In reply to comment #16) > (From update of attachment 95133 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95133&action=review > > > Tools/DumpRenderTree/chromium/WebPermissions.h:43 > > + // Sets the policy whether to allow storage or not. > > + void setStorageAllowed(bool storageAllowed) { m_storageAllowed = storageAllowed; } > > Does this state leak from test to test now that this client is global? Maybe we need to call a reset() method on it when we transition between tests? done

Comment on attachment 95191 [details] Patch Looks great. Thanks!

Created attachment 95207 [details] Patch

Comment on attachment 95207 [details] Patch rebased

The commit-queue encountered the following flaky tests while processing attachment 95207 [details]: java/lc3/ConvertJSObject/ToLong-002.html bug 61676 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 95207 [details] Patch Rejecting attachment 95207 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 1 Last 500 characters of output: runk) M Source/JavaScriptCore/ChangeLog M Source/JavaScriptCore/Configurations/Base.xcconfig M Source/WebKit/mac/ChangeLog M Source/WebKit/mac/Configurations/Base.xcconfig M Source/WebCore/ChangeLog M Source/WebCore/Configurations/Base.xcconfig M Source/WebKit2/ChangeLog M Source/WebKit2/Configurations/Base.xcconfig r87580 = 99d842dc5ed5aa8decb50e950db3463f077121db (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/8735841

Created attachment 95230 [details] Patch

Comment on attachment 95230 [details] Patch and filled in the reviewer...

Comment on attachment 95230 [details] Patch Clearing flags on attachment: 95230 Committed r87597: <http://trac.webkit.org/changeset/87597>

All reviewed patches have been landed. Closing bug.