Bug 34726

Summary: openDatabase() should not ignore the creationCallback if one is specified
Product: WebKit Reporter: Dumitru Daniliuc <dumi>
Component: New BugsAssignee: Dumitru Daniliuc <dumi>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, dbates, dglazkov, dimich, ericu, ggaren, japhet, levin, michaeln, ossy, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 35943    
Bug Blocks:    
Attachments:
Description Flags
patch
dumi: commit-queue-
patch
dumi: commit-queue-
patch
abarth: review-, dumi: commit-queue-
patch
dumi: commit-queue-
patch
dumi: commit-queue-
patch
abarth: review+, dumi: commit-queue-
patch abarth: review+, dumi: commit-queue-

Description Dumitru Daniliuc 2010-02-08 14:53:02 PST
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.
Comment 1 Dumitru Daniliuc 2010-02-23 13:45:14 PST
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?
Comment 2 WebKit Review Bot 2010-02-23 13:49:54 PST
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.
Comment 3 Nate Chapin 2010-02-24 17:31:22 PST
(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?
Comment 4 Dumitru Daniliuc 2010-02-24 19:04:57 PST
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.
Comment 5 Geoffrey Garen 2010-02-26 13:16:42 PST
JSC bindings look good to me.
Comment 6 Michael Nordman 2010-02-26 13:46:23 PST
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;
>  }
Comment 7 Dumitru Daniliuc 2010-02-26 18:11:57 PST
Created attachment 49669 [details]
patch

Addressed Michael's comments.
Comment 8 Adam Barth 2010-02-27 01:51:35 PST
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.
Comment 9 Dumitru Daniliuc 2010-03-01 18:17:26 PST
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).
Comment 10 Adam Barth 2010-03-01 21:28:14 PST
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>
Comment 11 Michael Nordman 2010-03-02 12:21:09 PST
Do the other Database related callbacks do the right thing with regard to isolated worlds? The transaction, statement, and error callbacks.
Comment 12 Adam Barth 2010-03-02 12:24:02 PST
> 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.
Comment 13 Dumitru Daniliuc 2010-03-02 15:59:56 PST
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).
Comment 14 Adam Barth 2010-03-02 22:04:58 PST
Comment on attachment 49860 [details]
patch

This looks great.  Thanks!

Do we need to do the same thing for the other database callbacks?
Comment 15 Dumitru Daniliuc 2010-03-03 11:51:11 PST
(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.
Comment 16 Dumitru Daniliuc 2010-03-03 13:49:17 PST
Landed as r55474.
Comment 17 Daniel Bates 2010-03-03 16:17:15 PST
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.
Comment 18 David Levin 2010-03-03 16:38:12 PST
Reverted r55474 for reason:

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

Committed r55485: <http://trac.webkit.org/changeset/55485>
Comment 19 David Levin 2010-03-03 16:40:03 PST
Comment on attachment 49860 [details]
patch

Cleared r+ so it doesn't show up in a commit list.
Comment 20 Dumitru Daniliuc 2010-03-04 18:55:38 PST
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.
Comment 21 Adam Barth 2010-03-04 19:06:58 PST
Comment on attachment 50079 [details]
patch

ok
Comment 22 Dumitru Daniliuc 2010-03-04 19:59:03 PST
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.
Comment 23 Dumitru Daniliuc 2010-03-05 14:18:31 PST
Re-landed as r55593.
Comment 24 Dmitry Titov 2010-03-07 09:03:39 PST
The patch has been reverted: http://trac.webkit.org/changeset/55635

It broke worker-cloneport.html on mac ports: see bug 35819.
Comment 25 Dumitru Daniliuc 2010-03-10 18:11:53 PST
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.
Comment 26 Adam Barth 2010-03-10 18:14:20 PST
Comment on attachment 50461 [details]
patch

okiedokes
Comment 27 Dumitru Daniliuc 2010-03-10 18:19:19 PST
Re-re-landed as r55823.
Comment 28 Csaba Osztrogon√°c 2010-03-11 02:35:45 PST
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.