Expose Durable Storage API to script

Source/modules/quota/StorageManager.cpp

Index: Source/modules/quota/StorageManager.cpp
diff --git a/Source/modules/quota/StorageManager.cpp b/Source/modules/quota/StorageManager.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..340d14ed34880f3dc923e98ca6aa8178728edaed
--- /dev/null
+++ b/Source/modules/quota/StorageManager.cpp
@@ -0,0 +1,86 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "config.h"
+#include "modules/quota/StorageManager.h"
+
+#include "bindings/core/v8/ScriptPromiseResolver.h"
+#include "core/dom/DOMError.h"

[jsbell, 2015/08/03 19:57:41]

DOMError is not used?

[dgrogan, 2015/08/06 23:52:02]

Only as a vehicle for ExceptionCode.h. Fixed.

+#include "core/dom/Document.h"
+#include "modules/permissions/Permissions.h"
+#include "public/platform/Platform.h"
+#include "public/platform/WebCallbacks.h"
+#include "public/platform/modules/permissions/WebPermissionClient.h"
+#include "public/platform/modules/permissions/WebPermissionStatus.h"
+
+namespace blink {
+
+class DurableStoragePermissionCallbacks : public WebCallbacks<WebPermissionStatus*, void> {

[jsbell, 2015/08/03 19:57:41]

Put this class in anonymous namespace?

[jsbell, 2015/08/03 19:57:41]

final?

[dgrogan, 2015/08/06 23:52:02]

Done.

[dgrogan, 2015/08/06 23:52:02]

Done.

+public:
+    DurableStoragePermissionCallbacks(PassRefPtr<ScriptPromiseResolver> resolver)
+        : m_resolver(resolver)
+    {
+    }
+
+    void onSuccess(WebPermissionStatus* rawStatus) override
+    {
+        OwnPtr<WebPermissionStatus> status = adoptPtr(rawStatus);
+        String toReturn;
+        switch (*status) {
+        case WebPermissionStatusGranted:
+            toReturn = "granted";
+            break;
+        case WebPermissionStatusDenied:
+            toReturn = "denied";
+            break;
+        case WebPermissionStatusPrompt:
+            toReturn = "default";
+            break;
+        }
+        m_resolver->resolve(toReturn);

[Lalit Maganti, 2015/08/04 13:27:41]

Not really any work to be done but very similar code to the above exists in
PermissionStatus - maybe find a nice place to consolidate?

[dgrogan, 2015/08/06 23:52:02]

It'd be awkward to consolidate while the strings are different. But if, as we
talk about elsewhere, this enum is merged with PermissionStatus then a
consolidation would be in order.

+    }
+    void onError() override
+    {
+        ASSERT_NOT_REACHED();

[Lalit Maganti, 2015/08/04 13:27:41]

I think propogating the error to the resolver may be the right thing to do here.

[dgrogan, 2015/08/06 23:52:02]

I was trying to document that this method can never be reached and that I wasn't
omitting it unintentionally. But you'd know better than me, can it ever be
reached? I don't see it being called in permission_dispatcher.cc.

[Lalit Maganti, 2015/08/07 08:47:28]

You're right - currently it cannot be reached. However, I would think that one
day we would make use of this in some way and in that case, it makes sense to be
implemented in a sensible way especially because it's very easy to simply reject
to the promise with no extra effort required.

[dgrogan, 2015/08/10 22:26:40]

I thought about this and still prefer to leave it as is for a few reasons. The
first is that there'd be no way to test the code that I add; there's no way to
invoke the error path because it doesn't exist. Adding untestable code seems
suboptimal. The second is what would I make this signature? Currently I specify
void in the template above. What would I change that to? I'd have to guess as to
what type of the potential future error condition is and if I guess wrong then I
haven't saved the future maintainer any work.

This is good to think through though, so let me know if I've missed something or
you remain unconvinced.

+    }
+
+private:
+    RefPtr<ScriptPromiseResolver> m_resolver;
+};
+
+ScriptPromise StorageManager::requestPersistent(ScriptState* scriptState)
+{
+    RefPtr<ScriptPromiseResolver> resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+    ExecutionContext* executionContext = scriptState->executionContext();
+    SecurityOrigin* securityOrigin = executionContext->securityOrigin();
+    // TODO(dgrogan): Durable is restricted to secure contexts in the browser
+    // process. Can we do it here too, before jumping the process boundary?

[jsbell, 2015/08/03 19:57:41]

Yes, use something like:

String errorMessage;
if (!executionContext->isPrivilegedContext(&errorMessage)) {
  resolver->reject(...);
  return promise;
}

Then on the Chromium side, terminate the renderer(via BadMessage) if a request
comes through from a non-secure origin (since it means this client-side test was
bypassed by a compromised renderer)

[dgrogan, 2015/08/06 23:52:02]

Done, thanks a lot for the pointer.
Makes sense. I'll have to figure out how to do implement it though. The code I
wrote doesn't handle the messages directly but is 2ish steps removed.

+    if (securityOrigin->isUnique()) {
+        resolver->reject(DOMException::create(NotSupportedError));
+        return promise;
+    }
+    if (executionContext->isDocument()) {

[mlamouri (slow - plz ping), 2015/08/04 13:15:19]

Why do you want to check if it's in a document? The call will work and will
return the current permission state if you are not in a frame.

[dgrogan, 2015/08/06 23:52:02]

I should probably rather DCHECK that it's a Document seeing as the IDL calls for
it to not even be exposed to anything but a Window.

I'd been thinking that I want the poor web dev who tries to call
requestPersistent from a worker that it won't actually prompt the user.

+        WebPermissionClient* permissionClient = Permissions::getClient(executionContext);
+        ASSERT(permissionClient);

[Lalit Maganti, 2015/08/04 13:27:41]

Should this be DASSERT or ASSERT? I think you can simply return an exception
like is done in Permissions if this is null.

[dgrogan, 2015/08/06 23:52:02]

I don't think DASSERT exists:
https://code.google.com/p/chromium/codesearch#search/&q=case:yes%20DASSERT&sq...

... but you're right, this case should be handled and not ASSERTed.
Done.

[Lalit Maganti, 2015/08/07 08:47:28]

Apologies - I meant DCHECK

+        permissionClient->requestPermission(WebPermissionTypeDurableStorage, KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new DurableStoragePermissionCallbacks(resolver));
+    } else {
+        resolver->reject(DOMException::create(NotSupportedError));
+        return promise;
+    }
+
+    return promise;
+}
+
+ScriptPromise StorageManager::persistentPermission(ScriptState* scriptState)
+{
+    // TODO(dgrogan): Add a test for calling this method on a file:/// origin.
+    RefPtr<ScriptPromiseResolver> resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+    WebPermissionClient* permissionClient = Permissions::getClient(scriptState->executionContext());
+    permissionClient->queryPermission(WebPermissionTypeDurableStorage, KURL(KURL(), scriptState->executionContext()->securityOrigin()->toString()), new DurableStoragePermissionCallbacks(resolver));
+    return promise;
+}
+
+} // namespace blink