Bug 34726 – openDatabase() should not ignore the creationCallback if one is specified

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

The spec (http://dev.w3.org/html5/webdatabase/Overview.html) says that openDatabase() can take an optional 'creationCallback' that should be "invoked if the database has not yet been created". WebKit currently ignores this callback. We should fix this.

[Dumitru Daniliuc]

Created attachment 49324 [details]
patch

Sam: can you please take a look at the JSC bindings?
Nate: can you please take a look at the V8 bindings?

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/js/JSDatabaseCallback.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 23 files


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

[Nate Chapin]

(In reply to comment #1)
> Created an attachment (id=49324) [details]
> patch
> 
> Sam: can you please take a look at the JSC bindings?
> Nate: can you please take a look at the V8 bindings?

V8 bindings changes themselves look OK to me, though this patch should probably include Android build file changes?

[Dumitru Daniliuc]

Created attachment 49460 [details]
patch

Fixed the style problem, and I think I made all the necessary changes to the Android build files, but I'm not 100% sure.

[Geoffrey Garen]

JSC bindings look good to me.

[Michael Nordman]

Couple of comments/questions about the Database class. I haven't looked at the v8 or jsc bindings.

> ===================================================================
> --- WebCore/storage/Database.cpp	(revision 55163)
> +++ WebCore/storage/Database.cpp	(working copy)
>  
> -PassRefPtr<Database> Database::openDatabase(ScriptExecutionContext* context, const String& name, const String& expectedVersion, const String& displayName, unsigned long estimatedSize, ExceptionCode& e)
> +class DatabaseCreationCallbackTask : public ScriptExecutionContext::Task {
> +public:
> +    static PassOwnPtr<DatabaseCreationCallbackTask> create(RefPtr<Database> database)

Should the method parameter be a PassRefPtr?

> +    {
> +        return new DatabaseCreationCallbackTask(database);
> +    }
> +
> +    virtual void performTask(ScriptExecutionContext*)
> +    {
> +        m_database->performCreationCallback();
> +    }
> +
> +private:
> +    DatabaseCreationCallbackTask(RefPtr<Database> database)

Ditto

> +        : m_database(database)
> +    {
> +    }
> +
> +    RefPtr<Database> m_database;
> +};
> +
>  
> +    // If it's a new database and a creation callback was provided, reset the expected
> +    // version to "" and schedule the creation callback. Because of some subtle String
> +    // implementation issues, we have to reset m_expectedVersion here instead of doing
> +    // it inside performOpenAndVerify() which is run on the DB thread.
> +    if (database.get() && database->isNew() && database->m_creationCallback.get()) {

I don't think you need to test database.get() here, looks like it can't be null at this point.

> +        database->m_expectedVersion = "";
> +        LOG(StorageAPI, "Scheduling DatabaseCreationCallbackTask for database %p\n", database.get());
> +        database->m_scriptExecutionContext->postTask(DatabaseCreationCallbackTask::create(database));
> +    }
> +
>      return database;
>  }

[Dumitru Daniliuc]

Created attachment 49669 [details]
patch

Addressed Michael's comments.

[Adam Barth]

Comment on attachment 49669 [details]
patch

This looks pretty good, but the one problem I saw is that we might not be calling the database callback in the correct world.  You're only saving a Frame* when you create the callback object.  You need to save something that remembers which world we're in.  There should be some example code for this pattern in the other code that makes callbacks.

Please add a test that the callback is called in the right world.  You can find examples of these tests in the LayoutTests/http/tests/security/isolatedWorlds directory.

[Dumitru Daniliuc]

Created attachment 49768 [details]
patch

Added a test as requested by Adam. Hasn't made any other changes Adam mentioned as I could not find an example, but the test seems to indicate that they are not necessary (it passes in webkit/win and webkit/mac using JSC bindings and chromium/win using V8 bindings).

[Adam Barth]

Ah, I think your test isn't testing quite the right thing.  Can you try this variation?

<!DOCTYPE html>
<html>
<body>
<div id="console"></div>
<script>
function done()
{
    if (window.layoutTestController)
        layoutTestController.notifyDone();
}

function creationCallback(db)
{
    alert("FAIL: Visible in isolated world.");
    done();
}

document.body.foo = "FAIL: document.body.foo visible in isolated world.";

if (window.layoutTestController) {
    layoutTestController.clearAllDatabases();
    layoutTestController.dumpAsText();
    layoutTestController.waitUntilDone();
    layoutTestController.evaluateScriptInIsolatedWorld(
        0,
        "function creationCallback(db)\n" +
        "{\n" +
        "    alert(document.body.foo);\n" +
        "    window.location='javascript:done()';\n" +
        "}\n" +
        "var db = openDatabase('OpenDatabaseCreationCallbackIsolatedWorld', '1.0', '', 1, creationCallback);");
}
</script>
</body>
</html>

[Michael Nordman]

Do the other Database related callbacks do the right thing with regard to isolated worlds? The transaction, statement, and error callbacks.

[Adam Barth]

> Do the other Database related callbacks do the right thing with regard to
> isolated worlds? The transaction, statement, and error callbacks.

Dunno.  I don't mean to hold up this patch with this issue.  We can deal with the isolated world interactions in a separate bug if you'd prefer.

[Dumitru Daniliuc]

Created attachment 49860 [details]
patch

Adam, I think I fixed the isolated worlds problem. Please take another look. The test passes in webkit/win (using JSC bindings) and chromium/win (using V8 bindings).

[Adam Barth]

Comment on attachment 49860 [details]
patch

This looks great.  Thanks!

Do we need to do the same thing for the other database callbacks?

[Dumitru Daniliuc]

(In reply to comment #14)
> (From update of attachment 49860 [details])
> This looks great.  Thanks!
> 
> Do we need to do the same thing for the other database callbacks?

There's a bug opened for that already: https://bugs.webkit.org/show_bug.cgi?id=27698. I will probably get to it in a couple of weeks.

[Dumitru Daniliuc]

Landed as r55474.

[Daniel Bates]

This change caused a test failure of fast/frames/sandboxed-iframe-storage.html across all of the bots, except the Qt Release bot (weird). The Qt Release bot is failing the test: storage/open-database-creation-callback.html.

[David Levin]

Reverted r55474 for reason:

This patch broke fast/frames/sandboxed-iframe-storage.html

Committed r55485: <http://trac.webkit.org/changeset/55485>

[David Levin]

Comment on attachment 49860 [details]
patch

Cleared r+ so it doesn't show up in a commit list.

[Dumitru Daniliuc]

Created attachment 50079 [details]
patch

Same patch, but fixing fast/frames/sandboxed-iframe-storage.html, and skipping one of the new tests on Qt since they don't seem to have LayoutTestController.evaluateScriptInIsolatedWorld() implemented.

[Adam Barth]

Comment on attachment 50079 [details]
patch

ok

[Dumitru Daniliuc]

I forgot to add a couple of lines to the xcode project file, so the "mac" rectangle might turn red. I got it all figured out now and the patch builds and passes the layout tests on Mac. So I will commit it without uploading it, to save a review cycle.

[Dumitru Daniliuc]

Re-landed as r55593.

[Dmitry Titov]

The patch has been reverted: http://trac.webkit.org/changeset/55635

It broke worker-cloneport.html on mac ports: see bug 35819.

[Dumitru Daniliuc]

Created attachment 50461 [details]
patch

Same patch, minus the changes to Document::postTask() that were submitted as part of http://trac.webkit.org/changeset/55816.

[Adam Barth]

Comment on attachment 50461 [details]
patch

okiedokes

[Dumitru Daniliuc]

Re-re-landed as r55823.

[Csaba Osztrogonác]

Guys, you forgot to re-re land my buildfix and adding the new test to Qt Skipped list. I did both of them: http://trac.webkit.org/changeset/55838 and http://trac.webkit.org/changeset/55839.